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

[Jason Gunthorpe <jgg@mellanox.com>]

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

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?

Jason