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 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?

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;
	}

	return supported_alignments;
}