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....@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.
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
