From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C091C07E95 for ; Tue, 13 Jul 2021 15:55:09 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E542A610C7 for ; Tue, 13 Jul 2021 15:55:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E542A610C7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id E12636B008C; Tue, 13 Jul 2021 11:55:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DE9666B0095; Tue, 13 Jul 2021 11:55:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C8AC76B0096; Tue, 13 Jul 2021 11:55:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0102.hostedemail.com [216.40.44.102]) by kanga.kvack.org (Postfix) with ESMTP id A38EB6B008C for ; Tue, 13 Jul 2021 11:55:08 -0400 (EDT) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 9BB128094CAA for ; Tue, 13 Jul 2021 15:55:07 +0000 (UTC) X-FDA: 78358013454.14.58F5683 Received: from mail-qk1-f174.google.com (mail-qk1-f174.google.com [209.85.222.174]) by imf01.hostedemail.com (Postfix) with ESMTP id 066ED5009F17 for ; Tue, 13 Jul 2021 15:55:06 +0000 (UTC) Received: by mail-qk1-f174.google.com with SMTP id m3so8247412qkm.10 for ; Tue, 13 Jul 2021 08:55:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=KBFRLopOdezb0UodrUpCebJmhvmovia/B4lLwnoHdLs=; b=XfUWsOz2BlkOg4tqZ8lqZuw2GWoctad4pFmKtYmW6w9ehY4KId6tuv/fzvnWG3zadx EJ9W/ux7JMpySOCL4YrYTwtNiBj4N2dXZCHw90P4AlCB0IkfliWPEYz3IPjqV2mwu13U bZ6h/3Cpt/yEQzmLR8gqsAwxzN3tU+Iqnq4Yd00LvDUfGXIKuHVQPzrq21mVy/HJLI6x To1M2Bek3aHsIGSBR4uPfLQmkUpJRdDEV8L5v/5sJDadtiDEVI29Ql0cYZWBPgmpglOd JUws1x8z4tEsBFuIIFoP0jHl+8DuZyn/AU3oafVmztY/lJ47pK5nNWjOzqg7TbLWwgYI GlBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=KBFRLopOdezb0UodrUpCebJmhvmovia/B4lLwnoHdLs=; b=SvwCmoUItjzDR+YGa+7ebGhLNE9Zu3yIad4czHWuB0o08wiRgjAG1ewhC9Q9BI91Gj HRB4razQzp/gbKdjTQolf8EGPZypvEvO+CM0moStDCPd/SsaeOwFrInfF+rTe2aAgKTH 3hKHdJZSCvcLQzr+tizpEWxoJ9CYD9MjXVdEKv33mklEqH+AWwxQpyrVTSmuIAHguKv8 yLE+utDapoz3Q9yCaTnR/ChKOJkK7NiX5S9KDgKzdYBxjgbq8DvDXjYHZQ0y4gL8nkFi hp7iz+SEP1rYZg7senvwJ9Lciwe5eXX1kFx/6oWsVESUraSktlkZMf6I0v7fk3AY1jg0 jrMA== X-Gm-Message-State: AOAM533Jh6QUOp6LghUBZ1o6/xvrEZtUXxXDWfZb7qcP52kKvahUnKiw u4gqFQ5+sIYU6zmZ2EQD6kPo3g== X-Google-Smtp-Source: ABdhPJz4LFRXvnxpqoLZjllpt8vO0c01aPHVzWSD7Ce1uuRsz3kfMNluQNuHRmIkteK1dervFMGYeQ== X-Received: by 2002:a05:620a:1235:: with SMTP id v21mr4805593qkj.360.1626191706116; Tue, 13 Jul 2021 08:55:06 -0700 (PDT) Received: from localhost (70.44.39.90.res-cmts.bus.ptd.net. [70.44.39.90]) by smtp.gmail.com with ESMTPSA id m6sm6950347qtx.9.2021.07.13.08.55.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 13 Jul 2021 08:55:05 -0700 (PDT) Date: Tue, 13 Jul 2021 11:55:04 -0400 From: Johannes Weiner To: Peter Zijlstra Cc: Matthew Wilcox , linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Christoph Hellwig , Jeff Layton , "Kirill A . Shutemov" , Vlastimil Babka , William Kucharski , David Howells , Linus Torvalds , Andrew Morton , Hugh Dickins Subject: Re: [PATCH v13 010/137] mm: Add folio flag manipulation functions Message-ID: References: <20210712030701.4000097-1-willy@infradead.org> <20210712030701.4000097-11-willy@infradead.org> <20210713091533.GB4132@worktop.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210713091533.GB4132@worktop.programming.kicks-ass.net> Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=cmpxchg-org.20150623.gappssmtp.com header.s=20150623 header.b=XfUWsOz2; spf=pass (imf01.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.174 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org X-Rspamd-Server: rspam05 X-Stat-Signature: juysb9eqpb1u5b3sdigxbo37595nmykn X-Rspamd-Queue-Id: 066ED5009F17 X-HE-Tag: 1626191706-499820 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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_ > > > 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 ;-)