| Message ID | 1502232369-19753-3-git-send-email-jason.ekstrand@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson <chris@chris-wilson.co.uk> 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()
Quoting Jason Ekstrand (2017-08-10 01:31:52) > > > On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson <chris@chris-wilson.co.uk> 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
Quoting Jason Ekstrand (2017-08-10 01:31:52) > > > On Wed, Aug 9, 2017 at 2:21 PM, Chris Wilson <chris@chris-wilson.co.uk> 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
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6f2a6041..dafca83 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -75,6 +75,22 @@ struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private, } EXPORT_SYMBOL(drm_syncobj_find); +struct dma_fence *drm_syncobj_fence_get(struct drm_syncobj *syncobj) +{ + struct dma_fence *fence; + + /* Lock to prevent someone from replacing the fence and dropping + * the syncobj's reference between when we get the dma_fence + * pointer and wehen we have a chance to incref. + */ + spin_lock(&syncobj->lock); + fence = dma_fence_get(syncobj->fence); + spin_unlock(&syncobj->lock); + + return fence; +} +EXPORT_SYMBOL(drm_syncobj_fence_get); + /** * drm_syncobj_replace_fence - replace fence in a sync object. * @file_private: drm file private pointer. @@ -91,7 +107,11 @@ void drm_syncobj_replace_fence(struct drm_file *file_private, if (fence) dma_fence_get(fence); - old_fence = xchg(&syncobj->fence, fence); + + spin_lock(&syncobj->lock); + old_fence = syncobj->fence; + syncobj->fence = fence; + spin_unlock(&syncobj->lock); dma_fence_put(old_fence); } @@ -107,7 +127,7 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (!syncobj) return -ENOENT; - *fence = dma_fence_get(syncobj->fence); + *fence = drm_syncobj_fence_get(syncobj); if (!*fence) { ret = -EINVAL; } @@ -143,6 +163,7 @@ static int drm_syncobj_create(struct drm_file *file_private, return -ENOMEM; kref_init(&syncobj->refcount); + spin_lock_init(&syncobj->lock); idr_preload(GFP_KERNEL); spin_lock(&file_private->syncobj_table_lock); diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index 9ffff22..422d631 100644 --- a/include/drm/drm_syncobj.h +++ b/include/drm/drm_syncobj.h @@ -46,6 +46,11 @@ struct drm_syncobj { */ struct dma_fence *fence; /** + * @spinlock: + * locks fence. + */ + spinlock_t lock; + /** * @file: * a file backing for this syncobj. */ @@ -79,6 +84,7 @@ drm_syncobj_put(struct drm_syncobj *obj) struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private, u32 handle); +struct dma_fence *drm_syncobj_fence_get(struct drm_syncobj *syncobj); void drm_syncobj_replace_fence(struct drm_file *file_private, struct drm_syncobj *syncobj, struct dma_fence *fence);
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. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++++++++++++-- include/drm/drm_syncobj.h | 6 ++++++ 2 files changed, 29 insertions(+), 2 deletions(-)