On Tue, Jul 13, 2021 at 03:15:10AM +0100, Matthew Wilcox wrote:

On Mon, Jul 12, 2021 at 08:24:09PM -0400, Johannes Weiner wrote:

quoted

On Mon, Jul 12, 2021 at 04:04:54AM +0100, Matthew Wilcox (Oracle) wrote:

quoted

+/* 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/ (local)

Aye; I hate me some Camels with a passion. And Linux Coding style
explicitly not having Camels these things were always a sore spot. I'm
very glad to see them go.

quoted

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.

Either _is_ or _test_ works for me, with a slight preference to _is_ on
account it of being shorter.

quoted

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

Yes, we -tip folk tend to also prefer consistent prefix_ naming, and
every time something big gets refactorered we make sure to make it so.

Look at it like a namespace; you can read it like
folio::del_from_lru_list() if you want. Obviously there's nothing like
'using folio' for this being C and not C++.

quoted

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.

Yeah, I don't particularly have a problem with the repeated folio_ thing
either, it's something you'll get used to.

I agree that significantly changing the naming of things is a majoy
PITA, but given the level of refactoring at that, I think folio_ beats
pageymcpageface_. Give it some time to get used to it...