mbox series

[v8.1,00/31] Memory Folios

Message ID 20210430180740.2707166-1-willy@infradead.org (mailing list archive)
Headers show
Series Memory Folios | expand

Message

Matthew Wilcox April 30, 2021, 6:07 p.m. UTC 
Managing memory in 4KiB pages is a serious overhead.  Many benchmarks
benefit from a larger "page size".  As an example, an earlier iteration
of this idea which used compound pages (and wasn't particularly tuned)
got a 7% performance boost when compiling the kernel.

Using compound pages or THPs exposes a serious weakness in our type
system.  Functions are often unprepared for compound pages to be passed
to them, and may only act on PAGE_SIZE chunks.  Even functions which are
aware of compound pages may expect a head page, and do the wrong thing
if passed a tail page.

There have been efforts to label function parameters as 'head' instead
of 'page' to indicate that the function expects a head page, but this
leaves us with runtime assertions instead of using the compiler to prove
that nobody has mistakenly passed a tail page.  Calling a struct page
'head' is also inaccurate as they will work perfectly well on base pages.

We also waste a lot of instructions ensuring that we're not looking at
a tail page.  Almost every call to PageFoo() contains one or more hidden
calls to compound_head().  This also happens for get_page(), put_page()
and many more functions.  There does not appear to be a way to tell gcc
that it can cache the result of compound_head(), nor is there a way to
tell it that compound_head() is idempotent.

This series introduces the 'struct folio' as a replacement for
head-or-base pages.  This initial set reduces the kernel size by
approximately 6kB by removing conversions from tail pages to head pages.
The real purpose of this series is adding infrastructure to enable
further use of the folio.

The medium-term goal is to convert all filesystems and some device
drivers to work in terms of folios.  This series contains a lot of
explicit conversions, but it's important to realise it's removing a lot
of implicit conversions in some relatively hot paths.  There will be very
few conversions from folios when this work is completed; filesystems,
the page cache, the LRU and so on will generally only deal with folios.

The text size reduces by between 6kB (a config based on Oracle UEK)
and 1.2kB (allnoconfig).  Performance seems almost unaffected based
on kernbench.

Current tree at:
https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/folio

(contains another ~120 patches on top of this batch, not all of which are
in good shape for submission)

v8.1:
 - Rebase on next-20210430
 - You need https://lore.kernel.org/linux-mm/20210430145549.2662354-1-willy@infradead.org/ first
 - 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()
   - put_folio() becomes folio_put()
   - get_folio() becomes folio_get()
   - put_folio_testzero() becomes folio_put_testzero()
   - set_folio_count() becomes folio_set_count()
   - attach_folio_private() becomes folio_attach_private()
   - detach_folio_private() becomes folio_detach_private()
   - lock_folio() becomes folio_lock()
   - unlock_folio() becomes folio_unlock()
   - trylock_folio() becomes folio_trylock()
   - __lock_folio_or_retry becomes __folio_lock_or_retry()
   - __lock_folio_async() becomes __folio_lock_async()
   - wake_up_folio_bit() becomes folio_wake_bit()
   - wake_up_folio() becomes folio_wake()
   - wait_on_folio_bit() becomes folio_wait_bit()
   - wait_for_stable_folio() becomes folio_wait_stable()
   - wait_on_folio() becomes folio_wait()
   - wait_on_folio_locked() becomes folio_wait_locked()
   - wait_on_folio_writeback() becomes folio_wait_writeback()
   - end_folio_writeback() becomes folio_end_writeback()
   - add_folio_wait_queue() becomes folio_add_wait_queue()
 - Add folio_young() and folio_idle() family of functions
 - Move page_folio() to page-flags.h and use _compound_head()
 - Make page_folio() const-preserving
 - Add folio_page() to get the nth page from a folio
 - Improve struct folio kernel-doc
 - Convert folio flag tests to return bool instead of int
 - Eliminate set_folio_private()
 - folio_get_private() is the equivalent of page_private() (as folio_private()
   is now a test for whether the private flag is set on the folio)
 - Move folio_rotate_reclaimable() into this patchset
 - Add page-flags.h to the kernel-doc
 - Add netfs.h to the kernel-doc
 - Add a family of folio_lock_lruvec() wrappers
 - Add a family of folio_relock_lruvec() wrappers

v7:
https://lore.kernel.org/linux-mm/20210409185105.188284-1-willy@infradead.org/

Matthew Wilcox (Oracle) (31):
  mm: Introduce struct folio
  mm: Add folio_pgdat and folio_zone
  mm/vmstat: Add functions to account folio statistics
  mm/debug: Add VM_BUG_ON_FOLIO and VM_WARN_ON_ONCE_FOLIO
  mm: Add folio reference count functions
  mm: Add folio_put
  mm: Add folio_get
  mm: Add folio flag manipulation functions
  mm: Add folio_young() and folio_idle()
  mm: Handle per-folio private data
  mm/filemap: Add folio_index, folio_file_page and folio_contains
  mm/filemap: Add folio_next_index
  mm/filemap: Add folio_offset and folio_file_offset
  mm/util: Add folio_mapping and folio_file_mapping
  mm: Add folio_mapcount
  mm/memcg: Add folio wrappers for various functions
  mm/filemap: Add folio_unlock
  mm/filemap: Add folio_lock
  mm/filemap: Add folio_lock_killable
  mm/filemap: Add __folio_lock_async
  mm/filemap: Add __folio_lock_or_retry
  mm/filemap: Add folio_wait_locked
  mm/swap: Add folio_rotate_reclaimable
  mm/filemap: Add folio_end_writeback
  mm/writeback: Add folio_wait_writeback
  mm/writeback: Add folio_wait_stable
  mm/filemap: Add folio_wait_bit
  mm/filemap: Add folio_wake_bit
  mm/filemap: Convert page wait queues to be folios
  mm/filemap: Add folio private_2 functions
  fs/netfs: Add folio fscache functions

 Documentation/core-api/mm-api.rst           |   4 +
 Documentation/filesystems/netfs_library.rst |   2 +
 fs/afs/write.c                              |   9 +-
 fs/cachefiles/rdwr.c                        |  16 +-
 fs/io_uring.c                               |   2 +-
 include/linux/memcontrol.h                  |  58 ++++
 include/linux/mm.h                          | 173 ++++++++++--
 include/linux/mm_types.h                    |  71 +++++
 include/linux/mmdebug.h                     |  20 ++
 include/linux/netfs.h                       |  77 +++--
 include/linux/page-flags.h                  | 222 +++++++++++----
 include/linux/page_idle.h                   |  99 ++++---
 include/linux/page_ref.h                    |  88 +++++-
 include/linux/pagemap.h                     | 276 +++++++++++++-----
 include/linux/swap.h                        |   7 +-
 include/linux/vmstat.h                      | 107 +++++++
 mm/Makefile                                 |   2 +-
 mm/filemap.c                                | 295 ++++++++++----------
 mm/folio-compat.c                           |  37 +++
 mm/internal.h                               |   1 +
 mm/memory.c                                 |   8 +-
 mm/page-writeback.c                         |  72 +++--
 mm/page_io.c                                |   4 +-
 mm/swap.c                                   |  18 +-
 mm/swapfile.c                               |   8 +-
 mm/util.c                                   |  30 +-
 26 files changed, 1247 insertions(+), 459 deletions(-)
 create mode 100644 mm/folio-compat.c

Comments

Hugh Dickins April 30, 2021, 6:47 p.m. UTC | #1 
Adding Linus to the Cc (of this one only): he surely has an interest.

On Fri, 30 Apr 2021, Matthew Wilcox (Oracle) wrote:

> Managing memory in 4KiB pages is a serious overhead.  Many benchmarks
> benefit from a larger "page size".  As an example, an earlier iteration
> of this idea which used compound pages (and wasn't particularly tuned)
> got a 7% performance boost when compiling the kernel.
> 
> Using compound pages or THPs exposes a serious weakness in our type
> system.  Functions are often unprepared for compound pages to be passed
> to them, and may only act on PAGE_SIZE chunks.  Even functions which are
> aware of compound pages may expect a head page, and do the wrong thing
> if passed a tail page.
> 
> There have been efforts to label function parameters as 'head' instead
> of 'page' to indicate that the function expects a head page, but this
> leaves us with runtime assertions instead of using the compiler to prove
> that nobody has mistakenly passed a tail page.  Calling a struct page
> 'head' is also inaccurate as they will work perfectly well on base pages.
> 
> We also waste a lot of instructions ensuring that we're not looking at
> a tail page.  Almost every call to PageFoo() contains one or more hidden
> calls to compound_head().  This also happens for get_page(), put_page()
> and many more functions.  There does not appear to be a way to tell gcc
> that it can cache the result of compound_head(), nor is there a way to
> tell it that compound_head() is idempotent.
> 
> This series introduces the 'struct folio' as a replacement for
> head-or-base pages.  This initial set reduces the kernel size by
> approximately 6kB by removing conversions from tail pages to head pages.
> The real purpose of this series is adding infrastructure to enable
> further use of the folio.
> 
> The medium-term goal is to convert all filesystems and some device
> drivers to work in terms of folios.  This series contains a lot of
> explicit conversions, but it's important to realise it's removing a lot
> of implicit conversions in some relatively hot paths.  There will be very
> few conversions from folios when this work is completed; filesystems,
> the page cache, the LRU and so on will generally only deal with folios.
> 
> The text size reduces by between 6kB (a config based on Oracle UEK)
> and 1.2kB (allnoconfig).  Performance seems almost unaffected based
> on kernbench.
> 
> Current tree at:
> https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/folio
> 
> (contains another ~120 patches on top of this batch, not all of which are
> in good shape for submission)
> 
> v8.1:
>  - Rebase on next-20210430
>  - You need https://lore.kernel.org/linux-mm/20210430145549.2662354-1-willy@infradead.org/ first
>  - 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()
>    - put_folio() becomes folio_put()
>    - get_folio() becomes folio_get()
>    - put_folio_testzero() becomes folio_put_testzero()
>    - set_folio_count() becomes folio_set_count()
>    - attach_folio_private() becomes folio_attach_private()
>    - detach_folio_private() becomes folio_detach_private()
>    - lock_folio() becomes folio_lock()
>    - unlock_folio() becomes folio_unlock()
>    - trylock_folio() becomes folio_trylock()
>    - __lock_folio_or_retry becomes __folio_lock_or_retry()
>    - __lock_folio_async() becomes __folio_lock_async()
>    - wake_up_folio_bit() becomes folio_wake_bit()
>    - wake_up_folio() becomes folio_wake()
>    - wait_on_folio_bit() becomes folio_wait_bit()
>    - wait_for_stable_folio() becomes folio_wait_stable()
>    - wait_on_folio() becomes folio_wait()
>    - wait_on_folio_locked() becomes folio_wait_locked()
>    - wait_on_folio_writeback() becomes folio_wait_writeback()
>    - end_folio_writeback() becomes folio_end_writeback()
>    - add_folio_wait_queue() becomes folio_add_wait_queue()
>  - Add folio_young() and folio_idle() family of functions
>  - Move page_folio() to page-flags.h and use _compound_head()
>  - Make page_folio() const-preserving
>  - Add folio_page() to get the nth page from a folio
>  - Improve struct folio kernel-doc
>  - Convert folio flag tests to return bool instead of int
>  - Eliminate set_folio_private()
>  - folio_get_private() is the equivalent of page_private() (as folio_private()
>    is now a test for whether the private flag is set on the folio)
>  - Move folio_rotate_reclaimable() into this patchset
>  - Add page-flags.h to the kernel-doc
>  - Add netfs.h to the kernel-doc
>  - Add a family of folio_lock_lruvec() wrappers
>  - Add a family of folio_relock_lruvec() wrappers
> 
> v7:
> https://lore.kernel.org/linux-mm/20210409185105.188284-1-willy@infradead.org/
> 
> Matthew Wilcox (Oracle) (31):
>   mm: Introduce struct folio
>   mm: Add folio_pgdat and folio_zone
>   mm/vmstat: Add functions to account folio statistics
>   mm/debug: Add VM_BUG_ON_FOLIO and VM_WARN_ON_ONCE_FOLIO
>   mm: Add folio reference count functions
>   mm: Add folio_put
>   mm: Add folio_get
>   mm: Add folio flag manipulation functions
>   mm: Add folio_young() and folio_idle()
>   mm: Handle per-folio private data
>   mm/filemap: Add folio_index, folio_file_page and folio_contains
>   mm/filemap: Add folio_next_index
>   mm/filemap: Add folio_offset and folio_file_offset
>   mm/util: Add folio_mapping and folio_file_mapping
>   mm: Add folio_mapcount
>   mm/memcg: Add folio wrappers for various functions
>   mm/filemap: Add folio_unlock
>   mm/filemap: Add folio_lock
>   mm/filemap: Add folio_lock_killable
>   mm/filemap: Add __folio_lock_async
>   mm/filemap: Add __folio_lock_or_retry
>   mm/filemap: Add folio_wait_locked
>   mm/swap: Add folio_rotate_reclaimable
>   mm/filemap: Add folio_end_writeback
>   mm/writeback: Add folio_wait_writeback
>   mm/writeback: Add folio_wait_stable
>   mm/filemap: Add folio_wait_bit
>   mm/filemap: Add folio_wake_bit
>   mm/filemap: Convert page wait queues to be folios
>   mm/filemap: Add folio private_2 functions
>   fs/netfs: Add folio fscache functions
> 
>  Documentation/core-api/mm-api.rst           |   4 +
>  Documentation/filesystems/netfs_library.rst |   2 +
>  fs/afs/write.c                              |   9 +-
>  fs/cachefiles/rdwr.c                        |  16 +-
>  fs/io_uring.c                               |   2 +-
>  include/linux/memcontrol.h                  |  58 ++++
>  include/linux/mm.h                          | 173 ++++++++++--
>  include/linux/mm_types.h                    |  71 +++++
>  include/linux/mmdebug.h                     |  20 ++
>  include/linux/netfs.h                       |  77 +++--
>  include/linux/page-flags.h                  | 222 +++++++++++----
>  include/linux/page_idle.h                   |  99 ++++---
>  include/linux/page_ref.h                    |  88 +++++-
>  include/linux/pagemap.h                     | 276 +++++++++++++-----
>  include/linux/swap.h                        |   7 +-
>  include/linux/vmstat.h                      | 107 +++++++
>  mm/Makefile                                 |   2 +-
>  mm/filemap.c                                | 295 ++++++++++----------
>  mm/folio-compat.c                           |  37 +++
>  mm/internal.h                               |   1 +
>  mm/memory.c                                 |   8 +-
>  mm/page-writeback.c                         |  72 +++--
>  mm/page_io.c                                |   4 +-
>  mm/swap.c                                   |  18 +-
>  mm/swapfile.c                               |   8 +-
>  mm/util.c                                   |  30 +-
>  26 files changed, 1247 insertions(+), 459 deletions(-)
>  create mode 100644 mm/folio-compat.c
> 
> -- 
> 2.30.2
Nicholas Piggin May 1, 2021, 1:32 a.m. UTC | #2 
Excerpts from Hugh Dickins's message of May 1, 2021 4:47 am:
> Adding Linus to the Cc (of this one only): he surely has an interest.
> 
> On Fri, 30 Apr 2021, Matthew Wilcox (Oracle) wrote:
> 
>> Managing memory in 4KiB pages is a serious overhead.  Many benchmarks
>> benefit from a larger "page size".  As an example, an earlier iteration
>> of this idea which used compound pages (and wasn't particularly tuned)
>> got a 7% performance boost when compiling the kernel.
>> 
>> Using compound pages or THPs exposes a serious weakness in our type
>> system.  Functions are often unprepared for compound pages to be passed
>> to them, and may only act on PAGE_SIZE chunks.  Even functions which are
>> aware of compound pages may expect a head page, and do the wrong thing
>> if passed a tail page.
>> 
>> There have been efforts to label function parameters as 'head' instead
>> of 'page' to indicate that the function expects a head page, but this
>> leaves us with runtime assertions instead of using the compiler to prove
>> that nobody has mistakenly passed a tail page.  Calling a struct page
>> 'head' is also inaccurate as they will work perfectly well on base pages.
>> 
>> We also waste a lot of instructions ensuring that we're not looking at
>> a tail page.  Almost every call to PageFoo() contains one or more hidden
>> calls to compound_head().  This also happens for get_page(), put_page()
>> and many more functions.  There does not appear to be a way to tell gcc
>> that it can cache the result of compound_head(), nor is there a way to
>> tell it that compound_head() is idempotent.
>> 
>> This series introduces the 'struct folio' as a replacement for
>> head-or-base pages.  This initial set reduces the kernel size by
>> approximately 6kB by removing conversions from tail pages to head pages.
>> The real purpose of this series is adding infrastructure to enable
>> further use of the folio.
>> 
>> The medium-term goal is to convert all filesystems and some device
>> drivers to work in terms of folios.  This series contains a lot of
>> explicit conversions, but it's important to realise it's removing a lot
>> of implicit conversions in some relatively hot paths.  There will be very
>> few conversions from folios when this work is completed; filesystems,
>> the page cache, the LRU and so on will generally only deal with folios.
>> 
>> The text size reduces by between 6kB (a config based on Oracle UEK)
>> and 1.2kB (allnoconfig).  Performance seems almost unaffected based
>> on kernbench.
>> 
>> Current tree at:
>> https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/folio
>> 
>> (contains another ~120 patches on top of this batch, not all of which are
>> in good shape for submission)
>> 
>> v8.1:
>>  - Rebase on next-20210430
>>  - You need https://lore.kernel.org/linux-mm/20210430145549.2662354-1-willy@infradead.org/ first
>>  - 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).

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
Matthew Wilcox May 1, 2021, 2:37 a.m. UTC | #3 
On Sat, May 01, 2021 at 11:32:20AM +1000, Nicholas Piggin wrote:
> Excerpts from Hugh Dickins's message of May 1, 2021 4:47 am:
> > On Fri, 30 Apr 2021, Matthew Wilcox (Oracle) 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).
> 
> We have a set_page_dirty(), so page_set_dirty() would be annoying.
> page_flag_set_dirty() keeps the easy distinction that SetPageDirty()
> provides.

Maybe I should have sent more of the patches in this batch ...

mark_page_accessed() becomes folio_mark_accessed()
set_page_dirty() becomes folio_mark_dirty()
set_page_writeback() becomes folio_start_writeback()
test_clear_page_writeback() becomes __folio_end_writeback()
cancel_dirty_page() becomes folio_cancel_dirty()
clear_page_dirty_for_io() becomes folio_clear_dirty_for_io()
lru_cache_add() becomes folio_add_lru()
add_to_page_cache_lru() becomes folio_add_to_page_cache()
write_one_page() becomes folio_write_one()
account_page_redirty() becomes folio_account_redirty()
account_page_cleaned() becomes folio_account_cleaned()

So the general pattern is that folio_set_foo() and folio_clear_foo()
works on the flag directly.  If we do anything fancy to it, it's
folio_verb_foo() where verb depends on foo.

I'm not entirely comfortable with this.  I'd like to stop modules
from accessing folio_set_dirty() because it's just going to mess
up filesystems.  I just haven't thought of a good way to expose
some flags and not others.

Actually, looking at what filesystems actually use at the moment, it's
quite a small subset:

ClearPageChecked
ClearPageDirty
ClearPageError
ClearPageFsCache
__ClearPageLocked
ClearPageMappedToDisk
ClearPagePrivate2
ClearPagePrivate
ClearPageReferenced
ClearPageUptodate
TestClearPageError
TestClearPageFsCache
TestClearPagePrivate2
TestClearPageDirty

SetPageError
__SetPageLocked
SetPageMappedToDisk
SetPagePrivate2
SetPagePrivate
SetPageUptodate
__SetPageUptodate
TestSetPageDirty
TestSetPageFsCache

several of those are ... confused ... but the vast majority of page flags
don't need to be exposed to filesystems.  Does it make you feel better if
folio_set_dirty() doesn't get exposed outside the VFS?
Matthew Wilcox May 1, 2021, 2:31 p.m. UTC | #4 
On Sat, May 01, 2021 at 03:37:11AM +0100, Matthew Wilcox wrote:
> On Sat, May 01, 2021 at 11:32:20AM +1000, Nicholas Piggin wrote:
> > Excerpts from Hugh Dickins's message of May 1, 2021 4:47 am:
> > > On Fri, 30 Apr 2021, Matthew Wilcox (Oracle) 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).
> > 
> > We have a set_page_dirty(), so page_set_dirty() would be annoying.
> > page_flag_set_dirty() keeps the easy distinction that SetPageDirty()
> > provides.
> 
> Maybe I should have sent more of the patches in this batch ...
> 
> mark_page_accessed() becomes folio_mark_accessed()
> set_page_dirty() becomes folio_mark_dirty()
> set_page_writeback() becomes folio_start_writeback()
> test_clear_page_writeback() becomes __folio_end_writeback()
> cancel_dirty_page() becomes folio_cancel_dirty()
> clear_page_dirty_for_io() becomes folio_clear_dirty_for_io()
> lru_cache_add() becomes folio_add_lru()
> add_to_page_cache_lru() becomes folio_add_to_page_cache()
> write_one_page() becomes folio_write_one()
> account_page_redirty() becomes folio_account_redirty()
> account_page_cleaned() becomes folio_account_cleaned()
> 
> So the general pattern is that folio_set_foo() and folio_clear_foo()
> works on the flag directly.  If we do anything fancy to it, it's
> folio_verb_foo() where verb depends on foo.

After sleeping on this, I now think "Why not both?"

folio_dirty() -- defined in page-flags.h

folio_test_set_dirty_flag()
folio_test_clear_dirty_flag()
__folio_clear_dirty_flag()
__folio_set_dirty_flag()
folio_clear_dirty_flag()
folio_set_dirty_flag() -- generated in filemap.h under #ifndef MODULE

folio_mark_dirty() -- declared in mm.h (this is rare; turns out all kinds of
			crap wants to mark pages as being dirty)
folio_clear_dirty_for_io() -- declared in filemap.h


the other flags would mostly follow this pattern.  i'd also change
folio_set_uptodate() to folio_mark_uptodate().
John Hubbard May 1, 2021, 9:38 p.m. UTC | #5 
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,
Matthew Wilcox May 2, 2021, 12:17 a.m. UTC | #6 
On Sat, May 01, 2021 at 02:38:50PM -0700, John Hubbard wrote:
> 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.

I've always hated that, FWIW.  And you can't add kernel-doc for them
because kernel-doc doesn't understand cpp.  So my current plan (quoting
my other email):

folio_dirty() -- defined in page-flags.h
would have kernel-doc, would be greppable

folio_test_set_dirty_flag()
folio_test_clear_dirty_flag()
__folio_clear_dirty_flag()
__folio_set_dirty_flag()
folio_clear_dirty_flag()
folio_set_dirty_flag() -- generated in filemap.h under #ifndef MODULE
would not have kernel-doc, would not be greppable, would only be used
in core vfs and core mm.

