| Message ID | 1491562175-27680-1-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks for the review, pushing series. On pe, 2017-04-07 at 11:25 +0000, Patchwork wrote: > == Series Details == > > Series: series starting with [1/2] drm/i915: Don't call synchronize_rcu_expedited under struct_mutex > URL : https://patchwork.freedesktop.org/series/22652/ > State : success > > == Summary == > > Series 22652v1 Series without cover letter > https://patchwork.freedesktop.org/api/1.0/series/22652/revisions/1/mbox/ > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-uc: > fail -> PASS (fi-snb-2600) fdo#100007 > > fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 > > fi-bdw-5557u total:278 pass:266 dwarn:0 dfail:0 fail:1 skip:11 time:430s > fi-bdw-gvtdvm total:278 pass:255 dwarn:8 dfail:0 fail:1 skip:14 time:424s > fi-bsw-n3050 total:278 pass:241 dwarn:0 dfail:0 fail:1 skip:36 time:577s > fi-bxt-j4205 total:278 pass:258 dwarn:0 dfail:0 fail:1 skip:19 time:506s > fi-byt-j1900 total:278 pass:253 dwarn:0 dfail:0 fail:1 skip:24 time:479s > fi-byt-n2820 total:278 pass:249 dwarn:0 dfail:0 fail:1 skip:28 time:488s > fi-hsw-4770 total:278 pass:261 dwarn:0 dfail:0 fail:1 skip:16 time:412s > fi-hsw-4770r total:278 pass:261 dwarn:0 dfail:0 fail:1 skip:16 time:410s > fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:1 skip:49 time:428s > fi-ivb-3520m total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:475s > fi-ivb-3770 total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:487s > fi-kbl-7500u total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:456s > fi-kbl-7560u total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:565s > fi-skl-6260u total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:451s > fi-skl-6700hq total:278 pass:260 dwarn:0 dfail:0 fail:1 skip:17 time:567s > fi-skl-6700k total:278 pass:255 dwarn:4 dfail:0 fail:1 skip:18 time:467s > fi-skl-6770hq total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:486s > fi-skl-gvtdvm total:278 pass:264 dwarn:0 dfail:0 fail:1 skip:13 time:436s > fi-snb-2520m total:278 pass:249 dwarn:0 dfail:0 fail:1 skip:28 time:527s > fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:412s > > fdc0fa0d6347cadec14396a172a98636e9dfebc0 drm-tip: 2017y-04m-07d-10h-32m-49s UTC integration manifest > 08e143d drm/i915: Simplify shrinker locking > 60dc19c drm/i915: Don't call synchronize_rcu_expedited under struct_mutex > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4439/
Hello Joonas, On Fri, Apr 07, 2017 at 01:49:34PM +0300, Joonas Lahtinen wrote: > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c > index 2978acd..129ed30 100644 > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c > @@ -53,6 +53,17 @@ static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock) > BUG(); > } > > +static void i915_gem_shrinker_unlock(struct drm_device *dev, bool unlock) > +{ > + if (!unlock) > + return; > + > + mutex_unlock(&dev->struct_mutex); > + > + /* expedite the RCU grace period to free some request slabs */ > + synchronize_rcu_expedited(); > +} > + > static bool any_vma_pinned(struct drm_i915_gem_object *obj) > { > struct i915_vma *vma; > @@ -232,11 +243,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, > intel_runtime_pm_put(dev_priv); > > i915_gem_retire_requests(dev_priv); > - if (unlock) > - mutex_unlock(&dev_priv->drm.struct_mutex); > > - /* expedite the RCU grace period to free some request slabs */ > - synchronize_rcu_expedited(); > + i915_gem_shrinker_unlock(&dev_priv->drm, unlock); > } > @@ -296,8 +304,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) > count += obj->base.size >> PAGE_SHIFT; > } > > - if (unlock) > - mutex_unlock(&dev->struct_mutex); > + i915_gem_shrinker_unlock(dev, unlock); Why here? This doesn't make sense, all it matters is the throttling to happen when scan_objects is run, if count_objects is run it's not worth it to wait a quiescent point and to call synchronize_rcu_expedited(), it is *very* expensive. I recommend to reverse this hunk, I think it's worsening the runtime with zero benefits to increase the reliability of reclaim. > return count; > } > @@ -324,8 +331,8 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > sc->nr_to_scan - freed, > I915_SHRINK_BOUND | > I915_SHRINK_UNBOUND); > - if (unlock) > - mutex_unlock(&dev->struct_mutex); > + > + i915_gem_shrinker_unlock(dev, unlock); > > return freed; > } Perfect here, faster than re-reading the mutex too, already thought of checking unlock instead. Although if that's the only place it can as well be explicit. > @@ -367,8 +374,7 @@ i915_gem_shrinker_unlock_uninterruptible(struct drm_i915_private *dev_priv, > struct shrinker_lock_uninterruptible *slu) > { > dev_priv->mm.interruptible = slu->was_interruptible; > - if (slu->unlock) > - mutex_unlock(&dev_priv->drm.struct_mutex); > + i915_gem_shrinker_unlock(&dev_priv->drm, slu->unlock); > } i915_gem_shrinker_unlock_uninterruptible is more of a helper function but it doesn't always make sense to wait a rcu grace period. I think it'd be better to be selective in when to explicitly run synchronize_rcu_expedited(); in fact in some case synchronize_sched() may be prefereable and it will cause less wasted CPU cycles at the only downside of higher latency, it's all about tradeoffs which one should be called as they're equivalent as far as i915 is concerned. I don't think calling synchronize_rcu_expedited(); unconditionally in a unlock helper is ok and it'd be better to decided if to call (and which one) on a case by case basis. I kind I prefer my patch, just cleaned up with the synchronize_rcu_expedited under if (unlock) { mutex_unlock; synchronize_rcu... }. I'd also like to see all those mutex_trylock_recursive dropped, the only place where it makes sense to use it really is i915_gem_shrinker_scan and i915_gem_shrinker_count, unless in fact we want to replace it there too with a mutex_trylock and stop the recursive behavior of reclaim into DRM code. The other cases where the code uses lock recursion don't make much sense to me, I think the code would be simpler if the information on the struct_mutex being hold would be passed as parameter in all other cases. It'd be likely faster as well for the same reason why checking "unlock" is better above than checking the mutex.
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 2978acd..129ed30 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -53,6 +53,17 @@ static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock) BUG(); } +static void i915_gem_shrinker_unlock(struct drm_device *dev, bool unlock) +{ + if (!unlock) + return; + + mutex_unlock(&dev->struct_mutex); + + /* expedite the RCU grace period to free some request slabs */ + synchronize_rcu_expedited(); +} + static bool any_vma_pinned(struct drm_i915_gem_object *obj) { struct i915_vma *vma; @@ -232,11 +243,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, intel_runtime_pm_put(dev_priv); i915_gem_retire_requests(dev_priv); - if (unlock) - mutex_unlock(&dev_priv->drm.struct_mutex); - /* expedite the RCU grace period to free some request slabs */ - synchronize_rcu_expedited(); + i915_gem_shrinker_unlock(&dev_priv->drm, unlock); return count; } @@ -296,8 +304,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) count += obj->base.size >> PAGE_SHIFT; } - if (unlock) - mutex_unlock(&dev->struct_mutex); + i915_gem_shrinker_unlock(dev, unlock); return count; } @@ -324,8 +331,8 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) sc->nr_to_scan - freed, I915_SHRINK_BOUND | I915_SHRINK_UNBOUND); - if (unlock) - mutex_unlock(&dev->struct_mutex); + + i915_gem_shrinker_unlock(dev, unlock); return freed; } @@ -367,8 +374,7 @@ i915_gem_shrinker_unlock_uninterruptible(struct drm_i915_private *dev_priv, struct shrinker_lock_uninterruptible *slu) { dev_priv->mm.interruptible = slu->was_interruptible; - if (slu->unlock) - mutex_unlock(&dev_priv->drm.struct_mutex); + i915_gem_shrinker_unlock(&dev_priv->drm, slu->unlock); } static int
Only call synchronize_rcu_expedited after unlocking struct_mutex to avoid deadlock because the workqueues depend on struct_mutex. From original patch by Andrea: synchronize_rcu/synchronize_sched/synchronize_rcu_expedited() will hang until its own workqueues are run. The i915 gem workqueues will wait on the struct_mutex to be released. So we cannot wait for a quiescent state using those rcu primitives while holding the struct_mutex or it creates a circular lock dependency resulting in kernel hangs (which is reproducible but goes undetected by lockdep). kswapd0 D 0 700 2 0x00000000 Call Trace: ? __schedule+0x1a5/0x660 ? schedule+0x36/0x80 ? _synchronize_rcu_expedited.constprop.65+0x2ef/0x300 ? wake_up_bit+0x20/0x20 ? rcu_stall_kick_kthreads.part.54+0xc0/0xc0 ? rcu_exp_wait_wake+0x530/0x530 ? i915_gem_shrink+0x34b/0x4b0 ? i915_gem_shrinker_scan+0x7c/0x90 ? i915_gem_shrinker_scan+0x7c/0x90 ? shrink_slab.part.61.constprop.72+0x1c1/0x3a0 ? shrink_zone+0x154/0x160 ? kswapd+0x40a/0x720 ? kthread+0xf4/0x130 ? try_to_free_pages+0x450/0x450 ? kthread_create_on_node+0x40/0x40 ? ret_from_fork+0x23/0x30 plasmashell D 0 4657 4614 0x00000000 Call Trace: ? __schedule+0x1a5/0x660 ? schedule+0x36/0x80 ? schedule_preempt_disabled+0xe/0x10 ? __mutex_lock.isra.4+0x1c9/0x790 ? i915_gem_close_object+0x26/0xc0 ? i915_gem_close_object+0x26/0xc0 ? drm_gem_object_release_handle+0x48/0x90 ? drm_gem_handle_delete+0x50/0x80 ? drm_ioctl+0x1fa/0x420 ? drm_gem_handle_create+0x40/0x40 ? pipe_write+0x391/0x410 ? __vfs_write+0xc6/0x120 ? do_vfs_ioctl+0x8b/0x5d0 ? SyS_ioctl+0x3b/0x70 ? entry_SYSCALL_64_fastpath+0x13/0x94 kworker/0:0 D 0 29186 2 0x00000000 Workqueue: events __i915_gem_free_work Call Trace: ? __schedule+0x1a5/0x660 ? schedule+0x36/0x80 ? schedule_preempt_disabled+0xe/0x10 ? __mutex_lock.isra.4+0x1c9/0x790 ? del_timer_sync+0x44/0x50 ? update_curr+0x57/0x110 ? __i915_gem_free_objects+0x31/0x300 ? __i915_gem_free_objects+0x31/0x300 ? __i915_gem_free_work+0x2d/0x40 ? process_one_work+0x13a/0x3b0 ? worker_thread+0x4a/0x460 ? kthread+0xf4/0x130 ? process_one_work+0x3b0/0x3b0 ? kthread_create_on_node+0x40/0x40 ? ret_from_fork+0x23/0x30 Fixes: 3d3d18f086cd ("drm/i915: Avoid rcu_barrier() from reclaim paths (shrinker)") Reported-by: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)