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