Re: [PATCH v8.1 00/31] Memory Folios

[John Hubbard <jhubbard@nvidia.com>]

On 4/30/21 6:32 PM, Nicholas Piggin wrote:
...
>>>   - Big renaming (thanks to peterz):
>>>     - PageFoo() becomes folio_foo()
>>>     - SetFolioFoo() becomes folio_set_foo()
>>>     - ClearFolioFoo() becomes folio_clear_foo()
>>>     - __SetFolioFoo() becomes __folio_set_foo()
>>>     - __ClearFolioFoo() becomes __folio_clear_foo()
>>>     - TestSetPageFoo() becomes folio_test_set_foo()
>>>     - TestClearPageFoo() becomes folio_test_clear_foo()
>>>     - PageHuge() is now folio_hugetlb()
> 
> If you rename these things at the same time, can you make it clear
> they're flags (folio_flag_set_foo())? The weird camel case accessors at
> least make that clear (after you get to know them).
> 

In addition to pointing out that the name was a page flag, the weird
camel case also meant, "if you try to search for this symbol, you will
be defeated", because the darn thing is constructed via macro
concatenation. Which was a reasonable tradeoff of "cannot find it" vs.
"it's extremely simple, and a macro keeps out the bugs when making lots
of these".

Except that over time, it turned out to be not quite that simple, and
people started adding functionality. So now it's "cannot find it, and
it's also got little goodies hiding in there--maybe!".

And now with this change, we have for example:

     #define CLEARPAGEFLAG_NOOP(uname, lname)				\
     static inline void folio_clear_##lname(struct folio *folio) { }	\
     static inline void ClearPage##uname(struct page *page) {  }

...which is both inconsistent, and as Nicholas points out, no longer
obviously a macro-concatenated name.

Given all that, I'd argue for either:

     a) sticking with the camel case ("ClearFolioFoo), or

     b) changing a bunch of the items to actual written-out names. What's
        the harm? We'd end up with a longer file, but one could grep or
        cscope for the names.

thanks,
-- 
John Hubbard
NVIDIA

> We have a set_page_dirty(), so page_set_dirty() would be annoying.
> page_flag_set_dirty() keeps the easy distinction that SetPageDirty()
> provides.
> 
> Thanks,
> Nick
>