On Sat, Aug 12, 2017 at 12:56 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
>
> From: Yair Shachar <yair.shachar at amd.com>
>
> Signed-off-by: Yair Shachar <yair.shachar at amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 6316aad..2a45718e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -451,8 +451,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
> return -EINVAL;
> }
>
> - mutex_lock(kfd_get_dbgmgr_mutex());
> mutex_lock(&p->mutex);
> + mutex_lock(kfd_get_dbgmgr_mutex());
>
> /*
> * make sure that we have pdd, if this the first queue created for
> @@ -460,8 +460,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
> */
> pdd = kfd_bind_process_to_device(dev, p);
> if (IS_ERR(pdd)) {
> - mutex_unlock(&p->mutex);
> mutex_unlock(kfd_get_dbgmgr_mutex());
> + mutex_unlock(&p->mutex);
> return PTR_ERR(pdd);
> }
>
> @@ -480,8 +480,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
> status = -EINVAL;
> }
>
> - mutex_unlock(&p->mutex);
> mutex_unlock(kfd_get_dbgmgr_mutex());
> + mutex_unlock(&p->mutex);
>
> return status;
> }
> --
> 2.7.4
>
Hi Felix,
Could you please explain why this change is necessary ?
It seems to me this actually makes things a bit worse in a
multi-process environment, because the p->mutex is per process but the
dbgmgr mutex is global. Therefore, if process A first takes the
process mutex, and process B takes the dbgmgr mutex (in this function
or some other function, such as kfd_ioctl_dbg_address_watch) *before*
process A managed to take dbgmgr mutex, then process A will be locked
from doing other, totally unrelated functions, such as
kfd_ioctl_create_queue.
While, if we keep things as they are now, process A will first take
the dbgmgr mutex, making process B stuck on it, but allowing it to do
other unrelated ioctls because he hadn't taken the process mutex.
Thanks,
Oded
