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 7E470ECAAD5 for ; Fri, 2 Sep 2022 17:30:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E04298D002F; Fri, 2 Sep 2022 13:30:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D8D7E8D0014; Fri, 2 Sep 2022 13:30:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C05AA8D002F; Fri, 2 Sep 2022 13:30:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id AAF8A8D0014 for ; Fri, 2 Sep 2022 13:30:22 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 85A651A1240 for ; Fri, 2 Sep 2022 17:30:22 +0000 (UTC) X-FDA: 79867834284.07.813F5D9 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by imf18.hostedemail.com (Postfix) with ESMTP id 3982D1C0046 for ; Fri, 2 Sep 2022 17:30:22 +0000 (UTC) Received: by mail-pl1-f177.google.com with SMTP id p18so2506350plr.8 for ; Fri, 02 Sep 2022 10:30:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=cOrUWtTrBL3zDaJGPAOPIuPhvPakueyffj9KJ2tlTT4=; b=BE8sVUGOaOsUdYhSmBT7vTpDvHDWvzQDGTHMIlQNsZnjHfGVnIuueCCDcTbhjvK65o 159BgFFdcgoaTt6YrUKubRK1pVYkLNO0nUwJsLvgfKXJRtiekHXLq7MP1peZUZRGSGqq XdbONBuOmMFNP6xau6YsxzPNEBugrC53ZSfnnKkTazkEcsj3fA7I0a7LQMfPTw8n2xT6 mD7PDJ0rvPjbjyppGxGYN32UNmrsV0Qyw68U7CnJh6GoSR7w8wuKo2tU33dOlsMNp7/K yWmdUpEHAcn+EGkQMrNG1XHARNimffDj2uzSlC6SZLkhV3ndW7KIaihGCgr5290pEuKG QInQ== 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; bh=cOrUWtTrBL3zDaJGPAOPIuPhvPakueyffj9KJ2tlTT4=; b=ZGQWfMyMoxDr2Wr6o0FIX+7GioXFSLQuU3cM9yxITxJrUBKSakVGtOdwtfMOqjiZ2d EWb4nx55NoHuX6I3JAbWIvh6dMnnTU97nyK8r4EBGmjBFCqHVx7CPqUCSkpyqf5yiHXx UQ0D7jMnd4iSH4aXOd7PuXAYPE6KUkxDbVk+b8bxUjxFjnhyB1rfRVwYXwlP7tODxQw3 Pt61ZvUdjADWxw9pcI9hMRPh9VPHN/YAjZvfUaO8kqqqljyHCzyQTwDuNx+YimpzmCCi w1uaoY2KKctzhkOA/Ch5CeZvSbeY7dSRnoFebbA3K41L1+muJiwtt75CNg6uLj6nvnWD BhOA== X-Gm-Message-State: ACgBeo22ikbXzkpPIXOPDEOk0U3vS92DDvwv+ELYOGMGNMZYmbVs/vLn tC3DlRTevcygZ2ZsI/LILs0KqIR+gxd51jYerk4= X-Google-Smtp-Source: AA6agR4jl2jkps2aj+vVe+mXkA8/Rt2LeFl1xOiADo7+zDNGDeUL8VGTxljciLMFcUxpxAWmde2pzYGbxQBl4eRxVnY= X-Received: by 2002:a17:90a:bc9:b0:1fb:c5bf:e9d with SMTP id x9-20020a17090a0bc900b001fbc5bf0e9dmr5769637pjd.21.1662139821211; Fri, 02 Sep 2022 10:30:21 -0700 (PDT) MIME-Version: 1.0 References: <20220901222707.477402-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Fri, 2 Sep 2022 10:30:08 -0700 Message-ID: Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse To: Peter Xu Cc: david@redhat.com, kirill.shutemov@linux.intel.com, jhubbard@nvidia.com, jgg@nvidia.com, hughd@google.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662139822; a=rsa-sha256; cv=none; b=pNS9UXIULg+b8uZFtutGkpgWx3Q3ZoBqYq7t7kJLhcJZBO0wTpW+4yErtPkMNnq7epbKJF ckdc3jkBUefj6znNveGl6EFTlU/ptt448bruM4HVvpw/SAkvnqiXU6rUo0F7yGYi/5JAGv jetPCG6p3Z7njIx5aGbeRS976mjWsfE= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=BE8sVUGO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of shy828301@gmail.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662139822; 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=cOrUWtTrBL3zDaJGPAOPIuPhvPakueyffj9KJ2tlTT4=; b=eIGsUfYVNyi1pp4BmBhUvSZXg7TGW39KIhHa8JF0zUdEds/CH20N9XSpaabrvLDUwRH3TK ncOZ1oyhW/+LWm/Z59q2QxALtLYrFKcYgosO75pJrhSOFMFiKRfhtaNJ1ynnCWc+27YI4S mZrpnM+IRn7uQY/odkNUUjrq82z/kEY= X-Stat-Signature: ar3cwftty8fzcg36mejgdh535g6aex4t X-Rspamd-Queue-Id: 3982D1C0046 Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=BE8sVUGO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of shy828301@gmail.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspam-User: X-Rspamd-Server: rspam02 X-HE-Tag: 1662139822-261337 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, Sep 2, 2022 at 9:00 AM Peter Xu wrote: > > On Thu, Sep 01, 2022 at 04:50:45PM -0700, Yang Shi wrote: > > On Thu, Sep 1, 2022 at 4:26 PM Peter Xu wrote: > > > > > > Hi, Yang, > > > > > > On Thu, Sep 01, 2022 at 03:27:07PM -0700, Yang Shi wrote: > > > > Since general RCU GUP fast was introduced in commit 2667f50e8b81 ("mm: > > > > introduce a general RCU get_user_pages_fast()"), a TLB flush is no longer > > > > sufficient to handle concurrent GUP-fast in all cases, it only handles > > > > traditional IPI-based GUP-fast correctly. > > > > > > If TLB flush (or, IPI broadcasts) used to work to protect against gup-fast, > > > I'm kind of confused why it's not sufficient even if with RCU gup? Isn't > > > that'll keep working as long as interrupt disabled (which current fast-gup > > > will still do)? > > > > Actually the wording was copied from David's commit log for his > > PageAnonExclusive fix. My understanding is the IPI broadcast still > > works, but it may not be supported by all architectures and not > > preferred anymore. So we should avoid depending on IPI broadcast IIUC. > > > > > > > > IIUC the issue is you suspect not all archs correctly implemented > > > pmdp_collapse_flush(), or am I wrong? > > > > This is a possible fix, please see below for details. > > > > > > > > > On architectures that send > > > > an IPI broadcast on TLB flush, it works as expected. But on the > > > > architectures that do not use IPI to broadcast TLB flush, it may have > > > > the below race: > > > > > > > > CPU A CPU B > > > > THP collapse fast GUP > > > > gup_pmd_range() <-- see valid pmd > > > > gup_pte_range() <-- work on pte > > > > pmdp_collapse_flush() <-- clear pmd and flush > > > > __collapse_huge_page_isolate() > > > > check page pinned <-- before GUP bump refcount > > > > pin the page > > > > check PTE <-- no change > > > > __collapse_huge_page_copy() > > > > copy data to huge page > > > > ptep_clear() > > > > install huge pmd for the huge page > > > > return the stale page > > > > discard the stale page > > > > > > > > The race could be fixed by checking whether PMD is changed or not after > > > > taking the page pin in fast GUP, just like what it does for PTE. If the > > > > PMD is changed it means there may be parallel THP collapse, so GUP > > > > should back off. > > > > > > Could the race also be fixed by impl pmdp_collapse_flush() correctly for > > > the archs that are missing? Do you know which arch(s) is broken with it? > > > > Yes, and this was suggested by me in the first place, but per the > > suggestion from John and David, this is not the preferred way. I think > > it is because: > > > > Firstly, using IPI to serialize against fast GUP is not recommended > > anymore since fast GUP does check PTE then back off so we should avoid > > it. > > Secondly, if checking PMD then backing off could solve the problem, > > why do we still need broadcast IPI? It doesn't sound performant. > > > > > > > > It's just not clear to me whether this patch is an optimization or a fix, > > > if it's a fix whether the IPI broadcast in ppc pmdp_collapse_flush() would > > > still be needed. > > > > It is a fix and the fix will make IPI broadcast not useful anymore. > > How about another patch to remove the ppc impl too? Then it can be a two > patches series. > > So that ppc developers can be copied and maybe it helps to have the ppc > people looking at current approach too. I was thinking about it before sending this patch, but I thought it may be better to wait for ppc developers to confirm some questions. I think Aneesh was copied in another thread. > > Then the last piece of it is the s390 pmdp_collapse_flush(). I'm wondering > whether generic pmdp_collapse_flush() would be good enough, since the only > addition comparing to the s390 one will be flush_tlb_range() (which is a > further __tlb_flush_mm_lazy). David may have some thoughts. > > The patch itself looks good to me, one trivial nit below. > > > > > > > > > Thanks, > > > > > > > > > > > Also update the stale comment about serializing against fast GUP in > > > > khugepaged. > > > > > > > > Fixes: 2667f50e8b81 ("mm: introduce a general RCU get_user_pages_fast()") > > > > Signed-off-by: Yang Shi > > > > --- > > > > mm/gup.c | 30 ++++++++++++++++++++++++------ > > > > mm/khugepaged.c | 10 ++++++---- > > > > 2 files changed, 30 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index f3fc1f08d90c..4365b2811269 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -2380,8 +2380,9 @@ static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start, > > > > } > > > > > > > > #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL > > > > -static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > > > - unsigned int flags, struct page **pages, int *nr) > > > > +static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > > > > + unsigned long end, unsigned int flags, > > > > + struct page **pages, int *nr) > > > > { > > > > struct dev_pagemap *pgmap = NULL; > > > > int nr_start = *nr, ret = 0; > > > > @@ -2423,7 +2424,23 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > > > goto pte_unmap; > > > > } > > > > > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > > > + /* > > > > + * THP collapse conceptually does: > > > > + * 1. Clear and flush PMD > > > > + * 2. Check the base page refcount > > > > + * 3. Copy data to huge page > > > > + * 4. Clear PTE > > > > + * 5. Discard the base page > > > > + * > > > > + * So fast GUP may race with THP collapse then pin and > > > > + * return an old page since TLB flush is no longer sufficient > > > > + * to serialize against fast GUP. > > > > + * > > > > + * Check PMD, if it is changed just back off since it > > > > + * means there may be parallel THP collapse. > > Would you mind rewording this comment a bit? I feel it a bit weird to > suddenly mention about thp collapse especially its details. > > Maybe some statement on the whole history of why check pte, and in what > case pmd check is needed (where the thp collapse example can be moved to, > imho)? > > One of my attempt for reference.. > > /* > * Fast-gup relies on pte change detection to avoid > * concurrent pgtable operations. > * > * To pin the page, fast-gup needs to do below in order: > * (1) pin the page (by prefetching pte), then (2) check > * pte not changed. > * > * For the rest of pgtable operations where pgtable updates > * can be racy with fast-gup, we need to do (1) clear pte, > * then (2) check whether page is pinned. > * > * Above will work for all pte-level operations, including > * thp split. > * > * For thp collapse, it's a bit more complicated because > * with RCU pgtable free fast-gup can be walking a pgtable > * page that is being freed (so pte is still valid but pmd > * can be cleared already). To avoid race in such > * condition, we need to also check pmd here to make sure > * pmd doesn't change (corresponds to pmdp_collapse_flush() > * in the thp collide code path). > */ > > If you agree with the comment change, feel free to add: Thanks a lot, it looks good to me, will update the patch to incorporate it. > > Acked-by: Peter Xu > > Thanks, > > -- > Peter Xu >