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:   Tue, 6 Jun 2023 15:59:18 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Yang Shi <shy828301@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        David Hildenbrand <david@redhat.com>,
        Alistair Popple <apopple@nvidia.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Johannes Weiner <hannes@cmpxchg.org>,
        John Hubbard <jhubbard@nvidia.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Muhammad Usama Anjum <usama.anjum@collabora.com>,
        Hugh Dickins <hughd@google.com>,
        Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH 4/4] mm: Make most walk page paths with
 pmd_trans_unstable() to retry
Message-ID: <ZH+QFvOuE0Egxsqi@x1n>
References: <20230602230552.350731-1-peterx@redhat.com>
 <20230602230552.350731-5-peterx@redhat.com>
 <CAHbLzkp_tzN8SZVeWTKxtMAnFSzUvk2064KFg3quj=raOSHPrA@mail.gmail.com>
 <ZH41YzZ0DBoF8csH@x1n>
 <CAHbLzkq-dE4B5k+4KV5YtSJRXf+x61V8iBte6Z=Afbh=_oCJtw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAHbLzkq-dE4B5k+4KV5YtSJRXf+x61V8iBte6Z=Afbh=_oCJtw@mail.gmail.com>
Precedence: bulk

On Tue, Jun 06, 2023 at 12:12:03PM -0700, Yang Shi wrote:
> > > > @@ -1539,8 +1544,10 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,
> > > >                 return err;
> > > >         }
> > > >
> > > > -       if (pmd_trans_unstable(pmdp))
> > > > +       if (pmd_trans_unstable(pmdp)) {
> > > > +               walk->action = ACTION_AGAIN;
> > > >                 return 0;
> > >
> > > Had a quick look at the pagemap code, I agree with your analysis,
> > > "returning 0" may mess up pagemap, retry should be fine. But I'm
> > > wondering whether we should just fill in empty entries. Anyway I don't
> > > have a  strong opinion on this, just a little bit concerned by
> > > potential indefinite retry.
> >
> > Yes, none pte is still an option.  But if we're going to fix this anyway,
> > it seems better to fix it with the accurate new thing that poped up, and
> > it's even less change (just apply walk->action rather than doing random
> > stuff in different call sites).
> >
> > I see that you have worry on deadloop over this, so I hope to discuss
> > altogether here.
> >
> > Unlike normal checks, pmd_trans_unstable() check means something must have
> > changed in the past very short period or it should just never if nothing
> > changed concurrently from under us, so it's not a "if (flag==true)" check
> > which is even more likely to loop.
> >
> > If we see the places that I didn't touch, most of them suggested a retry in
> > one form or another.  So if there's a worry this will also not the first
> > time to do a retry (and for such a "unstable" API, that's really the most
> > natural thing to do which is to retry until it's stable).
> 
> IIUC other than do_anonymous_page() suggests retry (retry page fault),
> others may not, for example:
>   - follow_pmd_mask: return -EBUSY

I assumed a -EBUSY would mean if the caller still needs the page it'll just
need to retry.

It's actually a very rare errno to return in follow page... e.g. some gup
callers may not even be able to handle -EBUSY afaiu, neither does the gup
core (__get_user_pages), afaiu it'll just forwarded upwards.

My bet is it's just so rare and only used with FOLL_SPLIT_PMD for now.  I
had a quick look, at least kvm handles -EBUSY as a real fault (hva_to_pfn,
where it should translate that -EBUSY into a KVM_PFN_ERR_FAULT), I think
it'll crash the hypervisor directly if returned from gup one day (not for
now if never with !FOLL_SPLIT_PMD)..

>   - wp_clean_pmd_entry: actually just retry for pmd_none case, but the
> pagewalk code does handle pmd_none by skipping it, so it basically
> just retry once

This one is very special IMHO, pmd_trans_unstable() should in most cases be
used after someone checked pmd value before walking ptes.  I had a feeling
it's kind of abused, to check whether it's a huge pmd in whatever format..
IMHO it should just use the other pmd_*() helpers but I won't touch it in
this series.

>   - min_core_pte_range: treated as unmapped range by calling
> __mincore_unmapped_range

Correct.

Also pmd_devmap_trans_unstable(), called in 3 call sites:

pmd_devmap_trans_unstable[1418] return pmd_devmap(*pmd) || pmd_trans_unstable(pmd);
    filemap_map_pmd[3423]          if (pmd_devmap_trans_unstable(vmf->pmd)) {
    finish_fault[4390]             if (pmd_devmap_trans_unstable(vmf->pmd))
    handle_pte_fault[4922]         if (pmd_devmap_trans_unstable(vmf->pmd))

They all suggest a retry on the page faults, afaiu.

So indeed not all of them retries, but I doubt those ones that are not and
whether that's the best we should have.  Again, I'll leave that out of this
series.

> 
> Anyway I really don't have a strong opinion on this. I may be just
> over-concerned. I just thought if nobody cares whether the result is
> accurate or not, why do we bother fixing those cases?

Because anyway we're already at it and it's easier and cleaner to do so? :)

I would say if to fix this I think the most important ones are pagemap and
memcg paths so far, but since I'm at it and anyway I checked all of them, I
figured maybe I should just make everywhere do right and in the same
pattern when handling unstable pmd. Especially, what this patch touched are
all using walk_page*() routines (I left special ones in first patches), so
it's quite straightforward IMHO to switch altogether using ACTION_AGAIN.

Thanks,

-- 
Peter Xu