Quoting Tvrtko Ursulin (2020-08-07 12:26:41)
>
> On 07/08/2020 09:32, Chris Wilson wrote:
> > static void signal_irq_work(struct irq_work *work)
> > {
> > struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
> > const ktime_t timestamp = ktime_get();
> > + struct llist_node *signal, *sn;
> > struct intel_context *ce, *cn;
> > struct list_head *pos, *next;
> > - LIST_HEAD(signal);
> > +
> > + signal = NULL;
> > + if (unlikely(!llist_empty(&b->signaled_requests)))
> > + signal = llist_del_all(&b->signaled_requests);
> > @@ -242,7 +248,9 @@ static void signal_irq_work(struct irq_work *work)
> > * spinlock as the callback chain may end up adding
> > * more signalers to the same context or engine.
> > */
> > - __signal_request(rq, &signal);
> > + clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > + if (__signal_request(rq))
> > + signal = __llist_add(&rq->signal_node, signal);
>
> Presumably here you count on no possible races, allowing for a more
> optimized, custom, __llist_add. It needs a comment at minimum, or even
> better just use llist_add.
It's a purely local singly linked list here. We own the
request->signal_node as that is locked by the b->irq_lock and the
clear_bit, and signal is a local variable.
-Chris