Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Sep 2022 13:41:59 -0400
From:   Peter Xu <peterx@redhat.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Sasha Levin <sasha.levin@oracle.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@linux.vnet.ibm.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Jerome Marchand <jmarchan@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Hugh Dickins <hughd@google.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Yang Shi <shy828301@gmail.com>
Subject: Re: [PATCH v1] mm/gup: adjust stale comment for RCU GUP-fast
Message-ID: <YxDu51TQTz8lhIPP@xz-m1.local>
References: <20220901072119.37588-1-david@redhat.com>
 <YxDdycTur733hMgt@xz-m1.local>
 <fa0bb4b1-3edd-eb5a-7ad6-dff785d88d8f@redhat.com>
 <YxDghv54uHYMGCfG@xz-m1.local>
 <c9dc3f22-4a72-9b9d-7a74-ad77fe4f3b6e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c9dc3f22-4a72-9b9d-7a74-ad77fe4f3b6e@redhat.com>
Precedence: bulk

On Thu, Sep 01, 2022 at 06:46:13PM +0200, David Hildenbrand wrote:
> On 01.09.22 18:40, Peter Xu wrote:
> > On Thu, Sep 01, 2022 at 06:34:41PM +0200, David Hildenbrand wrote:
> >> On 01.09.22 18:28, Peter Xu wrote:
> >>> On Thu, Sep 01, 2022 at 09:21:19AM +0200, David Hildenbrand wrote:
> >>>> commit 4b471e8898c3 ("mm, thp: remove infrastructure for handling splitting
> >>>> PMDs") didn't remove all details about the THP split requirements for
> >>>> RCU GUP-fast.
> >>>>
> >>>> IPI broeadcasts on THP split are no longer required.
> >>>>
> >>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >>>> Cc: Sasha Levin <sasha.levin@oracle.com>
> >>>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> >>>> Cc: Vlastimil Babka <vbabka@suse.cz>
> >>>> Cc: Jerome Marchand <jmarchan@redhat.com>
> >>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
> >>>> Cc: Hugh Dickins <hughd@google.com>
> >>>> Cc: Jason Gunthorpe <jgg@nvidia.com>
> >>>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>>> Cc: Peter Xu <peterx@redhat.com>
> >>>> Cc: Yang Shi <shy828301@gmail.com>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>  mm/gup.c | 5 ++---
> >>>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/mm/gup.c b/mm/gup.c
> >>>> index 5abdaf487460..cfe71f422787 100644
> >>>> --- a/mm/gup.c
> >>>> +++ b/mm/gup.c
> >>>> @@ -2309,9 +2309,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
> >>>>   *
> >>>>   * Another way to achieve this is to batch up page table containing pages
> >>>>   * belonging to more than one mm_user, then rcu_sched a callback to free those
> >>>> - * pages. Disabling interrupts will allow the fast_gup walker to both block
> >>>> - * the rcu_sched callback, and an IPI that we broadcast for splitting THPs
> >>>> - * (which is a relatively rare event). The code below adopts this strategy.
> >>>> + * pages. Disabling interrupts will allow the fast_gup walker to block the
> >>>> + * rcu_sched callback.
> >>>
> >>> This is the comment for fast-gup in general but not only for thp split.
> >>
> >> "an IPI that we broadcast for splitting THP" is about splitting THP.
> > 
> > Ah OK.  Shall we still keep some "IPI broadcast" information here if we're
> > modifying it?  Otherwise it gives a feeling that none needs the IPIs.
> 
> I guess that's the end goal -- and we forgot about the PMD collapse case.
> 
> Are we aware of any other case that needs an IPI? I'd rather avoid
> documenting something that's no longer true.

I'm not aware of any.

> 
> > 
> > It can be dropped later if you want to rework the thp collapse side and
> > finally remove IPI dependency on fast-gup, but so far it seems to me it's
> > still needed.  Or just drop this patch until that rework happens?
> 
> The doc as is is obviously stale, why drop this patch?
> 
> We should see a fix for the THP collapse issue very soon I guess. Most
> probably this patch will go upstream after that fix.

No objection to have this patch alone as the removal statement is only
about "thp split".  But IMHO this patch alone didn't really help a great
lot, especially if you plan to have more to come that is very relevant to
this, so it'll be clearer to put them together.  Your call.

> 
> > 
> >>
> >>>
> >>> I can understand that we don't need IPI for thp split, but isn't the IPIs
> >>> still needed for thp collapse (aka pmdp_collapse_flush)?
> >>
> >> That was, unfortunately, never documented -- and as discussed in the
> >> other thread, arm64 doesn't do that IPI before collapse and might need
> >> fixing. We'll most probably end up getting rid of that
> >> (undocumented/forgotten) IPI requirement and fix it in GUP-fast by
> >> re-rechecking if the PMD changed.
> > 
> > Yeah from an initial thought that looks valid to me.  It'll also allow
> > pmdp_collapse_flush() to be dropped too, am I right?
> 
> I think the magic about pmdp_collapse_flush() is not only the IPIs, but
> that we don't perform an ordinary PMD flush but we logically flush "all
> PTEs in that range".

Yes there's a difference, good to learn that, thanks.

-- 
Peter Xu