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
