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

From: Matthew Wilcox <willy@infradead.org>
To: Nicholas Piggin <npiggin@gmail.com>
Cc: Hugh Dickins <hughd@google.com>,
	akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v8.1 00/31] Memory Folios
Date: Sat, 1 May 2021 15:31:04 +0100	[thread overview]
Message-ID: <20210501143104.GR1847222@casper.infradead.org> (raw)
In-Reply-To: <20210501023711.GP1847222@casper.infradead.org>

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().