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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 080B9C4332F for ; Thu, 10 Nov 2022 03:23:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 77A2D8E0001; Wed, 9 Nov 2022 22:23:29 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 72A9D6B0072; Wed, 9 Nov 2022 22:23:29 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5F2D28E0001; Wed, 9 Nov 2022 22:23:29 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 50D896B0071 for ; Wed, 9 Nov 2022 22:23:29 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 1EAB5ABEA8 for ; Thu, 10 Nov 2022 03:23:29 +0000 (UTC) X-FDA: 80116087338.28.18EEC0E Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by imf02.hostedemail.com (Postfix) with ESMTP id BC80D80009 for ; Thu, 10 Nov 2022 03:23:28 +0000 (UTC) Received: by mail-qt1-f179.google.com with SMTP id a27so337549qtw.10 for ; Wed, 09 Nov 2022 19:23:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=bWZz52dVumfaYad7E+XSHRTGcmuqcru/wb7z9nclkNk=; b=C23yth4pnIogWnEBBs6iEXgO2JA/Ddn634YKahJiGxXjlGqwcWB+Z4vNMktgXO8jAc LXwdabCRxq1rxrGtBTXEhFD7rZSa5Al4wQBpypOnIpbE/7IbdAmIyqKzm1rbCjlyY7tn e6SNrBMjutv8GvhSGPa3ubb9Bo3gdcs1CygWQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bWZz52dVumfaYad7E+XSHRTGcmuqcru/wb7z9nclkNk=; b=EK36EnM0anCMLYiOgduOIBrsPK8RakRfj5hcj6nLXbDeLNhgpMs4AMthqhDAsRuITk LGkG0lwq+eh60rjNpchlmkq6TJ2mdE8f+JZ3kD3GcIqy1UkmbLCXfIAqPIvhVKctguO6 AZ1ruD/wG25YkHrGLui/oKp1/Ps0hOTUhMy9Mv92XbRHwPokihB5nX9F1aLIrFBAN0Lt rC2yJRK3m4ZWt4iOe0e2VYLBf+HD1C7JjFvjX9NHiXDM6WCrQtgpFodWrY82S2DxKnIG l9Q2gLHSWJ+esJKYPDDTVDDQKSQmyfBVki9BsYq93/djyC0UGF8z9BayLSctWcdFb+N+ +J2Q== X-Gm-Message-State: ACrzQf1C5pjfaqrzBICNbiq0MFllM8Snr9x0W9vtVJoayOlScqr7xuNV rKlASSjoryGAR2il6gXDbR/13BLGml5U2A== X-Google-Smtp-Source: AMsMyM5EQ9ryQIxeCqf6XRkBJZM7fqbpMAS5jsiDxiGMHOVCAU2cHrXHsKTK3METhIVl7UrSnL0WtA== X-Received: by 2002:ac8:7281:0:b0:3a5:3e54:694 with SMTP id v1-20020ac87281000000b003a53e540694mr36121336qto.647.1668050607723; Wed, 09 Nov 2022 19:23:27 -0800 (PST) Received: from mail-yw1-f179.google.com (mail-yw1-f179.google.com. [209.85.128.179]) by smtp.gmail.com with ESMTPSA id az36-20020a05620a172400b006ce9e880c6fsm12361124qkb.111.2022.11.09.19.23.25 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 09 Nov 2022 19:23:25 -0800 (PST) Received: by mail-yw1-f179.google.com with SMTP id 00721157ae682-370547b8ca0so4924937b3.0 for ; Wed, 09 Nov 2022 19:23:25 -0800 (PST) X-Received: by 2002:a81:8241:0:b0:370:5fad:47f0 with SMTP id s62-20020a818241000000b003705fad47f0mr52859595ywf.441.1668050604805; Wed, 09 Nov 2022 19:23:24 -0800 (PST) MIME-Version: 1.0 References: <5f52de70-975-e94f-f141-543765736181@google.com> In-Reply-To: From: Linus Torvalds Date: Wed, 9 Nov 2022 19:23:08 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 4/3] mm,thp,rmap: handle the normal !PageCompound case first To: Hugh Dickins Cc: Andrew Morton , Johannes Weiner , "Kirill A. Shutemov" , Matthew Wilcox , David Hildenbrand , Vlastimil Babka , Peter Xu , Yang Shi , John Hubbard , Mike Kravetz , Sidhartha Kumar , Muchun Song , Miaohe Lin , Naoya Horiguchi , Mina Almasry , James Houghton , "Zach O'Keefe" , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1668050608; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=bWZz52dVumfaYad7E+XSHRTGcmuqcru/wb7z9nclkNk=; b=y6bp1g5Q77+C3WId4rpAUHj0mZTTq8YNuXu65S9mnwnQEUhv7F1GYNd8ysOG6LQHR6hfPq +mycYUCewbCgjbcws70wWu+Ze7LjCiMCtrukDjBL/7RBFdnUKu1iVQ72lHcsutdnFsZGUM R8pW3lpStJreI7larXRG4NkLtu94tQQ= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=C23yth4p; spf=pass (imf02.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.160.179 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1668050608; a=rsa-sha256; cv=none; b=8LFNySbqv8JaUjWs4Hi2lBG5OLAFJcM9i0moz4/a9qJ+zfF8QuVqShRytBoEX9XeT8uYMe 9aAzaplQSbI0Iq0HLf73UCDuRLSmR1e++x7b4TIrV93n4p47C4sOJEu61XRWq63oxzZtfa hlGBMYRxZV8SCgCD+naQxhNGEjHAkZI= X-Rspamd-Queue-Id: BC80D80009 Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=C23yth4p; spf=pass (imf02.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.160.179 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org; dmarc=none X-Rspamd-Server: rspam10 X-Rspam-User: X-Stat-Signature: e4zrm9c5smxz5wip14u5ewhrz39k169q X-HE-Tag: 1668050608-756725 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 Wed, Nov 9, 2022 at 6:18 PM Hugh Dickins wrote: > > Commit ("mm,thp,rmap: lock_compound_mapcounts() on THP mapcounts") > propagated the "if (compound) {lock} else if (PageCompound) {lock} else > {atomic}" pattern throughout; but Linus hated the way that gives primacy > to the uncommon case: switch to "if (!PageCompound) {atomic} else if > (compound) {lock} else {lock}" throughout. Side note, that 'compound' naming is also on my list of "I'm _really_ not a fan". We actually have a completely different meaning for PageCompound() than the meaning of 'compound' in the rmap functions, and those functions literally mix those meanings if not on the same line, then at least right next to each other. What 'rmap' actually means with 'compound' in the add/remove functions is basically 'not PAGE_SIZE' as far as I can tell. So if I get the energy to do the rmap counts, I will *also* be renaming that horrible thing completely. In fact, I'd be inclined to just pass in the actual page size (possibly a page shift order), which some of the functions want anyway, and which would be a lot clearer than the horrid "compound" name. One reason I find the "compound" name so horrifying is that it is used very much for HUGETLB pages, which I don't think end up ever being marked as PageCompund(), and which are - for various historical reasons - doubly confusing because they use a "pte_t" to describe themselves, even when they are actually using a "pmd_t" or a "pud_t" to actually map the page. So a HUGETLB entry really is (for historical reasons) designed to look like a single last-level pte_t entry, but from an rmap perspective it is explicitly *not* made to look like that at all, completely violating the HUGETLB design. So the rmap code has several really REALLY confusing cases: - the common one: just a page mapped at a *real* pte_t level. To make that more confusing, it can actually be a single-page _part_ of a compound page in the PageCompound() sense, but the rmap 'compound' argument will *not* be set, because from a *mmap* standpoint it's mapped as a single page. This is generally recognized by the rmap code by 'compound' being zero. - a HUGETLB mapping, which uses '->pte' in the page walking (please kill me now) and is *not* using a PageCompund() page, but 'compound' is still set, because from a *mapping* standpoint it's not a final pte_t entry (buit from a MM design standpoint it _was_ supposed to be designed like a single page). This is randomly recognized by looking at the vma flags (using "is_vm_hugetlb_page(vma)") or just inherent in the code itself (ie the 'hugetlb()' functions are only called by code that has tested this situation one way or another) To make things more confusing, some places use PageHeadHuge() instead (but the folio version of said test is called "folio_test_hugetlb()", just so that nobody could possibly ever accuse the HUGETLB code to have consistency). You'd think that PageHeadHuge() is one of the functions that checks the page flag bits. You'd be wrong. It's very very special. - an *actual* PageCompound() page, mapped as such as a THP page (ie mapped by a pmd, not a pte). This may be the same page size as a HUGETLB mapping (and often is), but it's a completely different case in every single way. But like the HUGETLB case, the 'compound' argument will be set, and now it's actually a compound page (but hey, so could the single page mapping case be too). Unlike the HUGETLB case, the page walker does not use ->pte for this, and some of the walkers will very much use that, ie folio_referenced_one() will do if (pvmw.pte) { to distinguish the "natively mapped PageCompound()" case (no pte) from the "map a single page" or from the HUGETLB case (yes pte). There may be more cases than those three, and I may have confused myself and gotten some of the details wrong, but I did want to write the above diatribe out to (a) see if somebody corrects me for any of the cases I enumerated (b) see if somebody can point to yet another odd case (c) see if somebody has suggestions for better and more obvious names for that 'compound' argument in the rmap code I do wish the HUGETLB case didn't use 'pte' for its notion of how HUGETLB entries are mapped, but that's literally how HUGETLB is designed: it started life as a larger last-level pte. It just means that it ends up being very confusing when from a page table walk perspective, you're walking a pud or a pmd entry, and then you see a 'pte_t' instead. An example of that confusion is visible in try_to_unmap_one(), which can be called with a HUGEPTE page (well, folio), and that does while (page_vma_mapped_walk(&pvmw)) { to find the rmap entries, but it can't do that if (pvmw.pte) { test to see what mapping it's walking (since both regular pages and HUGETLB pages use that), so then it just keeps testing what kind of page that was passed in. Which really smells very odd to me, but apparently it works, presumably because unlike THP there can be no splitting. But it's a case where the whole "was it a regular page or a HUGETLB page" is really really confusing/ And mm/hugetlb.c (and places like mm/pagewalk.c too) has a fair number of random casts as a result of this "it's not really a pte_t, but it's designed to look like one" thing. This all really is understandable from a historical context, and from HUGETLB really being just kind of designed to be a bigger page (not a collection of small pages that can be mapped as a bigger entity), but it really does mean that 'rmap' calling those HUGETLB pages 'compound' is conceptually very very wrong. Oh well. That whole HUGETLB model isn't getting fixed, but I think the naming confusion about 'compound' *can* be fixed fairly easily, and we could try to at least avoid having 'compound' and 'PageCompound()' mean totally different things in the same function. I'm not going to do any of this cleanup now, but I wanted to at least voice my concerns. Maybe I'll get around to actually trying to clarify the code later. Because this was all stuff that was *very* confusing when I did the rmap simplification in that (now completely rewritten to explicitly _not_ touch rmap at all) original version of the delayed rmap patch series. Linus