* [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration
@ 2019-08-16 15:21 Chris Wilson
2019-08-16 15:21 ` [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal Chris Wilson
2019-08-16 16:14 ` [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Koenig, Christian
0 siblings, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2019-08-16 15:21 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter, Christian König
Before we notify the fence signal callback, we remove the cb from the
list. However, since we are processing the entire list from underneath
the spinlock, we do not need to individual delete each element, but can
simply reset the link and the entire list.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Christian König <christian.koenig@amd.com>
---
drivers/dma-buf/dma-fence.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 8025a891d3e9..ff0cd6eae766 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -149,9 +149,12 @@ int dma_fence_signal_locked(struct dma_fence *fence)
trace_dma_fence_signaled(fence);
}
- list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
- list_del_init(&cur->node);
- cur->func(fence, cur);
+ if (!list_empty(&fence->cb_list)) {
+ list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
+ INIT_LIST_HEAD(&cur->node);
+ cur->func(fence, cur);
+ }
+ INIT_LIST_HEAD(&fence->cb_list);
}
return ret;
}
--
2.23.0.rc1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal
2019-08-16 15:21 [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Chris Wilson
@ 2019-08-16 15:21 ` Chris Wilson
2019-08-16 19:02 ` Koenig, Christian
2019-08-16 16:14 ` [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Koenig, Christian
1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2019-08-16 15:21 UTC (permalink / raw)
To: dri-devel; +Cc: Daniel Vetter, Christian König
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;
- 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);
--
2.23.0.rc1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal
2019-08-16 15:21 ` [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal Chris Wilson
@ 2019-08-16 19:02 ` Koenig, Christian
2019-08-16 19:07 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Koenig, Christian @ 2019-08-16 19:02 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: Daniel Vetter
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);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal
2019-08-16 19:02 ` Koenig, Christian
@ 2019-08-16 19:07 ` Daniel Vetter
2019-08-16 19:28 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2019-08-16 19:07 UTC (permalink / raw)
To: Koenig, Christian; +Cc: dri-devel
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);
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal
2019-08-16 19:07 ` Daniel Vetter
@ 2019-08-16 19:28 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2019-08-16 19:28 UTC (permalink / raw)
To: Koenig, Christian, Daniel Vetter; +Cc: dri-devel
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration
2019-08-16 15:21 [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Chris Wilson
2019-08-16 15:21 ` [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal Chris Wilson
@ 2019-08-16 16:14 ` Koenig, Christian
1 sibling, 0 replies; 6+ messages in thread
From: Koenig, Christian @ 2019-08-16 16:14 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: Daniel Vetter
Am 16.08.19 um 17:21 schrieb Chris Wilson:
> Before we notify the fence signal callback, we remove the cb from the
> list. However, since we are processing the entire list from underneath
> the spinlock, we do not need to individual delete each element, but can
> simply reset the link and the entire list.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Christian König <christian.koenig@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com> for the series.
> ---
> drivers/dma-buf/dma-fence.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 8025a891d3e9..ff0cd6eae766 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -149,9 +149,12 @@ int dma_fence_signal_locked(struct dma_fence *fence)
> trace_dma_fence_signaled(fence);
> }
>
> - list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> - list_del_init(&cur->node);
> - cur->func(fence, cur);
> + if (!list_empty(&fence->cb_list)) {
> + list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> + INIT_LIST_HEAD(&cur->node);
> + cur->func(fence, cur);
> + }
> + INIT_LIST_HEAD(&fence->cb_list);
> }
> return ret;
> }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-16 19:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 15:21 [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Chris Wilson
2019-08-16 15:21 ` [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal Chris Wilson
2019-08-16 19:02 ` Koenig, Christian
2019-08-16 19:07 ` Daniel Vetter
2019-08-16 19:28 ` Chris Wilson
2019-08-16 16:14 ` [PATCH 1/2] dma-buf: Avoid list_del during fence->cb_list iteration Koenig, Christian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.