Re: [PATCH 2/9] drm/syncobj: Lock around drm_syncobj::fence

[Chris Wilson]

Quoting Jason Ekstrand (2017-08-10 01:31:52)
> 
> 
> On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson  wrote:
> 
> Quoting Jason Ekstrand (2017-08-08 23:46:02)
> > The atomic exchange operation we were doing before in replace_fence was
> > sufficient for the case where it raced with itself.  However, if you
> > have a race between a replace_fence and dma_fence_get(syncobj->fence),
> > you may end up with the entire replace_fence happening between the point
> > in time where the one thread gets the syncobj->fence pointer and when it
> > calls dma_fence_get() on it.  If this happens, then the reference may be
> > dropped before we get a chance to get a new one.
> 
> This doesn't require a spinlock, just dma_fence_get_rcu_safe(). The
> argument for keeping this patch lies in the merit of later patches..
> 
> 
> Doesn't that also require that we start using an RCU for syncobj?  That was my
> interpretation of the hieroglyphics above the definition of get_rcu_safe()

Interesting you mention RCUing syncobj... The spinlock in
drm_syncobj_find() is the first contention point we hit. If we do make
syncobj RCU'ed we can avoid that lock.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/syncobj: Lock around drm_syncobj::fence

[Chris Wilson]

Quoting Jason Ekstrand (2017-08-10 01:31:52)
> 
> 
> On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson  wrote:
> 
> Quoting Jason Ekstrand (2017-08-08 23:46:02)
> > The atomic exchange operation we were doing before in replace_fence was
> > sufficient for the case where it raced with itself.  However, if you
> > have a race between a replace_fence and dma_fence_get(syncobj->fence),
> > you may end up with the entire replace_fence happening between the point
> > in time where the one thread gets the syncobj->fence pointer and when it
> > calls dma_fence_get() on it.  If this happens, then the reference may be
> > dropped before we get a chance to get a new one.
> 
> This doesn't require a spinlock, just dma_fence_get_rcu_safe(). The
> argument for keeping this patch lies in the merit of later patches..
> 
> 
> Doesn't that also require that we start using an RCU for syncobj?  That was my
> interpretation of the hieroglyphics above the definition of get_rcu_safe()

That we use RCU for the fence, which we do. i.e. so that the fence
pointer remains valid during the atomic_inc_unless_zero. The caller is
responsible for making sure that the syncobj remains valid across the
call (which we do using a ref, but it too could use RCU if that was ever
desired).
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/syncobj: Lock around drm_syncobj::fence

[Jason Ekstrand]

On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson 
wrote:

> Quoting Jason Ekstrand (2017-08-08 23:46:02)
> > The atomic exchange operation we were doing before in replace_fence was
> > sufficient for the case where it raced with itself.  However, if you
> > have a race between a replace_fence and dma_fence_get(syncobj->fence),
> > you may end up with the entire replace_fence happening between the point
> > in time where the one thread gets the syncobj->fence pointer and when it
> > calls dma_fence_get() on it.  If this happens, then the reference may be
> > dropped before we get a chance to get a new one.
>
> This doesn't require a spinlock, just dma_fence_get_rcu_safe(). The
> argument for keeping this patch lies in the merit of later patches..
>

Doesn't that also require that we start using an RCU for syncobj?  That was
my interpretation of the hieroglyphics above the definition of
get_rcu_safe()
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/syncobj: Lock around drm_syncobj::fence

[Chris Wilson]

Quoting Jason Ekstrand (2017-08-08 23:46:02)
> The atomic exchange operation we were doing before in replace_fence was
> sufficient for the case where it raced with itself.  However, if you
> have a race between a replace_fence and dma_fence_get(syncobj->fence),
> you may end up with the entire replace_fence happening between the point
> in time where the one thread gets the syncobj->fence pointer and when it
> calls dma_fence_get() on it.  If this happens, then the reference may be
> dropped before we get a chance to get a new one.

This doesn't require a spinlock, just dma_fence_get_rcu_safe(). The
argument for keeping this patch lies in the merit of later patches..
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel