On Wed, Sep 4, 2019 at 9:10 PM Aneesh Kumar K.V
<aneesh.kumar@xxxxxxxxxxxxx> wrote:
>
> On 9/5/19 8:29 AM, Dan Williams wrote:
> >>> Keep this 'static' there's no usage of this routine outside of pfn_devs.c
> >>>
> >>>> {
> >>>> - /*
> >>>> - * This needs to be a non-static variable because the *_SIZE
> >>>> - * macros aren't always constants.
> >>>> - */
> >>>> - const unsigned long supported_alignments[] = {
> >>>> - PAGE_SIZE,
> >>>> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>> - HPAGE_PMD_SIZE,
> >>>> + static unsigned long supported_alignments[3];
> >>>
> >>> Why is marked static? It's being dynamically populated each invocation
> >>> so static is just wasting space in the .data section.
> >>>
> >>
> >> The return of that function is address and that would require me to use
> >> a global variable. I could add a check
> >>
> >> /* Check if initialized */
> >> if (supported_alignment[1])
> >> return supported_alignment;
> >>
> >> in the function to updating that array every time called.
> >
> > Oh true, my mistake. I was thrown off by the constant
> > re-initialization. Another option is to pass in the storage since the
> > array needs to be populated at run time. Otherwise I would consider it
> > a layering violation for libnvdimm to assume that
> > has_transparent_hugepage() gives a constant result. I.e. put this
> >
> > unsigned long aligns[4] = { [0] = 0, };
> >
> > ...in align_store() and supported_alignments_show() then
> > nd_pfn_supported_alignments() does not need to worry about
> > zero-initializing the fields it does not set.
>
> That requires callers to track the size of aligns array. If we add
> different alignment support later, we will end up updating all the call
> site?
2 sites for something that gets updated maybe once a decade?
>
> How about?
>
> static const unsigned long *nd_pfn_supported_alignments(void)
> {
> static unsigned long supported_alignments[4];
>
> if (supported_alignments[0])
> return supported_alignments;
>
> supported_alignments[0] = PAGE_SIZE;
>
> if (has_transparent_hugepage()) {
> supported_alignments[1] = HPAGE_PMD_SIZE;
> if (IS_ENABLED(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD))
> supported_alignments[2] = HPAGE_PUD_SIZE;
> }
Again, this makes assumptions that has_transparent_hugepage() always
returns the same result every time it is called.
