| Message ID | 20190816152152.27550-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dma-buf: Avoid list_del during fence->cb_list iteration | expand |
Am 16.08.19 um 17:21 schrieb Chris Wilson: > Currently dma_fence_signal() tries to avoid the spinlock and only takes > it if absolutely required to walk the callback list. However, to allow > for some users to surreptitiously insert lazy signal callbacks that > do not depend on enabling the signaling mechanism around every fence, > we always need to notify the callbacks on signaling. As such, we will > always need to take the spinlock and dma_fence_signal() effectively > becomes a clone of dma_fence_signal_locked(). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Christian König <christian.koenig@amd.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/dma-buf/dma-fence.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index ff0cd6eae766..f23eb9f19b4e 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -176,6 +176,7 @@ EXPORT_SYMBOL(dma_fence_signal_locked); > int dma_fence_signal(struct dma_fence *fence) > { > unsigned long flags; > + int ret; > > if (!fence) > return -EINVAL; > @@ -183,21 +184,11 @@ int dma_fence_signal(struct dma_fence *fence) > if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > return -EINVAL; I need to take my review back. You also need to drop this test_and_set_bit here or your completely break drivers using this. Regards, Christian. > > - fence->timestamp = ktime_get(); > - set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); > - trace_dma_fence_signaled(fence); > - > - if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { > - struct dma_fence_cb *cur, *tmp; > + spin_lock_irqsave(fence->lock, flags); > + ret = dma_fence_signal_locked(fence); > + spin_unlock_irqrestore(fence->lock, flags); > > - spin_lock_irqsave(fence->lock, flags); > - list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) { > - list_del_init(&cur->node); > - cur->func(fence, cur); > - } > - spin_unlock_irqrestore(fence->lock, flags); > - } > - return 0; > + return ret; > } > EXPORT_SYMBOL(dma_fence_signal); >
On Fri, Aug 16, 2019 at 9:02 PM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 16.08.19 um 17:21 schrieb Chris Wilson: > > Currently dma_fence_signal() tries to avoid the spinlock and only takes > > it if absolutely required to walk the callback list. However, to allow > > for some users to surreptitiously insert lazy signal callbacks that > > do not depend on enabling the signaling mechanism around every fence, > > we always need to notify the callbacks on signaling. As such, we will > > always need to take the spinlock and dma_fence_signal() effectively > > becomes a clone of dma_fence_signal_locked(). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Christian König <christian.koenig@amd.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/dma-buf/dma-fence.c | 19 +++++-------------- > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index ff0cd6eae766..f23eb9f19b4e 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -176,6 +176,7 @@ EXPORT_SYMBOL(dma_fence_signal_locked); > > int dma_fence_signal(struct dma_fence *fence) > > { > > unsigned long flags; > > + int ret; > > > > if (!fence) > > return -EINVAL; > > @@ -183,21 +184,11 @@ int dma_fence_signal(struct dma_fence *fence) > > if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > > return -EINVAL; > > I need to take my review back. You also need to drop this > test_and_set_bit here or your completely break drivers using this. Time for a pile of dma_fence selftests? Maybe with a bit of dma_resv thrown in for good? This stuff is tricky, and it feels like we had a few oopsies in review recently ... -Daniel > Regards, > Christian. > > > > > - fence->timestamp = ktime_get(); > > - set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); > > - trace_dma_fence_signaled(fence); > > - > > - if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { > > - struct dma_fence_cb *cur, *tmp; > > + spin_lock_irqsave(fence->lock, flags); > > + ret = dma_fence_signal_locked(fence); > > + spin_unlock_irqrestore(fence->lock, flags); > > > > - spin_lock_irqsave(fence->lock, flags); > > - list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) { > > - list_del_init(&cur->node); > > - cur->func(fence, cur); > > - } > > - spin_unlock_irqrestore(fence->lock, flags); > > - } > > - return 0; > > + return ret; > > } > > EXPORT_SYMBOL(dma_fence_signal); > > >
Quoting Daniel Vetter (2019-08-16 20:07:24) > On Fri, Aug 16, 2019 at 9:02 PM Koenig, Christian > <Christian.Koenig@amd.com> wrote: > > > > Am 16.08.19 um 17:21 schrieb Chris Wilson: > > > Currently dma_fence_signal() tries to avoid the spinlock and only takes > > > it if absolutely required to walk the callback list. However, to allow > > > for some users to surreptitiously insert lazy signal callbacks that > > > do not depend on enabling the signaling mechanism around every fence, > > > we always need to notify the callbacks on signaling. As such, we will > > > always need to take the spinlock and dma_fence_signal() effectively > > > becomes a clone of dma_fence_signal_locked(). > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Christian König <christian.koenig@amd.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/dma-buf/dma-fence.c | 19 +++++-------------- > > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > index ff0cd6eae766..f23eb9f19b4e 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -176,6 +176,7 @@ EXPORT_SYMBOL(dma_fence_signal_locked); > > > int dma_fence_signal(struct dma_fence *fence) > > > { > > > unsigned long flags; > > > + int ret; > > > > > > if (!fence) > > > return -EINVAL; > > > @@ -183,21 +184,11 @@ int dma_fence_signal(struct dma_fence *fence) > > > if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > > > return -EINVAL; > > > > I need to take my review back. You also need to drop this > > test_and_set_bit here or your completely break drivers using this. First time we were here, it was just a plain test_bit(). Skipping a patch, so had to start from scratch... (I blame glancing at the original outcome and glossing over the test_bit vs test_and_set_bit.) > Time for a pile of dma_fence selftests? Maybe with a bit of dma_resv > thrown in for good? This stuff is tricky, and it feels like we had a > few oopsies in review recently ... dma_fence_signal vs dma_fence_add_callback Something like: while (!timeout) f1 = mock_fence_create() push f1 to other thread pull f2 from other thread dma_fence_signal(f2); dma_fence_add_callback(f1) dma_fence_signal(f1); check cb Can't see an obvious way to make the test_and_set_bit window larger. -Chris
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index ff0cd6eae766..f23eb9f19b4e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -176,6 +176,7 @@ EXPORT_SYMBOL(dma_fence_signal_locked); int dma_fence_signal(struct dma_fence *fence) { unsigned long flags; + int ret; if (!fence) return -EINVAL; @@ -183,21 +184,11 @@ int dma_fence_signal(struct dma_fence *fence) if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return -EINVAL; - fence->timestamp = ktime_get(); - set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); - trace_dma_fence_signaled(fence); - - if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) { - struct dma_fence_cb *cur, *tmp; + spin_lock_irqsave(fence->lock, flags); + ret = dma_fence_signal_locked(fence); + spin_unlock_irqrestore(fence->lock, flags); - spin_lock_irqsave(fence->lock, flags); - list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) { - list_del_init(&cur->node); - cur->func(fence, cur); - } - spin_unlock_irqrestore(fence->lock, flags); - } - return 0; + return ret; } EXPORT_SYMBOL(dma_fence_signal);
Currently dma_fence_signal() tries to avoid the spinlock and only takes it if absolutely required to walk the callback list. However, to allow for some users to surreptitiously insert lazy signal callbacks that do not depend on enabling the signaling mechanism around every fence, we always need to notify the callbacks on signaling. As such, we will always need to take the spinlock and dma_fence_signal() effectively becomes a clone of dma_fence_signal_locked(). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Christian König <christian.koenig@amd.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/dma-buf/dma-fence.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)