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

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 9/5/19 10:45 AM, Dan Williams wrote:
> On Wed, Sep 4, 2019 at 9:10 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> 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.
> 

That assuming is true right? For architectures supporting THP we don't 
support changing that during runtime. Allowing to change that during 
runtime would have other impacts.

-aneesh