On Sun, Aug 07, 2016 at 03:45:12PM +0100, Chris Wilson wrote:
> The bottom-half we use for processing the breadcrumb interrupt is a
> task, which is an RCU protected struct. When accessing this struct, we
> need to be holding the RCU read lock to prevent it disappearing beneath
> us. We can use the RCU annotation to mark our irq_seqno_bh pointer as
> being under RCU guard and then use the RCU accessors to both provide
> correct ordering of access through the pointer.
>
> Most notably, this fixes the access from hard irq context to use the RCU
> read lock, which both Daniel and Tvrtko complained about.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
I'll leave sparse-checking this to 0day and runtime lockdep checking to CI
;-)
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 22 +++++++++-------------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 --
> drivers/gpu/drm/i915/intel_ringbuffer.h | 21 ++++++++++++++-------
> 4 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index feec00f769e1..3d546b5c2e4c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3848,7 +3848,7 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
> * is woken.
> */
> if (engine->irq_seqno_barrier &&
> - READ_ONCE(engine->breadcrumbs.irq_seqno_bh) == current &&
> + rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh) == current &&
> cmpxchg_relaxed(&engine->breadcrumbs.irq_posted, 1, 0)) {
> struct task_struct *tsk;
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 8ecb3b6538fc..7552bd039565 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -60,10 +60,8 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
> * every jiffie in order to kick the oldest waiter to do the
> * coherent seqno check.
> */
> - rcu_read_lock();
> if (intel_engine_wakeup(engine))
> mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> - rcu_read_unlock();
> }
>
> static void irq_enable(struct intel_engine_cs *engine)
> @@ -232,7 +230,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
> }
> rb_link_node(&wait->node, parent, p);
> rb_insert_color(&wait->node, &b->waiters);
> - GEM_BUG_ON(!first && !b->irq_seqno_bh);
> + GEM_BUG_ON(!first && !rcu_access_pointer(b->irq_seqno_bh));
Nit: reading through rcu docs I think the suggested accessor here is
rcu_dereference_protected for write-side access. That one allows the
compiler full freedom for reordering. otoh it's a bit more noise-y and meh
about optional debug checks anyway. So with or without that change:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> if (completed) {
> struct rb_node *next = rb_next(completed);
> @@ -242,7 +240,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
> GEM_BUG_ON(first);
> b->timeout = wait_timeout();
> b->first_wait = to_wait(next);
> - smp_store_mb(b->irq_seqno_bh, b->first_wait->tsk);
> + rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
> /* As there is a delay between reading the current
> * seqno, processing the completed tasks and selecting
> * the next waiter, we may have missed the interrupt
> @@ -269,7 +267,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
> GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
> b->timeout = wait_timeout();
> b->first_wait = wait;
> - smp_store_mb(b->irq_seqno_bh, wait->tsk);
> + rcu_assign_pointer(b->irq_seqno_bh, wait->tsk);
> /* After assigning ourselves as the new bottom-half, we must
> * perform a cursory check to prevent a missed interrupt.
> * Either we miss the interrupt whilst programming the hardware,
> @@ -280,7 +278,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
> */
> __intel_breadcrumbs_enable_irq(b);
> }
> - GEM_BUG_ON(!b->irq_seqno_bh);
> + GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh));
> GEM_BUG_ON(!b->first_wait);
> GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
>
> @@ -335,7 +333,7 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> const int priority = wakeup_priority(b, wait->tsk);
> struct rb_node *next;
>
> - GEM_BUG_ON(b->irq_seqno_bh != wait->tsk);
> + GEM_BUG_ON(rcu_access_pointer(b->irq_seqno_bh) != wait->tsk);
>
> /* We are the current bottom-half. Find the next candidate,
> * the first waiter in the queue on the remaining oldest
> @@ -379,13 +377,13 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> */
> b->timeout = wait_timeout();
> b->first_wait = to_wait(next);
> - smp_store_mb(b->irq_seqno_bh, b->first_wait->tsk);
> + rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
> if (b->first_wait->seqno != wait->seqno)
> __intel_breadcrumbs_enable_irq(b);
> - wake_up_process(b->irq_seqno_bh);
> + wake_up_process(b->first_wait->tsk);
> } else {
> b->first_wait = NULL;
> - WRITE_ONCE(b->irq_seqno_bh, NULL);
> + rcu_assign_pointer(b->irq_seqno_bh, NULL);
> __intel_breadcrumbs_disable_irq(b);
> }
> } else {
> @@ -399,7 +397,7 @@ out_unlock:
> GEM_BUG_ON(b->first_wait == wait);
> GEM_BUG_ON(rb_first(&b->waiters) !=
> (b->first_wait ? &b->first_wait->node : NULL));
> - GEM_BUG_ON(!b->irq_seqno_bh ^ RB_EMPTY_ROOT(&b->waiters));
> + GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh) ^ RB_EMPTY_ROOT(&b->waiters));
> spin_unlock(&b->lock);
> }
>
> @@ -596,11 +594,9 @@ unsigned int intel_kick_waiters(struct drm_i915_private *i915)
> * RCU lock, i.e. as we call wake_up_process() we must be holding the
> * rcu_read_lock().
> */
> - rcu_read_lock();
> for_each_engine(engine, i915)
> if (unlikely(intel_engine_wakeup(engine)))
> mask |= intel_engine_flag(engine);
> - rcu_read_unlock();
>
> return mask;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e08a1e1b04e4..16b726fe33eb 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2410,9 +2410,7 @@ void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno)
> /* After manually advancing the seqno, fake the interrupt in case
> * there are any waiters for that seqno.
> */
> - rcu_read_lock();
> intel_engine_wakeup(engine);
> - rcu_read_unlock();
> }
>
> static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 4aed4586b0b6..66dc93469076 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -171,7 +171,7 @@ struct intel_engine_cs {
> * the overhead of waking that client is much preferred.
> */
> struct intel_breadcrumbs {
> - struct task_struct *irq_seqno_bh; /* bh for user interrupts */
> + struct task_struct __rcu *irq_seqno_bh; /* bh for interrupts */
> bool irq_posted;
>
> spinlock_t lock; /* protects the lists of requests */
> @@ -541,23 +541,30 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request);
>
> static inline bool intel_engine_has_waiter(struct intel_engine_cs *engine)
> {
> - return READ_ONCE(engine->breadcrumbs.irq_seqno_bh);
> + return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
> }
>
> static inline bool intel_engine_wakeup(struct intel_engine_cs *engine)
> {
> bool wakeup = false;
> - struct task_struct *tsk = READ_ONCE(engine->breadcrumbs.irq_seqno_bh);
> +
> /* Note that for this not to dangerously chase a dangling pointer,
> - * the caller is responsible for ensure that the task remain valid for
> - * wake_up_process() i.e. that the RCU grace period cannot expire.
> + * we must hold the rcu_read_lock here.
> *
> * Also note that tsk is likely to be in !TASK_RUNNING state so an
> * early test for tsk->state != TASK_RUNNING before wake_up_process()
> * is unlikely to be beneficial.
> */
> - if (tsk)
> - wakeup = wake_up_process(tsk);
> + if (rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh)) {
> + struct task_struct *tsk;
> +
> + rcu_read_lock();
> + tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> + if (tsk)
> + wakeup = wake_up_process(tsk);
> + rcu_read_unlock();
> + }
> +
> return wakeup;
> }
>
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
