Re: [PATCH] drm: fix deadlock of syncobj v2

[zhoucm1]

On 2018年10月22日 16:34, Chris Wilson wrote:

Quoting Chunming Zhou (2018-10-21 12:14:24)

v2:
add a mutex between sync_cb execution and free.

The result would appear to be that syncobj->lock is relegated to
protecting the pt_list and the mutex would only be needed for the
syncobj->cb_list.

The patch looks correct for resolving the deadlock and avoiding
introducing any new fails, but I do wonder if

spinlock_t pt_lock;
struct mutex cb_mutex;

and clearly separating the roles would not be a better approach.

good idea, thanks,

-David

-Chris


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: fix deadlock of syncobj v2

[Chris Wilson]

Quoting Chunming Zhou (2018-10-21 12:14:24)
> v2:
> add a mutex between sync_cb execution and free.

The result would appear to be that syncobj->lock is relegated to
protecting the pt_list and the mutex would only be needed for the
syncobj->cb_list.

The patch looks correct for resolving the deadlock and avoiding
introducing any new fails, but I do wonder if

spinlock_t pt_lock;
struct mutex cb_mutex;

and clearly separating the roles would not be a better approach.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm: fix deadlock of syncobj v2

[Zhou, David(ChunMing)]

Ping...
Btw:
The patch is tested by syncobj_basic and syncobj_wait of IGT.

> -Original Message-
> From: Chunming Zhou 
> Sent: Sunday, October 21, 2018 7:14 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Zhou, David(ChunMing) ; Daniel Vetter
> ; Chris Wilson ; Koenig, Christian
> 
> Subject: [PATCH] drm: fix deadlock of syncobj v2
> 
> v2:
> add a mutex between sync_cb execution and free.
> 
> Signed-off-by: Chunming Zhou 
> Cc: Daniel Vetter 
> Cc: Chris Wilson 
> Cc: Christian König 
> ---
>  drivers/gpu/drm/drm_syncobj.c | 11 +--
>  include/drm/drm_syncobj.h |  4 
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c
> b/drivers/gpu/drm/drm_syncobj.c index 57bf6006394d..c025a0b93565
> 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -158,9 +158,11 @@ void drm_syncobj_add_callback(struct drm_syncobj
> *syncobj,  void drm_syncobj_remove_callback(struct drm_syncobj *syncobj,
>struct drm_syncobj_cb *cb)
>  {
> + mutex_lock(&syncobj->mutex);
>   spin_lock(&syncobj->lock);
>   list_del_init(&cb->node);
>   spin_unlock(&syncobj->lock);
> + mutex_unlock(&syncobj->mutex);
>  }
> 
>  static void drm_syncobj_init(struct drm_syncobj *syncobj) @@ -344,13
> +346,17 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   drm_syncobj_create_signal_pt(syncobj, fence, pt_value);
>   if (fence) {
>   struct drm_syncobj_cb *cur, *tmp;
> + LIST_HEAD(cb_list);
> 
> + mutex_lock(&syncobj->mutex);
>   spin_lock(&syncobj->lock);
> - list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node)
> {
> + list_splice_init(&syncobj->cb_list, &cb_list);
> + spin_unlock(&syncobj->lock);
> + list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>   list_del_init(&cur->node);
>   cur->func(syncobj, cur);
>   }
> - spin_unlock(&syncobj->lock);
> + mutex_unlock(&syncobj->mutex);
>   }
>  }
>  EXPORT_SYMBOL(drm_syncobj_replace_fence);
> @@ -501,6 +507,7 @@ int drm_syncobj_create(struct drm_syncobj
> **out_syncobj, uint32_t flags,
>   kref_init(&syncobj->refcount);
>   INIT_LIST_HEAD(&syncobj->cb_list);
>   spin_lock_init(&syncobj->lock);
> + mutex_init(&syncobj->mutex);
>   if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>   syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>   else
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
> 5e8c5c027e09..3d3c8c181bd2 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -78,6 +78,10 @@ struct drm_syncobj {
>* @lock: Protects syncobj list and write-locks &fence.
>*/
>   spinlock_t lock;
> + /**
> +  * @mutex: mutex between syncobj cb execution and free.
> +  */
> + struct mutex mutex;
>   /**
>* @file: A file backing for this syncobj.
>*/
> --
> 2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel