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=-14.8 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL 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 A1F2DC47082 for ; Thu, 27 May 2021 00:58:00 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 1D5886128B for ; Thu, 27 May 2021 00:58:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1D5886128B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B06C46B0036; Wed, 26 May 2021 20:57:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AB6836B006E; Wed, 26 May 2021 20:57:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90BD16B0070; Wed, 26 May 2021 20:57:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0007.hostedemail.com [216.40.44.7]) by kanga.kvack.org (Postfix) with ESMTP id 4952A6B0036 for ; Wed, 26 May 2021 20:57:59 -0400 (EDT) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id E20F6AD4D for ; Thu, 27 May 2021 00:57:58 +0000 (UTC) X-FDA: 78185199036.14.2AFAAFD Received: from mail-qt1-f181.google.com (mail-qt1-f181.google.com [209.85.160.181]) by imf04.hostedemail.com (Postfix) with ESMTP id ADAA12C0 for ; Thu, 27 May 2021 00:57:54 +0000 (UTC) Received: by mail-qt1-f181.google.com with SMTP id h24so2341027qtm.12 for ; Wed, 26 May 2021 17:57:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=VlAnyDw6pOEr4UrVszLcWbhU5bthiU7s1qIDq+xEOhg=; b=pCEe7RNPWnZ4rQBQ8SmjheElLpr1XVKjKcZt8DigpqtuH06dvNcxuWjLsT2XqVkraO soY1VcdN/ioF2VCB0b8mUg1CJcb+ssnQDXUY4tXAkOcl2wRu7CByql8gUxhH8PuaQnhx Q+l0GRyKQmnAjf9TAP1FaWKf09oRK0KCzaJE5O9hd6M8SugQidU20pJckcnUCEHekhm5 nfq0Qveh8tjf4KydffPgoL8Hny8Dpxf5RF4eWpR9ORU52lnE00qEyCS2SK5HVe9cTwhQ Q6tD0+WF4ua0dUM5UufvsZfISHQwHPdVi3uKUq+9LqfsNvdPv69iKEaaALmN6NBjGtWv qG0w== 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:in-reply-to:message-id :references:user-agent:mime-version; bh=VlAnyDw6pOEr4UrVszLcWbhU5bthiU7s1qIDq+xEOhg=; b=JOoSV/nR+tXq/6hgI+pMQ32WwvCglF3/xAOVFcUnTPvSqPKZWqnp3d7aKoOgukJqxF B44qlOHD3AyjrZQuVh5PjVvAJP54hS9jkHYIPM6LLUXDA2Dou6ANxiKA9PyNG9KgxwQu T1uPoPhGujVCuDGSjNb73nSaoTkXhIEfAFf4DEQ9bhYjCj1qhpj1R99FU4Iot2FS+UwG staZV/5z2G47bVBbZ7wU2lkj1DtQ8eGbgRFMEtKRNXCDcXuHWz08aPm3UC4+j1lEdC0I KeQ6c3lG7AkbYQXPsEc7W4Zlg7V1hIEohcjRzd/nprfJnruOHaB6AtdsWFCfEpTww8AC 7AWw== X-Gm-Message-State: AOAM530tR4ocaPWnM1Ln83SAnFc72Jb3+trQF1obcgC0NkNIFSjCqUHq pH+lNXkTvJfYGrKcc3/QA28q5g== X-Google-Smtp-Source: ABdhPJyvTOWEKd3+R9wmG3haRULo6mVhKdxafqBgRIXxQTrugp+kKU1rsHASJdFs/a00tMWwsP5eDg== X-Received: by 2002:a05:622a:40d:: with SMTP id n13mr885951qtx.59.1622077077752; Wed, 26 May 2021 17:57:57 -0700 (PDT) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id d28sm399733qkl.105.2021.05.26.17.57.56 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Wed, 26 May 2021 17:57:57 -0700 (PDT) Date: Wed, 26 May 2021 17:57:40 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Yang Shi cc: Hugh Dickins , Zi Yan , "Kirill A. Shutemov" , =?UTF-8?Q?HORIGUCHI_NAOYA=28=E5=A0=80=E5=8F=A3_=E7=9B=B4=E4=B9=9F=29?= , Wang Yugui , Andrew Morton , Linux MM , Linux Kernel Mailing List Subject: Re: [v3 PATCH 2/2] mm: thp: check page_mapped instead of page_mapcount for split In-Reply-To: Message-ID: References: <20210525162145.3510-1-shy828301@gmail.com> <20210525162145.3510-2-shy828301@gmail.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Rspamd-Queue-Id: ADAA12C0 Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=pCEe7RNP; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of hughd@google.com designates 209.85.160.181 as permitted sender) smtp.mailfrom=hughd@google.com X-Rspamd-Server: rspam03 X-Stat-Signature: zg86pdbb5h1t4m7eqe7bfajd5fd6g9sb X-HE-Tag: 1622077074-225022 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, 26 May 2021, Yang Shi wrote: > On Wed, May 26, 2021 at 3:48 PM Hugh Dickins wrote: > > On Wed, 26 May 2021, Yang Shi wrote: > > > On Tue, May 25, 2021 at 4:58 PM Hugh Dickins wrote: > > > > On Tue, 25 May 2021, Yang Shi wrote: > > > > > > > > > > We should be able to make dump_page() print total mapcount, right? The > > > > > dump_page() should be just called in some error paths so taking some > > > > > extra overhead to dump more information seems harmless, or am I > > > > > missing something? Of course, this can be done in a separate patch. > > > > > > > > I didn't want to ask that of you, but yes, if you're willing to add > > > > total_mapcount() into dump_page(), I think that would be ideal; and > > > > could be helpful for other cases too. > > > > > > > > Looking through total_mapcount(), I think it's safe to call from > > > > dump_page() - I always worry about extending crash info with > > > > something that depends on a maybe-corrupted pointer which would > > > > generate a further crash and either recurse or truncate the output - > > > > but please check that carefully. > > > > > > Yes, it is possible. If the THP is being split, some VM_BUG_* might be > > > triggered if total_mapcount() is called. But it is still feasible to > > > print total mapcount as long as we implement a more robust version for > > > dump_page(). > > > > Oh dear. I think the very last thing the kernel needs is yet another > > subtly different variant of *mapcount*(). > > > > Do you have a specific VM_BUG_* in mind there? Of course there's > > the VM_BUG_ON_PAGE(PageTail) at the start of it, and you'd want to > > print total_mapcount(head) to avoid that one. > > There are two more places in total_mapcount() other than the tail page > assertion. > > #1. compound_mapcount() has !PageCompound assertion. The similar > problem has been met before, please refer to commit 6dc5ea16c86f ("mm, > dump_page: do not crash with bad compound_mapcount()"). Thanks for the useful reference. > #2. PageDoubleMap has !PageHead assertion. > > > > > Looks like __dump_page() is already careful about "head", checking > > whether "page" is within the expected bounds. Of course, once we're > > in serious VM_WARN territory, there might be races which could flip > > fields midway: PageTail set by the time it reaches total_mapcount()? > > It seems possible, at least theoretically. > > > Narrow the race (rather like it does with PageSlab) by testing > > PageTail immediately before calling total_mapcount(head)? > > TBH I don't think of a simple testing to narrow all the races. We have > to add multiple testing in total_mapcount(), it seems too hacky. > Another variant like below might be neater? > > +static inline int __total_mapcount(struct page *head) > +{ > + int i, compound, nr, ret; > + > + compound = head_compound_mapcount(head); > + nr = compound_nr(head); > + if (PageHuge(head)) > + return compound; > + ret = compound; > + for (i = 0; i < nr; i++) > + ret += atomic_read(&head[i]._mapcount) + 1; > + /* File pages has compound_mapcount included in _mapcount */ > + if (!PageAnon(head)) > + return ret - compound * nr; > + if (head[1].flags & PG_double_map) > + ret -= nr; > + return ret; > +} I still don't want any more of those lovely functions. My current preference is just to drop the idea of trying to show total_mapcount from __dump_page(). I might end up compromising on printing the result of for (i = 0; i < nr; i++) ret += atomic_read(&head[i]._mapcount) + 1; (if we can safely decide nr), and leave all the convoluted flags logic to the poor reader of the page dump. But that in itself shows the limitation of printing total_mapcount(), when individual corrupt _mapcounts might be -1, +2, -3, +4, ... To some extent (modulo racing references to the THP), a good total_mapcount can be inferred from the page reference count. (But that probably describes better the situation when everything is going correctly: maybe problems tend to come precisely when there are multiple racing references.) Dunno. My mind is not on it at the moment. I'm more concerned that Wang Yugui's crash turns out not to be solved by any of the fixes I have lined up: we certainly did not promise that it would be, and it shouldn't stop advancing the fixes we already know, but I do want to give it a little more thought before resuming on my patches. Hugh