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 20732C07E9C for ; Tue, 13 Jul 2021 00:24:16 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A650760FED for ; Tue, 13 Jul 2021 00:24:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A650760FED 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 7DF5B6B0092; Mon, 12 Jul 2021 20:24:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7687F8D0002; Mon, 12 Jul 2021 20:24:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5BB4F8D0001; Mon, 12 Jul 2021 20:24:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0231.hostedemail.com [216.40.44.231]) by kanga.kvack.org (Postfix) with ESMTP id 33FC86B0092 for ; Mon, 12 Jul 2021 20:24:14 -0400 (EDT) Received: from smtpin37.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 41A3A8248047 for ; Tue, 13 Jul 2021 00:24:13 +0000 (UTC) X-FDA: 78355667586.37.C17C940 Received: from mail-qk1-f175.google.com (mail-qk1-f175.google.com [209.85.222.175]) by imf11.hostedemail.com (Postfix) with ESMTP id 78823F0000AA for ; Tue, 13 Jul 2021 00:24:12 +0000 (UTC) Received: by mail-qk1-f175.google.com with SMTP id p202so3435240qka.12 for ; Mon, 12 Jul 2021 17:24:11 -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=yB5qAIs9Fc7CEvMMJ8HjSEc+CJeHsMb3Z3r28kd1YEE=; b=01/+1PeyYqpCfkWSPiQzQ1/4ZzPCpTK75chfxFtWF7xgFFxO8o4Rb6MeI8H1ivl+R1 1ebZc2s7qCJgIHtRRFKwbcgLSUa4zIa4Kn6y6NMGTV1QlR87DV9iAeYNqMq5bDoBrrmC XSi53W67knvsyOnZ1i+6YWyZWQNWcAk+m6zx/QRcYOJTWpZRpKDsnGM0zk4Tqb3GXrN3 beKx5CGDrwhitiApD8OyX43Jrc3VFvy31lLfrNz0N5XDSD7AxLj9gFkWp+qDmeu0p9Yb o+evI0jqah2VbXE5Lt/o15HtAEE+yZjiMaIPwdVonhgkYelnStKJwaS/TWNSzoI3xkZq sQ3g== 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=yB5qAIs9Fc7CEvMMJ8HjSEc+CJeHsMb3Z3r28kd1YEE=; b=Nz50eFAv47KmFgNq+Bvyx7x36OqfI+ur76tdcjvBYj9+waEe/OWnc6thKXgaSUOCeb grkKi9SAOBz/uZd1/QVhnrFpXigGQvoAHomw+FL4tYBGNkfJ2xiT3SORzdzuWpasjOZB svakDWxlOgbY9JbfuYKswodX0v9rSfo4/6JC4Dh8HxZVWfqZe0iHJhgWwIbPWTk5NZBw z8jfhbPmN3g7pmljWoRpCqqNGsK+YLYeImczW1RKl11Gk6jSPMFvt2a0TDhLMqVRnXjY Blf6js+wX1yoprVuO1bjXf7pPlO53Bqw9aPbEn4RK03AGKQCBj9md2EDVSLMlkB572Ta giXw== X-Gm-Message-State: AOAM53340vlmhjRRg9FvucwrG+SioF+WdLVfL/WIT4pbwMCRnltLbCx4 yWK6wV5J9V6gKn9Nc/WptAnR+Q== X-Google-Smtp-Source: ABdhPJyNS88bT9lchdE/k2l0s9hF0E9K9iY57Ozrhl0XABc1zYmIZo6n9Z6iN5rSiOntQZCXbF+IQw== X-Received: by 2002:ae9:dd06:: with SMTP id r6mr1467355qkf.74.1626135851341; Mon, 12 Jul 2021 17:24:11 -0700 (PDT) Received: from localhost ([2620:10d:c091:480::1:a40c]) by smtp.gmail.com with ESMTPSA id x20sm7394463qkp.15.2021.07.12.17.24.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Jul 2021 17:24:10 -0700 (PDT) Date: Mon, 12 Jul 2021 20:24:09 -0400 From: Johannes Weiner To: "Matthew Wilcox (Oracle)" Cc: 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210712030701.4000097-11-willy@infradead.org> Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=cmpxchg-org.20150623.gappssmtp.com header.s=20150623 header.b="01/+1Pey"; spf=pass (imf11.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.175 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org X-Rspamd-Server: rspam05 X-Stat-Signature: op4g99wxtocy977daq6o5dhm9f65ua4c X-Rspamd-Queue-Id: 78823F0000AA X-HE-Tag: 1626135852-956019 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 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 _page() operations (which currently optically distinguish themselves from page_() 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_ 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.