Re: [PATCH 1/4] drm/sched: optimize drm_sched_job_add_dependency

["Christian König" <christian.koenig@amd.com>]

On 5/24/25 13:17, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 04:11:39PM +0200, Danilo Krummrich wrote:
>> On Fri, May 23, 2025 at 02:56:40PM +0200, Christian König wrote:
>>> +	if (xas_nomem(&xas, GFP_KERNEL)) {
>>> +		xa_lock(&job->dependencies);
>>> +		goto retry;
>>
>> Please don't use a goto here, if we would have failed to allocate memory here,
>> this would be an endless loop until we succeed eventually.
> 
> I think I got confused by xas_nomem() returning true meaning that memory was
> needed and was successfully allocated (which can be a bit counterintuitive).


Yeah, I had to take a look at the samples/tests to understand that as well.

> 
> So, your code here should be correct. However, I'd still remove the goto and
> just call xas_store() again. There's no reason to make this a loop and backwards
> goto is better avoided anyways. :)


I was considering that as well, but than abandoned this idea. The xarray() sample code and test cases as well as the use cases where I took a look either use a loop or a goto.

I'm not 100% sure why, my suspicion is that you need the loop when there can be concurrent add/remove operations on the xarray, but I think we should stick with the common approach.

Regards,
Christian.