Message-ID: <YO23WOUhhZtL6Gtn@cmpxchg.org>
Date:   Tue, 13 Jul 2021 11:55:04 -0400
From:   Johannes Weiner <hannes@...xchg.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Matthew Wilcox <willy@...radead.org>, 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>
Subject: Re: [PATCH v13 010/137] mm: Add folio flag manipulation functions

On Tue, Jul 13, 2021 at 11:15:33AM +0200, Peter Zijlstra wrote:
> 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:
> > > 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/
> 
> 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.
> 
> > > 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.

I agree that _is_ reads nicer by itself, but paired with other ops
such as testset, _test_ might be better.

For example, in __set_page_dirty_no_writeback()

	if (folio_is_dirty())
		return !folio_testset_dirty()

is less clear about what's going on than would be:

	if (folio_test_dirty())
		return !folio_testset_dirty()

My other example wasn't quoted, but IMO set and clear naming should
also match testing to not cause confusion. I.e. the current:

	if (folio_idle())
		folio_clear_idle_flag()

can make you think two different things are being tested and modified
(as in if (page_evictable()) ClearPageUnevictable()). IMO easier:

	if (folio_test_idle())
		folio_clear_idle()

Non-atomics would have the __ modifier in front of folio rather than
read __clear or __set, which works I suppose?

	__folio_clear_dirty()

With all that, we'd have something like:

	folio_test_foo()
	folio_set_foo()
	folio_clear_foo()
	folio_testset_foo()
	folio_testclear_foo()

	__folio_test_foo()
	__folio_set_foo()
	__folio_clear_foo()

Would that be a workable compromise for everybody?

> > > 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/
> 
> 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++.

Yeah the lack of `using` is my concern.

Namespacing is nice for more contained APIs. Classic class + method
type deals, with non-namespaced private helpers implementing public
methods, and public methods not layered past trivial stuff like
foo_insert() calling __foo_insert() with a lock held.

memcg, vmalloc, kobject, you name it.

But the page api is pretty sprawling with sizable overlaps between
interface and implementation, and heavy layering in both. `using`
would be great to avoid excessive repetition where file or function
context already does plenty of namespacing. Alas, it's not an option.

So IMO we're taking a concept of more stringent object-oriented
encapsulation to a large, heavily layered public API without having
the tools e.g. C++ provides to manage exactly such situations.

If everybody agrees we'll be fine, I won't stand in the way. But I do
think the page API is a bit unusual in that regard. And while it is
nice for the outward-facing filesystem interface - and I can see why
fs people love it - the cost of it seems to be carried by the MM
implementation code.

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

Yeah I won't stand in the way if everybody agrees this is fine.

Although I will say, folio_del_from_lru_list() reads a bit like
'a'.append_to(string) to me. lruvec_add_folio() would match more
conventional object hierarchy for container/collection/list/array
interactions, like with list_add, xa_store, rb_insert, etc.

Taking all of the above, we'd have:

	if (!folio_test_active(folio) && !folio_test_unevictable(folio)) {
		lruvec_del_folio(folio, lruvec);
		folio_set_active(folio);
		lruvec_add_folio(folio, lruvec);
		trace_mm_lru_activate(&folio->page);
	}

which reads a little better overall, IMO.

Is that a direction we could agree on?


It still loses the visual anchoring of page state changes. These are
often the "commit" part of multi-step transactions, and having those
cut through the procedural grind a bit is nice - to see more easily
what the code is fundamentally about, what is prerequisite for the
transaction, and what is post-transactional housekeeping noise:

	if (!PageActive(page) && !PageUnevictable(page)) {
		del_page_from_lru_list(page, lruvec);
		SetPageActive(page);
		add_page_to_lru_list(page, lruvec);
		trace_mm_lru_activate(page);
	}

Similar for isolation clearing PG_lru (empties, comments, locals
removed):

		if (page_zonenum(page) > sc->reclaim_idx) {
			list_move(&page->lru, &pages_skipped);
			nr_skipped[page_zonenum(page)] += nr_pages;
			continue;
		}
		scan += nr_pages;
		if (!__isolate_lru_page_prepare(page, mode)) {
			list_move(&page->lru, src);
			continue;
		}
		if (unlikely(!get_page_unless_zero(page))) {
			list_move(&page->lru, src);
			continue;
		}
		if (!TestClearPageLRU(page)) {
			put_page(page);
			list_move(&page->lru, src);
			continue;
		}
		nr_taken += nr_pages;
		nr_zone_taken[page_zonenum(page)] += nr_pages;
		list_move(&page->lru, dst);

Or writeback clearing PG_writeback:

	lock_page_memcg(page);
	if (mapping && mapping_use_writeback_tags(mapping)) {
		xa_lock_irqsave(&mapping->i_pages, flags);
		ret = TestClearPageWriteback(page);
		if (ret) {
			__xa_clear_mark(&mapping->i_pages, page_index(page),
						PAGECACHE_TAG_WRITEBACK);
			if (bdi->capabilities & BDI_CAP_WRITEBACK_ACCT) {
				dec_wb_stat(wb, WB_WRITEBACK);
				__wb_writeout_inc(wb);
			}
		}
		if (mapping->host && !mapping_tagged(mapping,
						     PAGECACHE_TAG_WRITEBACK))
			sb_clear_inode_writeback(mapping->host);
		xa_unlock_irqrestore(&mapping->i_pages, flags);
	} else {
		ret = TestClearPageWriteback(page);
	}
	if (ret) {
		dec_lruvec_page_state(page, NR_WRITEBACK);
		dec_zone_page_state(page, NR_ZONE_WRITE_PENDING);
		inc_node_page_state(page, NR_WRITTEN);
	}
	unlock_page_memcg(page);

It's somewhat unfortunate to lose that bit of extra help when
navigating the code, but I suppose we can live without it.

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

I'll try ;-)

Powered by blists - more mailing lists