Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 12 Jul 2021 20:24:09 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
        Jeff Layton <jlayton@kernel.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        William Kucharski <william.kucharski@oracle.com>,
        David Howells <dhowells@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH v13 010/137] mm: Add folio flag manipulation functions
Message-ID: <YOzdKYejOEUbjvMj@cmpxchg.org>
References: <20210712030701.4000097-1-willy@infradead.org>
 <20210712030701.4000097-11-willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210712030701.4000097-11-willy@infradead.org>
Precedence: bulk

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.

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

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.

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);
	}

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.