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

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Chris Wilson (2020-07-15 13:21:43)
> 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@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!

There's a further problem in that we call INIT_LIST_HEAD on the
dma_fence_cb before the signal callback. So even if list_empty_careful()
confirms the dma_fence_cb to be completely decoupled, the containing
struct may still be inuse.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel