Re: [PATCH v8] libnvdimm/dax: Pick the right alignment default when creating dax devices

[Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Wed, Sep 4, 2019 at 9:10 PM Aneesh Kumar K.V
<aneesh.kumar-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org> 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.