folio_mark_dirty() -- declared in mm.h (this is rare; turns out all kinds of
			crap wants to mark pages as being dirty)
folio_clear_dirty_for_io() -- declared in filemap.h
already have kernel-doc, are greppable, used by filesystems and sometimes
other random code.

> 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!".

I also don't like that.  With what I'm thinking, there are no special
cases hidden in the autogenerated names.  Special things like the current
SetPageUptodate would be in folio_mark_uptodate() and filesystems couldn't
even call folio_set_uptodate().

> Given all that, I'd argue for either:
>     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.

I hope the above makes you happy -- everything a filesystem author needs
gets kernel-doc.  People working inside the VM/VFS still get exposed
to undocumented folio_test_set_foo_flag(), but it's all regular and
autogenerated.
John Hubbard May 2, 2021, 12:42 a.m. UTC | #7 
On 5/1/21 5:17 PM, Matthew Wilcox wrote:
...
>> 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.
> 
> I've always hated that, FWIW.  And you can't add kernel-doc for them
> because kernel-doc doesn't understand cpp.  So my current plan (quoting
> my other email):
> 
> folio_dirty() -- defined in page-flags.h
> would have kernel-doc, would be greppable
> 
> folio_test_set_dirty_flag()
> folio_test_clear_dirty_flag()
> __folio_clear_dirty_flag()
> __folio_set_dirty_flag()
> folio_clear_dirty_flag()
> folio_set_dirty_flag() -- generated in filemap.h under #ifndef MODULE
> would not have kernel-doc, would not be greppable, would only be used
> in core vfs and core mm.
> 
> folio_mark_dirty() -- declared in mm.h (this is rare; turns out all kinds of
> 			crap wants to mark pages as being dirty)
> folio_clear_dirty_for_io() -- declared in filemap.h
> already have kernel-doc, are greppable, used by filesystems and sometimes
> other random code.

