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 at intel.com>
> Fixes regression from
> commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2]
> Author: Chris Wilson <chris at 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 at chris-wilson.co.uk>
> Cc: Jens Axboe <axboe at kernel.dk>
> Cc; "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Eero Tamminen <eero.t.tamminen at intel.com>
> Cc: "Rantala, Valtteri" <valtteri.rantala at intel.com>
> Cc: stable at 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;
> }
>
