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 91890C4707A for ; Fri, 21 May 2021 19:27:29 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 308CE60FE7 for ; Fri, 21 May 2021 19:27:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 308CE60FE7 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 7A3CE94003E; Fri, 21 May 2021 15:27:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 754F194003A; Fri, 21 May 2021 15:27:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5F4E694003E; Fri, 21 May 2021 15:27:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0009.hostedemail.com [216.40.44.9]) by kanga.kvack.org (Postfix) with ESMTP id 2E17894003A for ; Fri, 21 May 2021 15:27:28 -0400 (EDT) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id B7B2310F02 for ; Fri, 21 May 2021 19:27:27 +0000 (UTC) X-FDA: 78166222134.15.D2A9583 Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by imf10.hostedemail.com (Postfix) with ESMTP id A879E40B8CF5 for ; Fri, 21 May 2021 19:27:24 +0000 (UTC) Received: by mail-ej1-f47.google.com with SMTP id z12so30604981ejw.0 for ; Fri, 21 May 2021 12:27:27 -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=p33zgO2dvO4T3zlpBknK48bHhSk8cDL/T5K7X55c9LA=; b=Fmw3sIbNbdideQbu0Jt2y0/cgFto3i/P3elmBi/M1QfGnQX1tuLGkJiTCPwirANK29 kFjpbcyQEyri4rGDpIzkdphKhdJX0ydIDknuUgcGC8uc3wyGeJWfzGDrwWOuu3qhSg/U kkkmGuW8Hyjn1thYJSEsRWXnU3tq4AVi5klgM/zQRS4BS3GaxnaU98L/yGoik8lrHxTW X+Kt6Jcr6HEjAcmF7slCg3/yIecwc0lfRAhZOBsWmgKLYImB3vxSTy2pz3te4iJIQaTB QGROV55ZQn8ejr4IvbcLmS3FqfQj3yfnt80FBKXiYI4n6u5n0VqLQ5tokFW+sgtIAysx VfvQ== 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=p33zgO2dvO4T3zlpBknK48bHhSk8cDL/T5K7X55c9LA=; b=VMJoz1rlE+T0rXp1nwV8DUZ7AIYk6al7Vfk4vtXAQiWDX8sw1oX517NA2gHni8wmYR aoZUAHjOEitnaFzQ+TZ+mtakfvEs5IAtsKIwucghFgDZVtTnuVj+uQNGabfUOtPxnxBi KJ9QJ/em3aocFvs9LNYXJjEt0QW5pBc1Zs0z56xNMHZU/ULMl4125MnrUbEJH3fXFN3W OfYN9iNwmFtmf5mx7IcX0yvL5BYGaAx73UuK4kdO2yKuj/HsN8mUDdKDSLEpmqKLJUW0 fmQ7wIryAMYQUCznZQ3RqNxY2DW5Axj5fIyuVc9UnbE2gOPoF3Zt3WB+dgav/D87V1DK B1Hg== X-Gm-Message-State: AOAM5321Jcw+yoNX5T3E7P3mDAgi9/vYZ1eUbUYXOWuZZvq87ZIjCIju fykmu6llr03X1JAsCi/A8joMmEGeV5BpwsXnzlY= X-Google-Smtp-Source: ABdhPJxan+7gW+Zg554MZhYZ0zNINSTC/jvFQocK0naORdBpX2wvIbVgmqUi0sf5EZDX+lmDWCDwRTmE8nhMg2e22WA= X-Received: by 2002:a17:906:b7d6:: with SMTP id fy22mr11439547ejb.383.1621625246236; Fri, 21 May 2021 12:27:26 -0700 (PDT) MIME-Version: 1.0 References: <20210513212334.217424-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Fri, 21 May 2021 12:27:13 -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" Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=Fmw3sIbN; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf10.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Stat-Signature: chuzttwbjoa9ajn9a7tn9uy95x19gnk3 X-Rspamd-Queue-Id: A879E40B8CF5 X-Rspamd-Server: rspam02 X-HE-Tag: 1621625244-962161 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 Fri, May 21, 2021 at 10:16 AM Yang Shi wrote: > > 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. I misspelled the function name. There is *NOT* VM_WARN_ON_PAGE(), the name is VM_WARN_ON_ONCE_PAGE(). We may need to add VM_WARN_ON_PAGE() since I'd like this warning to be printed every time when it is met. > > > > > 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