[PATCH 2/4] drm/amdgpu: Optimization of critical section

[Christian König]

Am 13.06.2017 um 08:54 schrieb axie:
>
> On 2017-06-12 06:47 PM, Christian König wrote:
>> Am 12.06.2017 um 22:31 schrieb Alex Xie:
>>> Make the critical section smaller. There is no
>>> need to protect the bo_list, because there is
>>> no other task can access the newly created BO
>>> list yet.
>>
>> NAK, a task can guess the next id number so could get the kernel to 
>> use the structure before it is initialized.
>>
>> Christian.
>>
> How did you find such an extreme corner case?

Twenty-two years of experience working with that stuff, have seen all of 
that before.

> I am fine with this comment. Tuesday/Wednesday I will address it with 
> next version of patch set.
>
> Currently, there are 2 options:
> Option 1: I may use a write_lock in the create function. And restore 
> the original code for the creation of BO list.
> Option 2: I may move the function call of idr_alloc to the end of the 
> creation of BO list ioctl. This is more efficient but
> the code look dirty.

Option #2 doesn't sound so dirty to me. Properly initializing the 
bo_list and then adding it to the idr actually sounds rather sane to me.

The only tricky part is that the currently helper function doesn't 
really match that use case.

BTW: Something I've just notices while working on this:
> *result = kzalloc(sizeof(struct amdgpu_bo_list), GFP_KERNEL);
...
>         mutex_init(&(*result)->lock);
>         (*result)->num_entries = 0;
>         (*result)->array = NULL;

We can probably save quite some CPU cycles when we still to zero 
initialize the structure twice.

Regards,
Christian.

>
>
>>>
>>> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> index 02c138f..c994a04 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> @@ -48,8 +48,9 @@ static int amdgpu_bo_list_create(struct 
>>> amdgpu_fpriv *fpriv,
>>>       mutex_lock(&fpriv->bo_list_lock);
>>>       r = idr_alloc(&fpriv->bo_list_handles, *result,
>>>                 1, 0, GFP_KERNEL);
>>> +    mutex_unlock(&fpriv->bo_list_lock);
>>> +
>>>       if (r < 0) {
>>> -        mutex_unlock(&fpriv->bo_list_lock);
>>>           kfree(*result);
>>>           return r;
>>>       }
>>> @@ -60,7 +61,6 @@ static int amdgpu_bo_list_create(struct 
>>> amdgpu_fpriv *fpriv,
>>>       (*result)->array = NULL;
>>>         mutex_lock(&(*result)->lock);
>>> -    mutex_unlock(&fpriv->bo_list_lock);
>>>         return 0;
>>>   }
>>
>>
>