Yes, the page dirty stuff is definitely not simple, so it's very good to
move away from the auto-generated names there. Looks like you are down to
just a couple of generated names now, if I'm reading this correctly.

> 
>> 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!".
> 
> I also don't like that.  With what I'm thinking, there are no special
> cases hidden in the autogenerated names.  Special things like the current

Yes, that's a good guideline: no special cases in the auto-generated function
names. Because, either it is a simple, standard auto-generated thing, or
it is something that one needs to read, in order to see exactly what it does.


> SetPageUptodate would be in folio_mark_uptodate() and filesystems couldn't
> even call folio_set_uptodate().
> 
>> Given all that, I'd argue for either:
>>      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.
> 
> I hope the above makes you happy -- everything a filesystem author needs
> gets kernel-doc.  People working inside the VM/VFS still get exposed

If "kernel-doc" is effectively a proxy for "file names are directly visible
in the source code, then I'm a lot happier than I was, yes. :)

> to undocumented folio_test_set_foo_flag(), but it's all regular and
> autogenerated.
> 

Sounds pretty good.


thanks,
John Hubbard May 2, 2021, 12:45 a.m. UTC | #8 
On 5/1/21 5:42 PM, John Hubbard wrote:
> If "kernel-doc" is effectively a proxy for "file names are directly visible
> in the source code, then I'm a lot happier than I was, yes. :)

