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@xxxxxxxxxxxxx> 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
