| Message ID | 1435683333-17844-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/30/2015 05:55 PM, Chris Wilson wrote: > Whilst discussing possible ways to trigger an invalidate_range on a > userptr with an aliased GGTT mmapping (and so cause a struct_mutex > deadlock), the conclusion is that we can, and we must, prevent any > possible deadlock by avoiding taking the mutex at all during > invalidate_range. This has numerous advantages all of which stem from > avoid the sleeping function from inside the unknown context. In > particular, it simplifies the invalidate_range because we no longer > have to juggle the spinlock/mutex and can just hold the spinlock > for the entire walk. To compensate, we have to make get_pages a bit more > complicated in order to serialise with a pending cancel_userptr worker. > As we hold the struct_mutex, we have no choice but to return EAGAIN and > hope that the worker is then flushed before we retry after reacquiring > the struct_mutex. > > The important caveat is that the invalidate_range itself is no longer > synchronous. There exists a small but definite period in time in which > the old PTE remain accessible via the GPU. Note however that the > physical pages themselves are not invalidated by the mmu_notifier, just > the CPU view of the address space. The impact should be limited to a > delay in pages being flushed, rather than a possibility of writing to > the wrong pages. I915_USERPTR_SOMEWHAT_SYNCHRONIZED :/ Because "small but definite" might be not that small in some circumstances. Although.. it is not like current synchronous behavior really is so since a) GPU activity doesn't stop right away on cancellation and b) it is a "don't do that" situation anyway. So from that angle maybe it is not that bad, and advantages from your first commit paragraph are definitely real. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Ok ok :) > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 142 +++++++++++++------------------- > 1 file changed, 56 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index d3213fdefafc..9056aa5b00f3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -50,7 +50,6 @@ struct i915_mmu_notifier { > struct mmu_notifier mn; > struct rb_root objects; > struct list_head linear; > - unsigned long serial; > bool has_linear; > }; > > @@ -59,14 +58,16 @@ struct i915_mmu_object { > struct interval_tree_node it; > struct list_head link; > struct drm_i915_gem_object *obj; > + struct work_struct work; > bool active; > bool is_linear; > }; > > -static unsigned long cancel_userptr(struct drm_i915_gem_object *obj) > +static void __cancel_userptr__worker(struct work_struct *work) > { > + struct i915_mmu_object *mo = container_of(work, typeof(*mo), work); > + struct drm_i915_gem_object *obj = mo->obj; > struct drm_device *dev = obj->base.dev; > - unsigned long end; > > mutex_lock(&dev->struct_mutex); > /* Cancel any active worker and force us to re-evaluate gup */ > @@ -89,46 +90,26 @@ static unsigned long cancel_userptr(struct drm_i915_gem_object *obj) > dev_priv->mm.interruptible = was_interruptible; > } > > - end = obj->userptr.ptr + obj->base.size; > - > drm_gem_object_unreference(&obj->base); > mutex_unlock(&dev->struct_mutex); > - > - return end; > } > > -static void *invalidate_range__linear(struct i915_mmu_notifier *mn, > - struct mm_struct *mm, > - unsigned long start, > - unsigned long end) > +static unsigned long cancel_userptr(struct i915_mmu_object *mo) > { > - struct i915_mmu_object *mo; > - unsigned long serial; > - > -restart: > - serial = mn->serial; > - list_for_each_entry(mo, &mn->linear, link) { > - struct drm_i915_gem_object *obj; > - > - if (mo->it.last < start || mo->it.start > end) > - continue; > - > - obj = mo->obj; > - > - if (!mo->active || > - !kref_get_unless_zero(&obj->base.refcount)) > - continue; > - > - spin_unlock(&mn->lock); > - > - cancel_userptr(obj); > - > - spin_lock(&mn->lock); > - if (serial != mn->serial) > - goto restart; > - } > + unsigned long end = mo->obj->userptr.ptr + mo->obj->base.size; > + > + /* The mmu_object is released late when destroying the > + * GEM object so it is entirely possible to gain a > + * reference on an object in the process of being freed > + * since our serialisation is via the spinlock and not > + * the struct_mutex - and consequently use it after it > + * is freed and then double free it. > + */ > + if (mo->active && > + kref_get_unless_zero(&mo->obj->base.refcount)) > + schedule_work(&mo->work); > > - return NULL; > + return end; > } > > static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, > @@ -136,45 +117,32 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, > unsigned long start, > unsigned long end) > { > - struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn); > - struct interval_tree_node *it = NULL; > - unsigned long next = start; > - unsigned long serial = 0; > - > - end--; /* interval ranges are inclusive, but invalidate range is exclusive */ > - while (next < end) { > - struct drm_i915_gem_object *obj = NULL; > - > - spin_lock(&mn->lock); > - if (mn->has_linear) > - it = invalidate_range__linear(mn, mm, start, end); > - else if (serial == mn->serial) > - it = interval_tree_iter_next(it, next, end); > - else > - it = interval_tree_iter_first(&mn->objects, start, end); > - if (it != NULL) { > - struct i915_mmu_object *mo = > - container_of(it, struct i915_mmu_object, it); > - > - /* The mmu_object is released late when destroying the > - * GEM object so it is entirely possible to gain a > - * reference on an object in the process of being freed > - * since our serialisation is via the spinlock and not > - * the struct_mutex - and consequently use it after it > - * is freed and then double free it. > - */ > - if (mo->active && > - kref_get_unless_zero(&mo->obj->base.refcount)) > - obj = mo->obj; > + struct i915_mmu_notifier *mn = > + container_of(_mn, struct i915_mmu_notifier, mn); > + struct i915_mmu_object *mo; > + > + /* interval ranges are inclusive, but invalidate range is exclusive */ > + end--; > > - serial = mn->serial; > + spin_lock(&mn->lock); > + if (mn->has_linear) { > + list_for_each_entry(mo, &mn->linear, link) { > + if (mo->it.last < start || mo->it.start > end) > + continue; > + > + cancel_userptr(mo); > } > - spin_unlock(&mn->lock); > - if (obj == NULL) > - return; > + } else { > + struct interval_tree_node *it; > > - next = cancel_userptr(obj); > + it = interval_tree_iter_first(&mn->objects, start, end); > + while (it) { > + mo = container_of(it, struct i915_mmu_object, it); > + start = cancel_userptr(mo); > + it = interval_tree_iter_next(it, start, end); > + } > } > + spin_unlock(&mn->lock); > } > > static const struct mmu_notifier_ops i915_gem_userptr_notifier = { > @@ -194,7 +162,6 @@ i915_mmu_notifier_create(struct mm_struct *mm) > spin_lock_init(&mn->lock); > mn->mn.ops = &i915_gem_userptr_notifier; > mn->objects = RB_ROOT; > - mn->serial = 1; > INIT_LIST_HEAD(&mn->linear); > mn->has_linear = false; > > @@ -208,12 +175,6 @@ i915_mmu_notifier_create(struct mm_struct *mm) > return mn; > } > > -static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mn) > -{ > - if (++mn->serial == 0) > - mn->serial = 1; > -} > - > static int > i915_mmu_notifier_add(struct drm_device *dev, > struct i915_mmu_notifier *mn, > @@ -260,10 +221,9 @@ i915_mmu_notifier_add(struct drm_device *dev, > } else > interval_tree_insert(&mo->it, &mn->objects); > > - if (ret == 0) { > + if (ret == 0) > list_add(&mo->link, &mn->linear); > - __i915_mmu_notifier_update_serial(mn); > - } > + > spin_unlock(&mn->lock); > mutex_unlock(&dev->struct_mutex); > > @@ -291,7 +251,6 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mn, > mn->has_linear = i915_mmu_notifier_has_linear(mn); > else > interval_tree_remove(&mo->it, &mn->objects); > - __i915_mmu_notifier_update_serial(mn); > spin_unlock(&mn->lock); > } > > @@ -358,6 +317,7 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, > mo->it.start = obj->userptr.ptr; > mo->it.last = mo->it.start + obj->base.size - 1; > mo->obj = obj; > + INIT_WORK(&mo->work, __cancel_userptr__worker); > > ret = i915_mmu_notifier_add(obj->base.dev, mn, mo); > if (ret) { > @@ -546,10 +506,12 @@ err: > return ret; > } > > -static void > +static int > __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, > bool value) > { > + int ret = 0; > + > /* During mm_invalidate_range we need to cancel any userptr that > * overlaps the range being invalidated. Doing so requires the > * struct_mutex, and that risks recursion. In order to cause > @@ -562,12 +524,20 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, > */ > #if defined(CONFIG_MMU_NOTIFIER) > if (obj->userptr.mmu_object == NULL) > - return; > + return 0; > > spin_lock(&obj->userptr.mmu_object->mn->lock); > - obj->userptr.mmu_object->active = value; > + /* In order to serialise get_pages with an outstanding > + * cancel_userptr, we must drop the struct_mutex and try again. > + */ > + if (!value || !work_pending(&obj->userptr.mmu_object->work)) > + obj->userptr.mmu_object->active = value; > + else > + ret = -EAGAIN; > spin_unlock(&obj->userptr.mmu_object->mn->lock); > #endif > + > + return ret; > } I think it would be a lot more efficient if we dropped the mutex here and waited for the worker to complete in kernel, rather than letting userspace hammer it (the mutex). Especially having experienced one worker-structmutex starvation yesterday. Regards, Tvrtko
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6689
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -1 287/287 286/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_userptr_blits@sync-unmap-cycles PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
On Wed, Jul 01, 2015 at 01:56:30PM +0100, Tvrtko Ursulin wrote: > >@@ -562,12 +524,20 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, > > */ > > #if defined(CONFIG_MMU_NOTIFIER) > > if (obj->userptr.mmu_object == NULL) > >- return; > >+ return 0; > > > > spin_lock(&obj->userptr.mmu_object->mn->lock); > >- obj->userptr.mmu_object->active = value; > >+ /* In order to serialise get_pages with an outstanding > >+ * cancel_userptr, we must drop the struct_mutex and try again. > >+ */ > >+ if (!value || !work_pending(&obj->userptr.mmu_object->work)) > >+ obj->userptr.mmu_object->active = value; > >+ else > >+ ret = -EAGAIN; > > spin_unlock(&obj->userptr.mmu_object->mn->lock); > > #endif > >+ > >+ return ret; > > } > > I think it would be a lot more efficient if we dropped the mutex > here and waited for the worker to complete in kernel, rather than > letting userspace hammer it (the mutex). Especially having > experienced one worker-structmutex starvation yesterday. Yes, it would be much easier and simpler. It also goes bang very quickly - as we cannot drop the mutex here in the middle of an execbuffer as that quickly corrupts the state believed reserved by the execbuffer. If you look at other patches in my tree, we can hide the starvation issue in the kernel - but we still have no way to solve the priority inversion problem of using a worker for a high priority task, except to move them to the high priority queue. Worth it? Not sure. But flushing a single queue is easier than doing semaphore completion. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index d3213fdefafc..9056aa5b00f3 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -50,7 +50,6 @@ struct i915_mmu_notifier { struct mmu_notifier mn; struct rb_root objects; struct list_head linear; - unsigned long serial; bool has_linear; }; @@ -59,14 +58,16 @@ struct i915_mmu_object { struct interval_tree_node it; struct list_head link; struct drm_i915_gem_object *obj; + struct work_struct work; bool active; bool is_linear; }; -static unsigned long cancel_userptr(struct drm_i915_gem_object *obj) +static void __cancel_userptr__worker(struct work_struct *work) { + struct i915_mmu_object *mo = container_of(work, typeof(*mo), work); + struct drm_i915_gem_object *obj = mo->obj; struct drm_device *dev = obj->base.dev; - unsigned long end; mutex_lock(&dev->struct_mutex); /* Cancel any active worker and force us to re-evaluate gup */ @@ -89,46 +90,26 @@ static unsigned long cancel_userptr(struct drm_i915_gem_object *obj) dev_priv->mm.interruptible = was_interruptible; } - end = obj->userptr.ptr + obj->base.size; - drm_gem_object_unreference(&obj->base); mutex_unlock(&dev->struct_mutex); - - return end; } -static void *invalidate_range__linear(struct i915_mmu_notifier *mn, - struct mm_struct *mm, - unsigned long start, - unsigned long end) +static unsigned long cancel_userptr(struct i915_mmu_object *mo) { - struct i915_mmu_object *mo; - unsigned long serial; - -restart: - serial = mn->serial; - list_for_each_entry(mo, &mn->linear, link) { - struct drm_i915_gem_object *obj; - - if (mo->it.last < start || mo->it.start > end) - continue; - - obj = mo->obj; - - if (!mo->active || - !kref_get_unless_zero(&obj->base.refcount)) - continue; - - spin_unlock(&mn->lock); - - cancel_userptr(obj); - - spin_lock(&mn->lock); - if (serial != mn->serial) - goto restart; - } + unsigned long end = mo->obj->userptr.ptr + mo->obj->base.size; + + /* The mmu_object is released late when destroying the + * GEM object so it is entirely possible to gain a + * reference on an object in the process of being freed + * since our serialisation is via the spinlock and not + * the struct_mutex - and consequently use it after it + * is freed and then double free it. + */ + if (mo->active && + kref_get_unless_zero(&mo->obj->base.refcount)) + schedule_work(&mo->work); - return NULL; + return end; } static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, @@ -136,45 +117,32 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, unsigned long start, unsigned long end) { - struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn); - struct interval_tree_node *it = NULL; - unsigned long next = start; - unsigned long serial = 0; - - end--; /* interval ranges are inclusive, but invalidate range is exclusive */ - while (next < end) { - struct drm_i915_gem_object *obj = NULL; - - spin_lock(&mn->lock); - if (mn->has_linear) - it = invalidate_range__linear(mn, mm, start, end); - else if (serial == mn->serial) - it = interval_tree_iter_next(it, next, end); - else - it = interval_tree_iter_first(&mn->objects, start, end); - if (it != NULL) { - struct i915_mmu_object *mo = - container_of(it, struct i915_mmu_object, it); - - /* The mmu_object is released late when destroying the - * GEM object so it is entirely possible to gain a - * reference on an object in the process of being freed - * since our serialisation is via the spinlock and not - * the struct_mutex - and consequently use it after it - * is freed and then double free it. - */ - if (mo->active && - kref_get_unless_zero(&mo->obj->base.refcount)) - obj = mo->obj; + struct i915_mmu_notifier *mn = + container_of(_mn, struct i915_mmu_notifier, mn); + struct i915_mmu_object *mo; + + /* interval ranges are inclusive, but invalidate range is exclusive */ + end--; - serial = mn->serial; + spin_lock(&mn->lock); + if (mn->has_linear) { + list_for_each_entry(mo, &mn->linear, link) { + if (mo->it.last < start || mo->it.start > end) + continue; + + cancel_userptr(mo); } - spin_unlock(&mn->lock); - if (obj == NULL) - return; + } else { + struct interval_tree_node *it; - next = cancel_userptr(obj); + it = interval_tree_iter_first(&mn->objects, start, end); + while (it) { + mo = container_of(it, struct i915_mmu_object, it); + start = cancel_userptr(mo); + it = interval_tree_iter_next(it, start, end); + } } + spin_unlock(&mn->lock); } static const struct mmu_notifier_ops i915_gem_userptr_notifier = { @@ -194,7 +162,6 @@ i915_mmu_notifier_create(struct mm_struct *mm) spin_lock_init(&mn->lock); mn->mn.ops = &i915_gem_userptr_notifier; mn->objects = RB_ROOT; - mn->serial = 1; INIT_LIST_HEAD(&mn->linear); mn->has_linear = false; @@ -208,12 +175,6 @@ i915_mmu_notifier_create(struct mm_struct *mm) return mn; } -static void __i915_mmu_notifier_update_serial(struct i915_mmu_notifier *mn) -{ - if (++mn->serial == 0) - mn->serial = 1; -} - static int i915_mmu_notifier_add(struct drm_device *dev, struct i915_mmu_notifier *mn, @@ -260,10 +221,9 @@ i915_mmu_notifier_add(struct drm_device *dev, } else interval_tree_insert(&mo->it, &mn->objects); - if (ret == 0) { + if (ret == 0) list_add(&mo->link, &mn->linear); - __i915_mmu_notifier_update_serial(mn); - } + spin_unlock(&mn->lock); mutex_unlock(&dev->struct_mutex); @@ -291,7 +251,6 @@ i915_mmu_notifier_del(struct i915_mmu_notifier *mn, mn->has_linear = i915_mmu_notifier_has_linear(mn); else interval_tree_remove(&mo->it, &mn->objects); - __i915_mmu_notifier_update_serial(mn); spin_unlock(&mn->lock); } @@ -358,6 +317,7 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, mo->it.start = obj->userptr.ptr; mo->it.last = mo->it.start + obj->base.size - 1; mo->obj = obj; + INIT_WORK(&mo->work, __cancel_userptr__worker); ret = i915_mmu_notifier_add(obj->base.dev, mn, mo); if (ret) { @@ -546,10 +506,12 @@ err: return ret; } -static void +static int __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value) { + int ret = 0; + /* During mm_invalidate_range we need to cancel any userptr that * overlaps the range being invalidated. Doing so requires the * struct_mutex, and that risks recursion. In order to cause @@ -562,12 +524,20 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, */ #if defined(CONFIG_MMU_NOTIFIER) if (obj->userptr.mmu_object == NULL) - return; + return 0; spin_lock(&obj->userptr.mmu_object->mn->lock); - obj->userptr.mmu_object->active = value; + /* In order to serialise get_pages with an outstanding + * cancel_userptr, we must drop the struct_mutex and try again. + */ + if (!value || !work_pending(&obj->userptr.mmu_object->work)) + obj->userptr.mmu_object->active = value; + else + ret = -EAGAIN; spin_unlock(&obj->userptr.mmu_object->mn->lock); #endif + + return ret; } static void