Re: [Intel-gfx] [PATCH 04/23] drm/i915/gem: Only revoke mmap handlers if active

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Chris Wilson (2020-07-02 13:47:00)
> Quoting Tvrtko Ursulin (2020-07-02 13:35:41)
> > 
> > On 02/07/2020 09:32, Chris Wilson wrote:
> > > Avoid waking up the device and taking stale locks if we know that the
> > > object is not currently mmapped. This is particularly useful as not many
> > > object are actually mmapped and so we can destroy them without waking
> > > the device up, and gives us a little more freedom of workqueue ordering
> > > during shutdown.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > index fe27c5b344e3..522ca4f51b53 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > @@ -516,8 +516,11 @@ void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
> > >    */
> > >   void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> > >   {
> > > -     i915_gem_object_release_mmap_gtt(obj);
> > > -     i915_gem_object_release_mmap_offset(obj);
> > > +     if (obj->userfault_count)
> > > +             i915_gem_object_release_mmap_gtt(obj);
> > > +
> > > +     if (!RB_EMPTY_ROOT(&obj->mmo.offsets))
> > > +             i915_gem_object_release_mmap_offset(obj);
> > >   }
> > >   
> > >   static struct i915_mmap_offset *
> > > 
> > 
> > Both conditions will need explaining why they are not racy.
> 
> It's an identical race even if you do take the mutex.
> 
> Thread A                Thread B
> release_mmap            create_mmap_offset
>   mutex_lock/unlock     ...
>                         mutex_lock/unlock
> 
> Thread A will only operate on a snapshot of the current state with or
> without the mutex; if Thread B is concurrently adding new mmaps, that
> may occur before after Thread A makes decision the object is clean.
> Thread A can only assess the state at that moment in time, and only
> cares enough to ensure that from its pov, it has cleared the old
> mmaps.
> 
> During free, we know there can be no concurrency (refcnt==0) and so the
> snapshot is true.

Beyond the free usecase, the serialisation of the individual releases is
coordinated by owning the backing storage operation i.e. we release
when revoking the vma under the vma->vm->mutex, and the pages under
currently the obj->mm.lock; to create a new fault mapping, the handlers
will have taken a reference to either the vma or backing store and thus
have serialised with the release. i915_gem_object_release_mmap() should
be only used on the free path, since it's usual for us to have to do
both. Now what are we doing in set-tiling? The tiling only affects ggtt
mmapings...
-Chris
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx