[Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Daniel Vetter (2020-07-15 13:10:22)
> On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > When waiting with a callback on the stack, we must remove the callback
> > upon wait completion. Since this will be notified by the fence signal
> > callback, the removal often contends with the fence->lock being held by
> > the signaler. We can look at the list entry to see if the callback was
> > already signaled before we take the contended lock.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/dma-buf/dma-fence.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 8d5bdfce638e..b910d7bc0854 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> >       unsigned long flags;
> >       bool ret;
> >  
> > +     if (list_empty(&cb->node))
> 
> I was about to say "but the races" but then noticed that Paul fixed
> list_empty to use READ_ONCE like 5 years ago :-)

I'm always going "when exactly do we need list_empty_careful()"?

We can rule out a concurrent dma_fence_add_callback() for the same
dma_fence_cb, as that is a lost cause. So we only have to worry about
the concurrent list_del_init() from dma_fence_signal_locked(). So it's
the timing of
	list_del_init(): WRITE_ONCE(list->next, list)
vs
	READ_ONCE(list->next) == list
and we don't need to care about the trailing instructions in
list_del_init()...

Wait that trailing instruction is actually important here if the
dma_fence_cb is on the stack, or other imminent free.

Ok, this does need to be list_empty_careful!
-Chris