Re: [PATCH v10 07/10] mm: Device exclusive memory access

[Alistair Popple]

On Saturday, 12 June 2021 1:01:42 AM AEST Peter Xu wrote:
> On Fri, Jun 11, 2021 at 01:43:20PM +1000, Alistair Popple wrote:
> > On Friday, 11 June 2021 11:00:34 AM AEST Peter Xu wrote:
> > > On Fri, Jun 11, 2021 at 09:17:14AM +1000, Alistair Popple wrote:
> > > > On Friday, 11 June 2021 9:04:19 AM AEST Peter Xu wrote:
> > > > > On Fri, Jun 11, 2021 at 12:21:26AM +1000, Alistair Popple wrote:
> > > > > > > Hmm, the thing is.. to me FOLL_SPLIT_PMD should have similar effect to explicit
> > > > > > > call split_huge_pmd_address(), afaict.  Since both of them use __split_huge_pmd()
> > > > > > > internally which will generate that unwanted CLEAR notify.
> > > > > >
> > > > > > Agree that gup calls __split_huge_pmd() via split_huge_pmd_address()
> > > > > > which will always CLEAR. However gup only calls split_huge_pmd_address() if it
> > > > > > finds a thp pmd. In follow_pmd_mask() we have:
> > > > > >
> > > > > >       if (likely(!pmd_trans_huge(pmdval)))
> > > > > >               return follow_page_pte(vma, address, pmd, flags, &ctx->pgmap);
> > > > > >
> > > > > > So I don't think we have a problem here.
> > > > >
> > > > > Sorry I didn't follow here..  We do FOLL_SPLIT_PMD after this check, right?  I
> > > > > mean, if it's a thp for the current mm, afaict pmd_trans_huge() should return
> > > > > true above, so we'll skip follow_page_pte(); then we'll check FOLL_SPLIT_PMD
> > > > > and do the split, then the CLEAR notify.  Hmm.. Did I miss something?
> > > >
> > > > That seems correct - if the thp is not mapped with a pmd we won't split and we
> > > > won't CLEAR. If there is a thp pmd we will split and CLEAR, but in that case it
> > > > is fine - we will retry, but the retry will won't CLEAR because the pmd has
> > > > already been split.
> > >
> > > Aha!
> > >
> > > >
> > > > The issue arises with doing it unconditionally in make device exclusive is that
> > > > you *always* CLEAR even if there is no thp pmd to split. Or at least that's my
> > > > understanding, please let me know if it doesn't make sense.
> > >
> > > Exactly.  But if you see what I meant here, even if it can work like this, it
> > > sounds still fragile, isn't it?  I just feel something is slightly off there..
> > >
> > > IMHO split_huge_pmd() checked pmd before calling __split_huge_pmd() for
> > > performance, afaict, because if it's not a thp even without locking, then it
> > > won't be, so further __split_huge_pmd() is not necessary.
> > >
> > > IOW, it's very legal if someday we'd like to let split_huge_pmd() call
> > > __split_huge_pmd() directly, then AFAIU device exclusive API will be the 1st
> > > one to be broken with that seems-to-be-irrelevant change I'm afraid..
> >
> > Well I would argue the performance of memory notifiers is becoming increasingly
> > important, and a change that causes them to be called unnecessarily is
> > therefore not very legal. Likely the correct fix here is to optimise
> > __split_huge_pmd() to only call the notifier if it's actually going to split a
> > pmd. As you said though that's a completely different story which I think would
> > be best done as a separate series.
> 
> Right, maybe I can look a bit more into that later; but my whole point was to
> express that one functionality shouldn't depend on such a trivial detail of
> implementation of other modules (thp split in this case).
> 
> >
> > > This lets me goes back a step to think about why do we need this notifier at
> > > all to cover this whole range of make_device_exclusive() procedure..
> > >
> > > What I am thinking is, we're afraid some CPU accesses this page so the pte got
> > > quickly restored when device atomic operation is carrying on.  Then with this
> > > notifier we'll be able to cancel it.  Makes perfect sense.
> > >
> > > However do we really need to register this notifier so early?  The thing is the
> > > GPU driver still has all the page locks, so even if there's a race to restore
> > > the ptes, they'll block at taking the page lock until the driver releases it.
> > >
> > > IOW, I'm wondering whether the "non-fragile" way to do this is not do
> > > mmu_interval_notifier_insert() that early: what if we register that notifier
> > > after make_device_exclusive_range() returns but before page_unlock() somehow?
> > > So before page_unlock(), race is protected fully by the lock itself; after
> > > that, it's done by mmu notifier.  Then maybe we don't need to worry about all
> > > these notifications during marking exclusive (while we shouldn't)?
> >
> > The notifier is needed to protect against races with pte changes. Once a page
> > has been marked for exclusive access the driver will update it's page tables to
> > allow atomic access to the page. However in the meantime the page could become
> > unmapped entirely or write protected.
> >
> > As I understand things the page lock won't protect against these kind of pte
> > changes, hence the need for mmu_interval_read_begin/retry which allows the
> > driver to hold a mutex protecting against invalidations via blocking the
> > notifier until the device page tables have been updated.
> 
> Indeed, I suppose you mean change_pte_range() and zap_pte_range()
> correspondingly.

Right.

> Do you think we can restore pte right before wr-protect or zap?  Then all
> things serializes with page lock (btw: it's already an insane userspace to
> either unmap a page or wr-protect a page if it knows the device is using it!).
> If these are the only two cases, it still sounds a cleaner approach to me than
> the current approach.

Perhaps we could but it would make {zap|change}_pte_range() much more complex as
we can't sleep taking the page lock whilst holding the ptl, so we'd have to
implement a retry scheme similar to copy_pte_range() in both those functions as
well. Given mmu_interval_read_begin/retry was IMHO added to solve this type of
problem (freezing pte's to safely program device pte's) it seems like the
better option rather than adding more complex code to generic mm paths.

It's also worth noting i915 seems to use mmu_interval_read_begin/retry() with
gup to sync mappings so this isn't an entirely new concept. I'm not an expert
in that driver but I imagine changing gup to generate unconditional mmu notifier
invalidates would also cause issues there. So I think overall this is the
cleanest solution as it reduces the amount of code (particularly in generic mm
paths).

> This also reminded me that right now the cpu pgtable recovery is lazy - it
> happens either from fork() or a cpu page fault.  Even after device finished
> using it, swap ptes keep there.
> 
> What if the device tries to do atomic op on the same page twice?  I am not sure
> whether it means we may also want to teach both GUP (majorly follow_page_pte()
> for now before pmd support) and process of page_make_device_exclusive() with
> understanding the device exclusive entries too?  Another option seems to be
> restoring pte after device finish using it, as long as the device knows when.

I don't think we need to complicate follow_page_pte() with knowledge of
exclusive entries. GUP will just restore the original pte via the normal
fault path - follow_page_pte() will return NULL for an exclusive entry,
resulting in handle_mm_path() getting called via faultin_page(). Therefore
a driver calling make_device_exclusive() twice on the same page won't cause an
issue. Also the device shouldn't fault on subsequent accesses if the exclusive
entry is still in place anyway.

We can't restore the pte when the device is finished with it because there is
no way of knowing when a device is done using an exclusive entry - device
pte's work much the same as cpu pte's in that regard.

 - Alistair

> --
> Peter Xu
>