umm, s/file names/function names/ , please.

  thanks,
Matthew Wilcox May 2, 2021, 2:31 a.m. UTC | #9 
On Sat, May 01, 2021 at 05:42:21PM -0700, John Hubbard wrote:
> On 5/1/21 5:17 PM, Matthew Wilcox wrote:
> > folio_dirty() -- defined in page-flags.h
> > would have kernel-doc, would be greppable
> > 
> > folio_test_set_dirty_flag()
> > folio_test_clear_dirty_flag()
> > __folio_clear_dirty_flag()
> > __folio_set_dirty_flag()
> > folio_clear_dirty_flag()
> > folio_set_dirty_flag() -- generated in filemap.h under #ifndef MODULE
> > would not have kernel-doc, would not be greppable, would only be used
> > in core vfs and core mm.
> > 
> > folio_mark_dirty() -- declared in mm.h (this is rare; turns out all kinds of
> > 			crap wants to mark pages as being dirty)
> > folio_clear_dirty_for_io() -- declared in filemap.h
> > already have kernel-doc, are greppable, used by filesystems and sometimes
> > other random code.
> 
> Yes, the page dirty stuff is definitely not simple, so it's very good to
> move away from the auto-generated names there. Looks like you are down to
> just a couple of generated names now, if I'm reading this correctly.

Six -- test_set, test_clear, __set, __clear, set, clear.

> > I hope the above makes you happy -- everything a filesystem author needs
> > gets kernel-doc.  People working inside the VM/VFS still get exposed
> 
> If "kernel-doc" is effectively a proxy for "file names are directly visible
> in the source code, then I'm a lot happier than I was, yes. :)

I'm thinking about this kind of thing for each flag (uptodate was the
easiest one to start with because it's already not autogenerated):

/**
 * folio_uptodate - Is this folio up to date?
 * @folio: The folio.
 *
 * The uptodate flag is set on a folio when every byte in the folio is at
 * least as new as the corresponding bytes on storage.  Anonymous folios
 * are always uptodate.  If the folio is not uptodate, some of the bytes
 * in it may be; see the is_partially_uptodate() address_space operation.
 */
static inline bool folio_uptodate(struct folio *folio)
{
...

(um, this is going to increase the patch series significantly.  i may
not do this until later.  there's more important things to get in that
are already done and waiting on this initial patch series.)