drm/syncobj: Avoid kmalloc(GFP_KERNEL) under spinlock
diff mbox series

Message ID 20181026062027.12179-1-david1.zhou@amd.com
State New
Headers show
Series
  • drm/syncobj: Avoid kmalloc(GFP_KERNEL) under spinlock
Related show

Commit Message

Zhou, David(ChunMing) Oct. 26, 2018, 6:20 a.m. UTC
drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL

  Find functions that refer to GFP_KERNEL but are called with locks held.

Generated by: scripts/coccinelle/locks/call_kern.cocci

v2:
syncobj->timeline still needs protect.

v3:
use a global signaled fence instead of re-allocation.

v4:
Don't need moving lock.
Don't expose func.

v5:
rename func and directly return.

Tested by: syncobj_wait and ./deqp-vk -n dEQP-VK.*semaphore* with
lock debug kernel options enabled.

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: Christian König <easy2remember.chk@googlemail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
CC: Julia Lawall <julia.lawall@lip6.fr>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_syncobj.c | 36 ++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

Comments

Christian König Oct. 26, 2018, 7:43 a.m. UTC | #1
Am 26.10.18 um 08:20 schrieb Chunming Zhou:
> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL
>
>    Find functions that refer to GFP_KERNEL but are called with locks held.
>
> Generated by: scripts/coccinelle/locks/call_kern.cocci
>
> v2:
> syncobj->timeline still needs protect.
>
> v3:
> use a global signaled fence instead of re-allocation.
>
> v4:
> Don't need moving lock.
> Don't expose func.
>
> v5:
> rename func and directly return.
>
> Tested by: syncobj_wait and ./deqp-vk -n dEQP-VK.*semaphore* with
> lock debug kernel options enabled.
>
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Christian König <easy2remember.chk@googlemail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Julia Lawall <julia.lawall@lip6.fr>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_syncobj.c | 36 ++++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b7eaa603f368..d1c6f21c72b5 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -80,6 +80,23 @@ struct drm_syncobj_signal_pt {
>   	struct list_head list;
>   };
>   
> +static DEFINE_SPINLOCK(signaled_fence_lock);
> +static struct dma_fence signaled_fence;
> +
> +static struct dma_fence *drm_syncobj_get_stub_fence(void)
> +{
> +	spin_lock(&signaled_fence_lock);
> +	if (!signaled_fence.ops) {
> +		dma_fence_init(&signaled_fence,
> +			       &drm_syncobj_stub_fence_ops,
> +			       &signaled_fence_lock,
> +			       0, 0);
> +		dma_fence_signal_locked(&signaled_fence);
> +	}
> +	spin_unlock(&signaled_fence_lock);
> +
> +	return dma_fence_get(&signaled_fence);
> +}
>   /**
>    * drm_syncobj_find - lookup and reference a sync object.
>    * @file_private: drm file private pointer
> @@ -113,23 +130,8 @@ static struct dma_fence
>   	struct drm_syncobj_signal_pt *signal_pt;
>   
>   	if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
> -	    (point <= syncobj->timeline)) {
> -		struct drm_syncobj_stub_fence *fence =
> -			kzalloc(sizeof(struct drm_syncobj_stub_fence),
> -				GFP_KERNEL);
> -
> -		if (!fence)
> -			return NULL;
> -		spin_lock_init(&fence->lock);
> -		dma_fence_init(&fence->base,
> -			       &drm_syncobj_stub_fence_ops,
> -			       &fence->lock,
> -			       syncobj->timeline_context,
> -			       point);
> -
> -		dma_fence_signal(&fence->base);
> -		return &fence->base;
> -	}
> +	    (point <= syncobj->timeline))
> +		return drm_syncobj_get_stub_fence();
>   
>   	list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
>   		if (point > signal_pt->value)
Maarten Lankhorst Oct. 26, 2018, 11:49 a.m. UTC | #2
Op 26-10-18 om 08:20 schreef Chunming Zhou:
> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL
>
>   Find functions that refer to GFP_KERNEL but are called with locks held.
>
> Generated by: scripts/coccinelle/locks/call_kern.cocci
>
> v2:
> syncobj->timeline still needs protect.
>
> v3:
> use a global signaled fence instead of re-allocation.
>
> v4:
> Don't need moving lock.
> Don't expose func.
>
> v5:
> rename func and directly return.
>
> Tested by: syncobj_wait and ./deqp-vk -n dEQP-VK.*semaphore* with
> lock debug kernel options enabled.
>
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Christian König <easy2remember.chk@googlemail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Julia Lawall <julia.lawall@lip6.fr>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 36 ++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b7eaa603f368..d1c6f21c72b5 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -80,6 +80,23 @@ struct drm_syncobj_signal_pt {
>  	struct list_head list;
>  };
>  
> +static DEFINE_SPINLOCK(signaled_fence_lock);
> +static struct dma_fence signaled_fence;
> +
> +static struct dma_fence *drm_syncobj_get_stub_fence(void)
> +{
> +	spin_lock(&signaled_fence_lock);
> +	if (!signaled_fence.ops) {
> +		dma_fence_init(&signaled_fence,
> +			       &drm_syncobj_stub_fence_ops,
> +			       &signaled_fence_lock,
> +			       0, 0);
> +		dma_fence_signal_locked(&signaled_fence);
> +	}
> +	spin_unlock(&signaled_fence_lock);
Could this be used by drm_syncobj_assign_null_handle too? Maybe as a separate patch?

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

~Maarten
Christian König Oct. 26, 2018, 12:13 p.m. UTC | #3
Am 26.10.18 um 10:28 schrieb zhoucm1:
> Thanks, Could you help to submit to drm-misc again?

Done.

Christian.

>
> -David
>
>
> On 2018年10月26日 15:43, Christian König wrote:
>> Am 26.10.18 um 08:20 schrieb Chunming Zhou:
>>> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function 
>>> drm_syncobj_find_signal_pt_for_point called on line 390 inside lock 
>>> on line 389 but uses GFP_KERNEL
>>>
>>>    Find functions that refer to GFP_KERNEL but are called with locks 
>>> held.
>>>
>>> Generated by: scripts/coccinelle/locks/call_kern.cocci
>>>
>>> v2:
>>> syncobj->timeline still needs protect.
>>>
>>> v3:
>>> use a global signaled fence instead of re-allocation.
>>>
>>> v4:
>>> Don't need moving lock.
>>> Don't expose func.
>>>
>>> v5:
>>> rename func and directly return.
>>>
>>> Tested by: syncobj_wait and ./deqp-vk -n dEQP-VK.*semaphore* with
>>> lock debug kernel options enabled.
>>>
>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: Christian König <easy2remember.chk@googlemail.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> CC: Julia Lawall <julia.lawall@lip6.fr>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>>> ---
>>>   drivers/gpu/drm/drm_syncobj.c | 36 
>>> ++++++++++++++++++-----------------
>>>   1 file changed, 19 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>>> b/drivers/gpu/drm/drm_syncobj.c
>>> index b7eaa603f368..d1c6f21c72b5 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -80,6 +80,23 @@ struct drm_syncobj_signal_pt {
>>>       struct list_head list;
>>>   };
>>>   +static DEFINE_SPINLOCK(signaled_fence_lock);
>>> +static struct dma_fence signaled_fence;
>>> +
>>> +static struct dma_fence *drm_syncobj_get_stub_fence(void)
>>> +{
>>> +    spin_lock(&signaled_fence_lock);
>>> +    if (!signaled_fence.ops) {
>>> +        dma_fence_init(&signaled_fence,
>>> +                   &drm_syncobj_stub_fence_ops,
>>> +                   &signaled_fence_lock,
>>> +                   0, 0);
>>> +        dma_fence_signal_locked(&signaled_fence);
>>> +    }
>>> +    spin_unlock(&signaled_fence_lock);
>>> +
>>> +    return dma_fence_get(&signaled_fence);
>>> +}
>>>   /**
>>>    * drm_syncobj_find - lookup and reference a sync object.
>>>    * @file_private: drm file private pointer
>>> @@ -113,23 +130,8 @@ static struct dma_fence
>>>       struct drm_syncobj_signal_pt *signal_pt;
>>>         if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
>>> -        (point <= syncobj->timeline)) {
>>> -        struct drm_syncobj_stub_fence *fence =
>>> -            kzalloc(sizeof(struct drm_syncobj_stub_fence),
>>> -                GFP_KERNEL);
>>> -
>>> -        if (!fence)
>>> -            return NULL;
>>> -        spin_lock_init(&fence->lock);
>>> -        dma_fence_init(&fence->base,
>>> -                   &drm_syncobj_stub_fence_ops,
>>> -                   &fence->lock,
>>> -                   syncobj->timeline_context,
>>> -                   point);
>>> -
>>> -        dma_fence_signal(&fence->base);
>>> -        return &fence->base;
>>> -    }
>>> +        (point <= syncobj->timeline))
>>> +        return drm_syncobj_get_stub_fence();
>>>         list_for_each_entry(signal_pt, &syncobj->signal_pt_list, 
>>> list) {
>>>           if (point > signal_pt->value)
>>
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index b7eaa603f368..d1c6f21c72b5 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -80,6 +80,23 @@  struct drm_syncobj_signal_pt {
 	struct list_head list;
 };
 
+static DEFINE_SPINLOCK(signaled_fence_lock);
+static struct dma_fence signaled_fence;
+
+static struct dma_fence *drm_syncobj_get_stub_fence(void)
+{
+	spin_lock(&signaled_fence_lock);
+	if (!signaled_fence.ops) {
+		dma_fence_init(&signaled_fence,
+			       &drm_syncobj_stub_fence_ops,
+			       &signaled_fence_lock,
+			       0, 0);
+		dma_fence_signal_locked(&signaled_fence);
+	}
+	spin_unlock(&signaled_fence_lock);
+
+	return dma_fence_get(&signaled_fence);
+}
 /**
  * drm_syncobj_find - lookup and reference a sync object.
  * @file_private: drm file private pointer
@@ -113,23 +130,8 @@  static struct dma_fence
 	struct drm_syncobj_signal_pt *signal_pt;
 
 	if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
-	    (point <= syncobj->timeline)) {
-		struct drm_syncobj_stub_fence *fence =
-			kzalloc(sizeof(struct drm_syncobj_stub_fence),
-				GFP_KERNEL);
-
-		if (!fence)
-			return NULL;
-		spin_lock_init(&fence->lock);
-		dma_fence_init(&fence->base,
-			       &drm_syncobj_stub_fence_ops,
-			       &fence->lock,
-			       syncobj->timeline_context,
-			       point);
-
-		dma_fence_signal(&fence->base);
-		return &fence->base;
-	}
+	    (point <= syncobj->timeline))
+		return drm_syncobj_get_stub_fence();
 
 	list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
 		if (point > signal_pt->value)