On 2017-06-13 06:23 AM, Christian König wrote:
> Am 13.06.2017 um 08:29 schrieb axie:
>> On 2017-06-12 07:00 PM, Christian König 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.
>> As I understand this piece of code, these mutex_lock and mutex_unlock
>> pair are used to make sure all other tasks have finished
>> access of the bo list. Another side of this story is in function
>> amdgpu_bo_list_get. These two piece of codes together make sure
>> we can safely destroy bo list.
>
> Yeah, the idea behind the code is correct. But using mutexes in that
> way is illegal, see here https://lwn.net/Articles/575460/.
>
> I'm not sure if that is still up to date, but in ancient times you
> needed to avoid patterns like this:
> mutex_unlock(&obj->lock);
> kfree(obj);
>
> Anyway I suggest we just replace the whole bo_list handling with and
> RCU and refcount based implementation. That should avoid the whole
> locking for the read only code path.
>
> Regards,
> Christian.
>
The article may be still generally applicable. But it is not applicable
for amdgpu_bo_list.c.
amdgpu_bo_list.c has two extra steps to avoid similar issue.
1. In function amdgpu_bo_list_destroy,
...
down_write(&fpriv->bo_list_lock); //Note that the bo_list_lock is
mutex or write lock here.
list = idr_remove(&fpriv->bo_list_handles, id);
...
With the above statements, there is no new task(user) can access the bo
list.
2. With the above statments, there is no task currently waiting for the
bo list lock after function amdgpu_bo_list_destroy holds bo_list_lock.
Why? In function amdgpu_bo_list_get, other task need to hold
bo_list_lock mutex or read lock in order to wait for result->lock.
amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id)
{
...
down_read(&fpriv->bo_list_lock);
...
mutex_lock(&result->lock);
...
up_read(&fpriv->bo_list_lock);
}
So when amdgpu_bo_list_destroy function is holding bo_list_lock. All
other tasks are not waiting for result->lock.
3.
After the above 2 points, we know that when destroy function is holding
bo_list_lock, there can only be zero/one task is holding
list->lock. There is no other scenario.
The following piece of code wait for possible existing bo list
task(user) finishes accessing the bo list:
static void amdgpu_bo_list_destroy(struct amdgpu_fpriv *fpriv, int id)
{
...
down_write(&fpriv->bo_list_lock);
...
/* Another user may have a reference to this list still */
mutex_lock(&list->lock); //// WAITing for the exising
task/user unlock, after this waiting, we can safely destroy and free the
mutex. [Alex Bin]
mutex_unlock(&list->lock);
up_write(&fpriv->bo_list_lock);
amdgpu_bo_list_free(list);
...
NOTE: When we are holding bo_list_lock, there is only one possible
scenario for list->lock left, that is,
in function amdgpu_bo_list_put, to unlock list->lock.
PLEASE give me example to prove if I miss anything.
Thanks for all other comments.
RCU lock and kref might be a cleaner solution than rw_semaphore. But
that requires bigger change to current implementation.
I would have a look at that later.
Regards,
Alex Bin Xie
>>
>> Otherwise we can easily simplify these lockings.
>> Let me give an example here.
>>
>> In function amdgpu_bo_list_get, assuming we change the code like this:
>> ...
>> down_read(&fpriv->bo_list_lock);
>> result = idr_find(&fpriv->bo_list_handles, id);
>> up_read(&fpriv->bo_list_lock);
>> /**Line 1. Task A was scheduled away from CPU**/
>> if (result)
>> mutex_lock(&result->lock);
>> ...
>>
>> In function amdgpu_bo_list_destroy, assuming we change the code like
>> this:
>> ...
>> down_write(&fpriv->bo_list_lock);
>> list = idr_remove(&fpriv->bo_list_handles, id);
>> up_write(&fpriv->bo_list_lock);
>> if (list) {
>> /* Another user may have a reference to this list still */
>> mutex_lock(&list->lock);
>> mutex_unlock(&list->lock);
>> amdgpu_bo_list_free(list);
>> }
>> }
>>
>> When task A is running in function amdgpu_bo_list_get in line 1, CPU
>> scheduler takes CPU away from task A.
>> Then Task B run function amdgpu_bo_list_destroy. Task B can run all
>> the way to destroy and free mutex.
>> Later Task A is back to run. The mutex result->lock has been
>> destroyed by task B. Now task A try to lock a mutex
>> which has been destroyed and freed.
>>
>>
>>>
>>> Christian.
>>>
>>>> }
>>>> static int amdgpu_bo_list_set(struct amdgpu_device *adev,
>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
