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 24FBAC4707A for ; Sat, 22 May 2021 00:36:58 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 9AB0F613EA for ; Sat, 22 May 2021 00:36:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9AB0F613EA 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 F21138E007B; Fri, 21 May 2021 20:36:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id ED0B98E0079; Fri, 21 May 2021 20:36:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D23538E007B; Fri, 21 May 2021 20:36:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0221.hostedemail.com [216.40.44.221]) by kanga.kvack.org (Postfix) with ESMTP id 9A7348E0079 for ; Fri, 21 May 2021 20:36:56 -0400 (EDT) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 23B621801D50A for ; Sat, 22 May 2021 00:36:56 +0000 (UTC) X-FDA: 78167002032.27.77453D2 Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by imf02.hostedemail.com (Postfix) with ESMTP id 199B540B8CDE for ; Sat, 22 May 2021 00:36:53 +0000 (UTC) Received: by mail-ej1-f49.google.com with SMTP id c20so32948094ejm.3 for ; Fri, 21 May 2021 17:36: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=yj9Tp8m5cjLjR8k4Nv5ka13mCqgurl+qdeLQ6EUm7JQ=; b=cXKw6py40XjMouzyvbjx9xJMgUO++Qpg8FCs5W8F6Gee0eZ3Tifp4oXq5E8gOl1VIt Oqdgco5kzEsDgvs7q3kSGJ+nJW51W1G1QyXFnupm+pex7KI5aTbqlcu5DQruobjdQTFZ VhhCrqiuf9KTpMa1/oh9O6IS8YGDKQe3gGGL7EKExMZUBbUe2dO4mPObjNEJY5tYgQ0/ Uh28QHKqKnf1MT8XrUCYrys4R2mjc0iQwaFv0tChGvzMW2H7pJg67eZJ/4ErWwO9nYve GmQIDg80cImLJpnOn91LU4uIrwELWn+txGSHniANT5ACSLhyyCmSu9MhWB1JvtBulNQi KNGg== 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=yj9Tp8m5cjLjR8k4Nv5ka13mCqgurl+qdeLQ6EUm7JQ=; b=WUId0E/lxqAaWJVF3GF2Bhw/pPF7V5FvWuPuyH+iPByOWVGS2Rn3qyaqKg6rF57Ixg mvP3oYdP8GVqKE+vKVYwed4uE+qSacF1aX3be+ZAjG3J2fOGY1dhuOjRSsLoAwHGgIas 5SLL9gdSANm4BPJnU/X8f//Bbu48yFPwtbWcYKhCY/QRbp5u9T1EeFF7RCSLNma3xD7p mzj8SGL0SlcTNORoITQJugWBRc41cgcOyUXRd9Dl8NNKJ0kE2LWBJyTlc2Z0lQnYrh8p xYzl5Tp+QL4GgilzhICCDvtp+7Gtlc7hVT13o+etJf53JNpnsQjWGJj9GSUoQQHFyZYK 4LrQ== X-Gm-Message-State: AOAM532StnIhp7s8tbVMhZEsJ+OPSwNpPeE2t9W8UEYuZwyplcVcHh3u IbMOKaDXgTYzH72OtvbKmLRJFnRTxXkWZamP7Po= X-Google-Smtp-Source: ABdhPJyp4OJESdAEaf3/Q7LDxXIkELwBeNIDP5yLqZtyLlI1/u4KjZeIsUimoFV4I1RwyTrgSzCcbBsEg3TImc04D54= X-Received: by 2002:a17:906:2dca:: with SMTP id h10mr12382657eji.507.1621643814311; Fri, 21 May 2021 17:36: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 17:36:42 -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 , Andrea Arcangeli , Linux MM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 199B540B8CDE Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=cXKw6py4; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.49 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspamd-Server: rspam03 X-Stat-Signature: jcf47j1rqnj6bdcjser4cprwb7ygxtcm X-HE-Tag: 1621643813-54225 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 4:17 PM Hugh Dickins wrote: > > On Fri, 21 May 2021, Yang Shi wrote: > > 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. > > memory-failure.c and one in vmscan.c: most callers are not interested, > so replacing a quick atomic_read by a scan of 512 struct pages is not > necessarily a good idea. But I am exaggerating, it's not usually that > bad: probably most of those callers will rarely encounter a THP. > > > > > > > > > > > > > } > > > > > > > > > > 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. > > Yes, you're right, I was forgetting that. The original DEBUG_VM block > does do a separate dump_page() for head and tail (if different), but > I don't think you need to replicate that now: (a) it was more a mark > of frustration at not getting enough info to solve the problem (and > if we really want more info, it's a dump of 512 struct pages needed); > and (b) I think Matthew and others have enhanced dump_page() meanwhile, > to issue some head info when dumping a tail; and (c) most splitters > pass the head anyway, IIRC there's only one or two who pass a tail. Yes, deferred split queue is one of them. We may just get rid of it by passing the head page to reduce the exception case. > > > > > 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. > > I get very confused by all those variants too; and then indeed very > often the one variant you think you want turns out not to exist yet. > > But I'm not sure that printing it every time will be good: isn't it > quite likely that the situation is long-lasting, and that the page > is on the anon deferred or shmem unused list, and will keep coming > back to report the same unhelpful info, just cluttering up the logs? > I'd be more comfortable with _ONCE myself. > > But I'd also suggest dropping the VM_ part of it: we have no idea > how often this happens on non-DEBUG_VM machines in production, > and a pattern might be revealed there which would help to solve it. If we use the non-VM_ version we have to call dump_page() explicitly and duplicate what VM_WARN_ON_ONCE_PAGE() does since WARN_* don't dump struct page at all. > > > > > > > > > > > > > > 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. > > I have to confess, I have known of several reasons why one or other > of those BUG()s can be hit. Sigh. I'll rearrange priorities, research > back through what work I've done and not had time to upstream, filter > out the patches relevant to this "mysterious failure to unmap a THP". > > I did not speak up earlier because I did not notice any obvious > connection between Wang Yugui's occurrence, and anything we have > fixed; and (as usual) there's other work I should be doing. But > it's getting to feel dishonest, having this conversation without > showing our fixes: I'd better sort those out and post (but that > will take me more than a week I expect). That would be great. I tried to figure out the problem, but the bug report doesn't show a pattern, and I didn't get too much by staring at the code. > > Not saying that you should stop with this patch at all: > no, I can easily rebase on top of what you end up with here. Thanks. > > > > > > > > > > > > > > > > > > /* 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? > > I wasn't disagreeing with your use of total_mapcount() in that comment, > just with your propagating the "return boolean() ? true : false" style. > > > > > > > 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. > > I haven't looked through those other places. I do agree with you that > total_mapcount() or page_mapped() is likely to be more correct in most > instances, but it's often more costly than it's worth (on THPs). Kind of. But some of them are not a hot path, for example, madvise. > > And you've reminded me that some months back, Andrea pointed out how > total_mapcount() can race with something, and give the wrong answer: > he was arguing against relying on it (but has patches with additional > locking to make it more reliable - though that gets more complicated). > > In Google prod actually, we have mostly avoided using total_mapcount(), > merely because I feared the cost and couldn't convince myself of its > safety - I'm not saying I ever saw it go wrong, I just didn't feel > comfortable relying on it. Maybe that's part of what I need to post > after sifting through. I'm supposed it should be safe (but of course more costly) as long as the page table lock is held. Very few places call page_mapcount() or total_mapcount() *without* page table lock held, compaction is an example. > > Hugh > > > > > > > > > > > > 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