Date:   Tue, 13 Jul 2021 03:15:10 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, Christoph Hellwig <hch@....de>,
        Jeff Layton <jlayton@...nel.org>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        William Kucharski <william.kucharski@...cle.com>,
        David Howells <dhowells@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v13 010/137] mm: Add folio flag manipulation functions

On Mon, Jul 12, 2021 at 08:24:09PM -0400, Johannes Weiner wrote:
> On Mon, Jul 12, 2021 at 04:04:54AM +0100, Matthew Wilcox (Oracle) wrote:
> > +/* Whether there are one or multiple pages in a folio */
> > +static inline bool folio_single(struct folio *folio)
> > +{
> > +	return !folio_head(folio);
> > +}
> 
> Reading more converted code in the series, I keep tripping over the
> new non-camelcased flag testers.

Added PeterZ as he asked for it.

https://lore.kernel.org/linux-mm/20210419135528.GC2531743@casper.infradead.org/

> It's not an issue when it's adjectives: folio_uptodate(),
> folio_referenced(), folio_locked() etc. - those are obvious. But nouns
> and words that overlap with struct member names can easily be confused
> with non-bool accessors and lookups. Pop quiz: flag test or accessor?
> 
> folio_private()
> folio_lru()
> folio_nid()
> folio_head()
> folio_mapping()
> folio_slab()
> folio_waiters()

I know the answers to each of those, but your point is valid.  So what's
your preferred alternative?  folio_is_lru(), folio_is_uptodate(),
folio_is_slab(), etc?  I've seen suggestions for folio_test_lru(),
folio_test_uptodate(), and I don't much care for that alternative.

> This requires a lot of double-taking on what is actually being
> queried. Bool types, ! etc. don't help, since we test pointers for
> NULL/non-NULL all the time.
> 
> I see in a later patch you changed the existing page_lru() (which
> returns an enum) to folio_lru_list() to avoid the obvious collision
> with the PG_lru flag test. page_private() has the same problem but it
> changed into folio_get_private() (no refcounting involved). There
> doesn't seem to be a consistent, future-proof scheme to avoid this new
> class of collisions between flag testing and member accessors.
> 
> There is also an inconsistency between flag test and set that makes me
> pause to think if they're actually testing and setting the same thing:
> 
> 	if (folio_idle(folio))
> 		folio_clear_idle_flag(folio);
> 
> Compare this to check_move_unevictable_pages(), where we do
> 
> 	if (page_evictable(page))
> 		ClearPageUnevictable(page);
> 
> where one queries a more complex, contextual userpage state and the
> other updates the corresponding pageframe bit flag.
> 
> The camelcase stuff we use for page flag testing is unusual for kernel
> code. But the page API is also unusually rich and sprawling. What
> would actually come close? task? inode? Having those multiple
> namespaces to structure and organize the API has been quite helpful.
> 
> On top of losing the flagops namespacing, this series also disappears
> many <verb>_page() operations (which currently optically distinguish
> themselves from page_<noun>() accessors) into the shared folio_
> namespace. This further increases the opportunities for collisions,
> which force undesirable naming compromises and/or ambiguity.
> 
> More double-taking when the verb can be read as a noun: lock_folio()
> vs folio_lock().
> 
> Now, is anybody going to mistake folio_lock() for an accessor? Not
> once they think about it. Can you figure out and remember what
> folio_head() returns? Probably. What about all the examples above at
> the same time? Personally, I'm starting to struggle. It certainly
> eliminates syntactic help and pattern matching, and puts much more
> weight on semantic analysis and remembering API definitions.

Other people have given the opposite advice.  For example,
https://lore.kernel.org/linux-mm/YMmfQNjExNs3cuyq@kroah.com/

> What about functions like shrink_page_list() which are long sequences
> of page queries and manipulations? Many lines would be folio_<foo>
> with no further cue whether you're looking at tests, accessors, or a
> high-level state change that is being tested for success. There are
> fewer visual anchors to orient yourself when you page up and down. It
> quite literally turns some code into blah_(), blah_(), blah_():
> 
>        if (!folio_active(folio) && !folio_unevictable(folio)) {
> 	       folio_del_from_lru_list(folio, lruvec);
> 	       folio_set_active_flag(folio);
> 	       folio_add_to_lru_list(folio, lruvec);
> 	       trace_mm_lru_activate(&folio->page);
> 	}

I actually like the way that looks (other than the trace_mm_lru_activate()
which is pending a conversion from page to folio).  But I have my head
completely down in it, and I can't tell what works for someone who's
fresh to it.  I do know that it's hard to change from an API you're
used to (and that's part of the cost of changing an API), and I don't
know how to balance that against making a more discoverable API.

> Think about the mental strain of reading and writing complicated
> memory management code with such a degree of syntactic parsimony, let
> alone the repetetive monotony.
> 
> In those few lines of example code alone, readers will pause on things
> that should be obvious, and miss grave errors that should stand out.
> 
> Add compatible return types to similarly named functions and we'll
> provoke subtle bugs that the compiler won't catch either.
> 
> There are warts and inconsistencies in our naming patterns that could
> use cleanups. But I think this compresses a vast API into one template
> that isn't nearly expressive enough to adequately communicate and
> manage the complexity of the underlying structure and its operations.

I don't want to dismiss your concerns.  I just don't agree with them.
If there's a consensus on folio_verb() vs verb_folio(), I'm happy to
go back through all these patches and do the rename.

Powered by blists - more mailing lists