> > 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.
> >> + supported_alignments[0] = PAGE_SIZE;
> >> +
> >> + if (has_transparent_hugepage()) {
> >> +
> >> + supported_alignments[1] = HPAGE_PMD_SIZE;
> >> +
> >> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> >> - HPAGE_PUD_SIZE,
> >> -#endif
> >> + supported_alignments[2] = HPAGE_PUD_SIZE;
> >> #endif
> >
> > This ifdef could be hidden in by:
> >
> > if IS_ENABLED(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
> >
> > ...or otherwise moving this to header file with something like
> > NVDIMM_PUD_SIZE that is optionally 0 or HPAGE_PUD_SIZE depending on
> > the config
>
>
> I can switch to if IS_ENABLED but i am not sure that make it any
> different in the current code. So I will keep it same?
It at least follows the general guidance to keep #ifdef out of .c files.
>
> NVDIMM_PUD_SIZE is an indirection I find confusing.
>
Ok.
> >
> > Ok, this is better, but I think it can be clarified further.
> >
> > "For dax vmas, try to always use hugepage mappings. If the kernel does
> > not support hugepages, fsdax mappings will fallback to PAGE_SIZE
> > mappings, and device-dax namespaces, that try to guarantee a given
> > mapping size, will fail to enable."
> >
> > The last sentence about PAGE_SIZE namespaces is not relevant to
> > __transparent_hugepage_enabled(), it's an internal implementation
> > detail of the device-dax driver.
> >
>
> I will use the above update.
>
Thanks.
