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

[Christian König]

Am 13.06.2017 um 04:25 schrieb Dave Airlie:
> On 13 June 2017 at 09:00, Christian König <deathsimple at vodafone.de> wrote:
>> Am 12.06.2017 um 22:31 schrieb Alex Xie:
>>> This patch is to move a loop of unref BOs and
>>> several memory free function calls out of
>>> critical sections.
>>>
>>> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> index a664987..02c138f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> @@ -75,9 +75,12 @@ static void amdgpu_bo_list_destroy(struct amdgpu_fpriv
>>> *fpriv, int id)
>>>                  /* Another user may have a reference to this list still */
>>>                  mutex_lock(&list->lock);
>>>                  mutex_unlock(&list->lock);
>>> +               mutex_unlock(&fpriv->bo_list_lock);
>>>                  amdgpu_bo_list_free(list);
>>>          }
>>> -       mutex_unlock(&fpriv->bo_list_lock);
>>> +       else {
>>> +               mutex_unlock(&fpriv->bo_list_lock);
>>> +       }
>>
>> You could move the unlock of bo_list_lock even before the if.
>>
>> But since you pointed it out there is quite a bug in this function:
>>>                  /* Another user may have a reference to this list still */
>>>                  mutex_lock(&list->lock);
>>>                  mutex_unlock(&list->lock);
>> Not sure if that is still up to date, but that use case used to be illegal
>> with mutexes.
> Oh wow, looks like this code has refcounting bugs, there should never
> be a reason to lock/unlock to find another user.

Actually that is not the issue.

We remove the bo_list object from the idr first which only works when 
nobody else is looking it up at the same time.

Taking and releasing the lock then should make sure that nobody is 
actually using the object any more in another thread.

> Can anyone say krefs?

The problem is rather that unlocking a mutex directly before it is 
released used to be illegal because mutexes work with their structure 
even after releasing it.

But the best solution would be to do the whole thing lockless with a 
kref and/or atomic updates.

Regards,
Christian.

>
> Dave.