| Message ID | 1447594364-4206-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 15/11/15 13:32, Chris Wilson wrote: > The busywait in __i915_spin_request() does not respect pending signals > and so may consume the entire timeslice for the task instead of > returning to userspace to handle the signal. Obviously correct to break the spin, but if spending a jiffie to react to signals was the only problem then it is not too severe. Add something to the commit message about how it was found/reported and about the severity of impact, etc? Otherwise, Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Fixes regression from > commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2] > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Tue Apr 7 16:20:41 2015 +0100 > > drm/i915: Optimistically spin for the request completion > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jens Axboe <axboe@kernel.dk> > Cc; "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Eero Tamminen <eero.t.tamminen@intel.com> > Cc: "Rantala, Valtteri" <valtteri.rantala@intel.com> > Cc: stable@kernel.vger.org > --- > drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 5cf4a1998273..740530c571d1 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1146,7 +1146,7 @@ static bool missed_irq(struct drm_i915_private *dev_priv, > return test_bit(ring->id, &dev_priv->gpu_error.missed_irq_rings); > } > > -static int __i915_spin_request(struct drm_i915_gem_request *req) > +static int __i915_spin_request(struct drm_i915_gem_request *req, int state) > { > unsigned long timeout; > > @@ -1158,6 +1158,9 @@ static int __i915_spin_request(struct drm_i915_gem_request *req) > if (i915_gem_request_completed(req, true)) > return 0; > > + if (signal_pending_state(state, current)) > + break; > + > if (time_after_eq(jiffies, timeout)) > break; > > @@ -1197,6 +1200,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > struct drm_i915_private *dev_priv = dev->dev_private; > const bool irq_test_in_progress = > ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_ring_flag(ring); > + int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; > DEFINE_WAIT(wait); > unsigned long timeout_expire; > s64 before, now; > @@ -1221,7 +1225,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > before = ktime_get_raw_ns(); > > /* Optimistic spin for the next jiffie before touching IRQs */ > - ret = __i915_spin_request(req); > + ret = __i915_spin_request(req, state); > if (ret == 0) > goto out; > > @@ -1233,8 +1237,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > for (;;) { > struct timer_list timer; > > - prepare_to_wait(&ring->irq_queue, &wait, > - interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); > + prepare_to_wait(&ring->irq_queue, &wait, state); > > /* We need to check whether any gpu reset happened in between > * the caller grabbing the seqno and now ... */ > @@ -1252,7 +1255,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, > break; > } > > - if (interruptible && signal_pending(current)) { > + if (signal_pending_state(state, current)) { > ret = -ERESTARTSYS; > break; > } >
On Mon, Nov 16, 2015 at 09:54:10AM +0000, Tvrtko Ursulin wrote: > > Hi, > > On 15/11/15 13:32, Chris Wilson wrote: > >The busywait in __i915_spin_request() does not respect pending signals > >and so may consume the entire timeslice for the task instead of > >returning to userspace to handle the signal. > > Obviously correct to break the spin, but if spending a jiffie to > react to signals was the only problem then it is not too severe. > > Add something to the commit message about how it was found/reported > and about the severity of impact, etc? Perhaps: At the worst case this could cause a delay in signal processing of 20ms, which would be a noticeable jitter in cursor tracking. If a higher resolution signal was being used, for example to provide fairness of a server timeslices between clients, we could expect to detect some unfairness between clients. This issue was noticed when inspecting a report of poor interactivity resulting from excessively high __i915_spin_request usage. -Chris
On 16/11/15 11:22, Chris Wilson wrote: > On Mon, Nov 16, 2015 at 09:54:10AM +0000, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 15/11/15 13:32, Chris Wilson wrote: >>> The busywait in __i915_spin_request() does not respect pending signals >>> and so may consume the entire timeslice for the task instead of >>> returning to userspace to handle the signal. >> >> Obviously correct to break the spin, but if spending a jiffie to >> react to signals was the only problem then it is not too severe. >> >> Add something to the commit message about how it was found/reported >> and about the severity of impact, etc? > > Perhaps: > > At the worst case this could cause a delay in signal processing of 20ms, > which would be a noticeable jitter in cursor tracking. If a higher > resolution signal was being used, for example to provide fairness of a > server timeslices between clients, we could expect to detect some > unfairness between clients. This issue was noticed when inspecting a > report of poor interactivity resulting from excessively high > __i915_spin_request usage. Oh its the Xorg scheduler tick... I always forget about that. Was thinking that it is only about fatal, or at least infrequent signals. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 5cf4a1998273..740530c571d1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1146,7 +1146,7 @@ static bool missed_irq(struct drm_i915_private *dev_priv, return test_bit(ring->id, &dev_priv->gpu_error.missed_irq_rings); } -static int __i915_spin_request(struct drm_i915_gem_request *req) +static int __i915_spin_request(struct drm_i915_gem_request *req, int state) { unsigned long timeout; @@ -1158,6 +1158,9 @@ static int __i915_spin_request(struct drm_i915_gem_request *req) if (i915_gem_request_completed(req, true)) return 0; + if (signal_pending_state(state, current)) + break; + if (time_after_eq(jiffies, timeout)) break; @@ -1197,6 +1200,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, struct drm_i915_private *dev_priv = dev->dev_private; const bool irq_test_in_progress = ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_ring_flag(ring); + int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; DEFINE_WAIT(wait); unsigned long timeout_expire; s64 before, now; @@ -1221,7 +1225,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, before = ktime_get_raw_ns(); /* Optimistic spin for the next jiffie before touching IRQs */ - ret = __i915_spin_request(req); + ret = __i915_spin_request(req, state); if (ret == 0) goto out; @@ -1233,8 +1237,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, for (;;) { struct timer_list timer; - prepare_to_wait(&ring->irq_queue, &wait, - interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); + prepare_to_wait(&ring->irq_queue, &wait, state); /* We need to check whether any gpu reset happened in between * the caller grabbing the seqno and now ... */ @@ -1252,7 +1255,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, break; } - if (interruptible && signal_pending(current)) { + if (signal_pending_state(state, current)) { ret = -ERESTARTSYS; break; }
The busywait in __i915_spin_request() does not respect pending signals and so may consume the entire timeslice for the task instead of returning to userspace to handle the signal. Fixes regression from commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2] Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Apr 7 16:20:41 2015 +0100 drm/i915: Optimistically spin for the request completion Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jens Axboe <axboe@kernel.dk> Cc; "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Eero Tamminen <eero.t.tamminen@intel.com> Cc: "Rantala, Valtteri" <valtteri.rantala@intel.com> Cc: stable@kernel.vger.org --- drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)