Re: [PATCH hmm 00/15] Consolidate the mmu notifier interval_tree and locking

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 22, 2019 at 03:01:13PM +0000, Jason Gunthorpe wrote:
> On Tue, Oct 22, 2019 at 09:57:35AM +0200, Daniel Vetter wrote:
> 
> > > The unusual bit in all of this is using a lock's critical region to
> > > 'protect' data for read, but updating that same data before the lock's
> > > critical secion. ie relying on the unlock barrier to 'release' program
> > > ordered stores done before the lock's own critical region, and the
> > > lock side barrier to 'acquire' those stores.
> > 
> > I think this unusual use of locks as barriers for other unlocked accesses
> > deserves comments even more than just normal barriers. Can you pls add
> > them? I think the design seeems sound ...
> > 
> > Also the comment on the driver's lock hopefully prevents driver
> > maintainers from moving the driver_lock around in a way that would very
> > subtle break the scheme, so I think having the acquire barrier commented
> > in each place would be really good.
> 
> There is already a lot of documentation, I think it would be helpful
> if you could suggest some specific places where you think an addition
> would help? I think the perspective of someone less familiar with this
> design would really improve the documentation

Hm I just meant the usual recommendation that "barriers must have comments
explaining what they order, and where the other side of the barrier is".
Using unlock/lock as a barrier imo just makes that an even better idea.
Usually what I do is something like "we need to order $this against $that
below, and the other side of this barrier is in function()." With maybe a
bit more if it's not obvious how things go wrong if the orderin is broken.

Ofc seqlock.h itself skimps on that rule and doesn't bother explaining its
barriers :-/

> I've been tempted to force the driver to store the seq number directly
> under the driver lock - this makes the scheme much clearer, ie
> something like this:
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 712c99918551bc..738fa670dcfb19 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -488,7 +488,8 @@ struct svm_notifier {
>  };
>  
>  static bool nouveau_svm_range_invalidate(struct mmu_range_notifier *mrn,
> -                                        const struct mmu_notifier_range *range)
> +                                        const struct mmu_notifier_range *range,
> +                                        unsigned long seq)
>  {
>         struct svm_notifier *sn =
>                 container_of(mrn, struct svm_notifier, notifier);
> @@ -504,6 +505,7 @@ static bool nouveau_svm_range_invalidate(struct mmu_range_notifier *mrn,
>                 mutex_lock(&sn->svmm->mutex);
>         else if (!mutex_trylock(&sn->svmm->mutex))
>                 return false;
> +       mmu_range_notifier_update_seq(mrn, seq);
>         mutex_unlock(&sn->svmm->mutex);
>         return true;
>  }
> 
> 
> At the cost of making the driver a bit more complex, what do you
> think?

Hm, spinning this further ... could we initialize the mmu range notifier
with a pointer to the driver lock, so that we could put a
lockdep_assert_held into mmu_range_notifier_update_seq? I think that would
make this scheme substantially more driver-hacker proof :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch