| Message ID | 20200702083225.20044-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/23] drm/i915: Drop vm.ref for duplicate vma on construction | expand |
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. First should normally be done under the ggtt->mutex, second under obj->mmo.lock. Regards, Tvrtko
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. -Chris
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
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 *
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(-)