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@xxxxxxxxxxxxxxxxxxxx/
>
> 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@xxxxxxxxx/
>
> 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 ;-)
