Re: [PATCH 6/6] drm/amdkfd: Fix sdma queue allocate race condition

["Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>]

Patches 5 and 6 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

On 2019-06-06 2:25 p.m., Zeng, Oak wrote:
> SDMA queue allocation requires the dqm lock at it modify
> the global dqm members. Move up the dqm_lock so sdma
> queue allocation is enclosed in the critical section. Move
> mqd allocation out of critical section to avoid circular
> lock dependency.
>
> Change-Id: I96abd42eae6e77c82a5ba1b8e600af3efe8d791d
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  | 24 +++++++++++-----------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 166636c..cd259b8 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -1133,23 +1133,27 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q,
>   	if (dqm->total_queue_count >= max_num_of_queues_per_device) {
>   		pr_warn("Can't create new usermode queue because %d queues were already created\n",
>   				dqm->total_queue_count);
> -		retval = -EPERM;
> -		goto out;
> +		return -EPERM;
>   	}
>   
> +	mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
> +			q->properties.type)];
> +	q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> +	if (!q->mqd_mem_obj)
> +		return -ENOMEM;
> +
> +	dqm_lock(dqm);
>   	if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
>   		q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
>   		retval = allocate_sdma_queue(dqm, q);
>   		if (retval)
> -			goto out;
> +			goto out_unlock;
>   	}
>   
>   	retval = allocate_doorbell(qpd, q);
>   	if (retval)
>   		goto out_deallocate_sdma_queue;
>   
> -	mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
> -			q->properties.type)];
>   	/*
>   	 * Eviction state logic: mark all queues as evicted, even ones
>   	 * not currently active. Restoring inactive queues later only
> @@ -1161,12 +1165,8 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q,
>   		dqm->asic_ops.init_sdma_vm(dqm, q, qpd);
>   	q->properties.tba_addr = qpd->tba_addr;
>   	q->properties.tma_addr = qpd->tma_addr;
> -	q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> -	if (!q->mqd_mem_obj)
> -		goto out_deallocate_doorbell;
>   	mqd_mgr->init_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj,
>   				&q->gart_mqd_addr, &q->properties);
> -	dqm_lock(dqm);
>   
>   	list_add(&q->list, &qpd->queues_list);
>   	qpd->queue_count++;
> @@ -1192,13 +1192,13 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q,
>   	dqm_unlock(dqm);
>   	return retval;
>   
> -out_deallocate_doorbell:
> -	deallocate_doorbell(qpd, q);
>   out_deallocate_sdma_queue:
>   	if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
>   		q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)
>   		deallocate_sdma_queue(dqm, q);
> -out:
> +out_unlock:
> +	dqm_unlock(dqm);
> +	mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
>   	return retval;
>   }
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx