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=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 5A677C4707C for ; Fri, 21 May 2021 17:16:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D2E8F61163 for ; Fri, 21 May 2021 17:16:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D2E8F61163 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5FA72940021; Fri, 21 May 2021 13:16:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 57D3494001C; Fri, 21 May 2021 13:16:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0CC24940021; Fri, 21 May 2021 13:16:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0114.hostedemail.com [216.40.44.114]) by kanga.kvack.org (Postfix) with ESMTP id BB20294001C for ; Fri, 21 May 2021 13:16:56 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4552282499A8 for ; Fri, 21 May 2021 17:16:56 +0000 (UTC) X-FDA: 78165893232.21.D1EDE2A Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by imf25.hostedemail.com (Postfix) with ESMTP id D6D3C6000265 for ; Fri, 21 May 2021 17:16:52 +0000 (UTC) Received: by mail-ej1-f47.google.com with SMTP id u21so31492411ejo.13 for ; Fri, 21 May 2021 10:16:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=h4/CwNk0FN5qvx+50wn7PTfWmi1N+v6lF6hq8B8Z0+M=; b=KXVt6s5DY0WD87o/HNLXBpqqo1MqEcSRjiohfszAFUncp7HQiMkeAvjtkvqR3OPYN7 03qdYY4RnBnbrnovcyyrHXQ3a5LNSHilQlcKHkiuQsts7ajUohjhkw4RDTnnY2kljWb8 RA4aeTRK0BDFOHTeMQwLTWTDUOWNC2CZHxxrbJH9S1zkPgAzAhwRoCHLS2I5vCwl4mXz OwnHAe1FOodgKpySSIxzWO02VxfKMc2W/Ho7if4QQ7gUrmOnKg3g5HcUeHeJ7crN6l4J SaJ9kZO6ckVk96R5JK5c4gBI1y/WeZLLXJlTrzYqbxbQjUnUaDtoEjkZddYfKH6+6vY+ 7RGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=h4/CwNk0FN5qvx+50wn7PTfWmi1N+v6lF6hq8B8Z0+M=; b=lsOg+ZXhObWDKDGCB7d/IPI5AGZPMyH1SXeqA8uWTQipZPzy6KJvSCyG0RF82VrEks vR3cHc0o7nWDYpEU73P195gbP1R8+scoGsInCKYjNdQ/fSXsY2Vx/FsX3+hcXb96Jn+H d6qJFhOpk01Z1vMja1S2SdmcdnawJiMPqzLjcA58+Xf8X801YECBb4ujcfleebgXp3WT FNN0k5lJRX0EGJ2UXY/UkN9nKZ3tXk2vdh5Je5ygTERxnE219K9AqtXyaurKrlT0NRaD PoWcEhNRwlBoUxlKDh5axFCWiTLLF4WWBfuiEdL3+Gc5+BfuTAoJO2LZZCo0HFmWwaCY SepQ== X-Gm-Message-State: AOAM530ue5Hcn3KX+9LDgZ/gM6V98B8ceqnj8Yi5FHW0V/bOvTGeJx6o fE2AMAMlpe/grQPDTLPT5UxGPwaXEelQjtPOgBY= X-Google-Smtp-Source: ABdhPJwYPFLbDho6OUZa5Gd2kS+qNLXtO1L5UeCv1ZEkEBjMF2CDlJBrEwLVl/d5F2SQc/WNJuJFGpb79qnY8g+xCZg= X-Received: by 2002:a17:906:a51:: with SMTP id x17mr11482393ejf.25.1621617414570; Fri, 21 May 2021 10:16:54 -0700 (PDT) MIME-Version: 1.0 References: <20210513212334.217424-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Fri, 21 May 2021 10:16:41 -0700 Message-ID: Subject: Re: [v2 PATCH] mm: thp: check total_mapcount instead of page_mapcount To: Hugh Dickins Cc: Zi Yan , "Kirill A. Shutemov" , Wang Yugui , Andrew Morton , Linux MM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: D6D3C6000265 Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=KXVt6s5D; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspamd-Server: rspam03 X-Stat-Signature: bhqwj4iextb13kbnnudsfe5p4iqiqkso X-HE-Tag: 1621617412-747499 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 Thu, May 20, 2021 at 10:06 PM Hugh Dickins wrote: > > On Thu, 13 May 2021, Yang Shi wrote: > > > When debugging the bug reported by Wang Yugui [1], try_to_unmap() may > > return false positive for PTE-mapped THP since page_mapcount() is used > > to check if the THP is unmapped, but it just checks compound mapount and > > head page's mapcount. If the THP is PTE-mapped and head page is not > > mapped, it may return false positive. > > > > Use total_mapcount() instead of page_mapcount() for try_to_unmap() and > > do so for the VM_BUG_ON_PAGE in split_huge_page_to_list as well. > > > > This changed the semantic of try_to_unmap(), but I don't see there is > > any usecase that expects try_to_unmap() just unmap one subpage of a huge > > page. So using page_mapcount() seems like a bug. > > > > [1] https://lore.kernel.org/linux-mm/20210412180659.B9E3.409509F4@e16-tech.com/ > > > > Signed-off-by: Yang Shi > > I don't object to this patch, I've no reason to NAK it; but I'll > point out a few deficiencies which might make you want to revisit it. > > > --- > > v2: Removed dead code and updated the comment of try_to_unmap() per Zi > > Yan. > > > > mm/huge_memory.c | 11 +---------- > > mm/rmap.c | 10 ++++++---- > > 2 files changed, 7 insertions(+), 14 deletions(-) > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 63ed6b25deaa..3b08b9ba1578 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -2348,7 +2348,6 @@ static void unmap_page(struct page *page) > > ttu_flags |= TTU_SPLIT_FREEZE; > > > > unmap_success = try_to_unmap(page, ttu_flags); > > - VM_BUG_ON_PAGE(!unmap_success, page); > > The unused variable unmap_success has already been reported and > dealt with. But I couldn't tell what you intended: why change > try_to_unmap()'s output, if you then ignore it? Because some other callers of try_to_unmap() check the output. > > > } > > > > static void remap_page(struct page *page, unsigned int nr) > > @@ -2718,7 +2717,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > > } > > > > unmap_page(head); > > - VM_BUG_ON_PAGE(compound_mapcount(head), head); > > + VM_BUG_ON_PAGE(total_mapcount(head), head); > > And having forced try_to_unmap() to do the expensive-on-a-THP > total_mapcount() calculation, you now repeat it here. Better > to stick with the previous VM_BUG_ON_PAGE(!unmap_success). > > Or better a VM_WARN_ONCE(), accompanied by dump_page()s as before, > to get some perhaps useful info out, which this patch has deleted. > Probably better inside unmap_page() than cluttering up here. Moving the BUG or WARN into unmap_page() looks fine to me. IIUC, VM_BUG_ON_PAGE or VM_WARN_ON_PAGE does call dump_page(), so dumping something useful is not deleted. > > VM_WARN_ONCE() because nothing in this patch fixes whatever Wang > Yugui is suffering from; and (aside from the BUG()) it's harmless, > because there are other ways in which the page_ref_freeze() can fail, > and that is allowed for. We would like to know when this problem > occurs: there is something wrong, but no reason to crash. Yes, it fixes nothing. I didn't figure out why try_to_unmap() failed. I agree BUG_ON could be relaxed. > > > > > /* block interrupt reentry in xa_lock and spinlock */ > > local_irq_disable(); > > @@ -2758,14 +2757,6 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) > > __split_huge_page(page, list, end); > > ret = 0; > > } else { > > - if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) { > > - pr_alert("total_mapcount: %u, page_count(): %u\n", > > - mapcount, count); > > - if (PageTail(page)) > > - dump_page(head, NULL); > > - dump_page(page, "total_mapcount(head) > 0"); > > - BUG(); > > - } > > This has always looked ugly (as if Kirill had hit an unsolved case), > so it is nice to remove it; but you're losing the dump_page() info, > and not really gaining anything more than a cosmetic cleanup. As I mentioned above, IIUC VM_BUG_ON_PAGE and VM_WARN_ON_PAGE do call dump_page(). > > > spin_unlock(&ds_queue->split_queue_lock); > > fail: if (mapping) > > xa_unlock(&mapping->i_pages); > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 693a610e181d..f52825b1330d 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -1742,12 +1742,14 @@ static int page_not_mapped(struct page *page) > > } > > > > /** > > - * try_to_unmap - try to remove all page table mappings to a page > > - * @page: the page to get unmapped > > + * try_to_unmap - try to remove all page table mappings to a page and the > > + * compound page it belongs to > > + * @page: the page or the subpages of compound page to get unmapped > > * @flags: action and flags > > * > > * Tries to remove all the page table entries which are mapping this > > - * page, used in the pageout path. Caller must hold the page lock. > > + * page and the compound page it belongs to, used in the pageout path. > > + * Caller must hold the page lock. > > * > > * If unmap is successful, return true. Otherwise, false. > > */ > > @@ -1777,7 +1779,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags) > > else > > rmap_walk(page, &rwc); > > > > - return !page_mapcount(page) ? true : false; > > + return !total_mapcount(page) ? true : false; > > That always made me wince: "return !total_mapcount(page);" surely. But page_mapcount() seems not correct, it may return false positive, right? Or it is harmless? And I actually spotted a few other places which should use total_mapcount() but using page_mapcount() instead, for example, some madvise code check if the page is shared by using page_mapcount(), however it may return false negative (double mapped THP, but head page is not PTE-mapped, just like what Wang Yugui reported). It is not fatal, but not expected behavior. I understand total_mapcount() is expensive, so is it a trade-off between cost and correctness or just overlooked the false negative case in the first place? I can't tell. > > Or slightly better, "return !page_mapped(page);", since at least that > one breaks out as soon as it sees a mapcount. Though I guess I'm > being silly there, since that case should never occur, so both > total_mapcount() and page_mapped() scan through all pages. > > Or better, change try_to_unmap() to void: most callers ignore its > return value anyway, and make their own decisions; the remaining > few could be changed to do the same. Though again, I may be > being silly, since the expensive THP case is not the common case. I'd say half callers ignore its return value. But I think it should be worth doing. At least we could remove half unnecessary total_mapcount() or page_mapped() call. Thanks a lot for all the suggestions, will incorporate them in the new version. > > > } > > > > /** > > -- > > 2.26.2