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

[Philipp Stanner <phasta@mailbox.org>]

On Mon, 2025-05-26 at 11:25 +0200, Christian König wrote:
> On 5/23/25 16:16, 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:
> > > > It turned out that we can actually massively optimize here.
> > > > 
> > > > The previous code was horrible inefficient since it constantly
> > > > released
> > > > and re-acquired the lock of the xarray and started each
> > > > iteration from the
> > > > base of the array to avoid concurrent modification which in our
> > > > case
> > > > doesn't exist.
> > > > 
> > > > Additional to that the xas_find() and xas_store() functions are
> > > > explicitly
> > > > made in a way so that you can efficiently check entries and if
> > > > you don't
> > > > find a match store a new one at the end or replace existing
> > > > ones.
> > > > 
> > > > So use xas_for_each()/xa_store() instead of
> > > > xa_for_each()/xa_alloc().
> > > > It's a bit more code, but should be much faster in the end.
> > > 
> > > This commit message does neither explain the motivation of the
> > > commit nor what it
> > > does. It describes what instead belongs into the changelog
> > > between versions.
> > 
> > Sorry, this is wrong. I got confused, the commit message is
> > perfectly fine. :)
> > 
> > The rest still applies though.
> > 
> > > Speaking of versioning of the patch series, AFAIK there were
> > > previous versions,
> > > but this series was sent as a whole new series -- why?
> > > 
> > > Please resend with a proper commit message, version and
> > > changelog. Thanks!
> 
> 
> Well Philip asked to remove the changelog. I'm happy to bring it
> back, but yeah...

No no no no :D

Philipp asked for the changelog to be removed *from the git commit
message*; because it doesn't belong / isn't useful there.

If there's a cover letter, the changelog should be in the cover letter.
If there's no cover letter, it should be between the --- separators:


Signed-off-by: Gordon Freeman <freeman@blackmesa.org>
Reviewed-by: Alyx Vance <alyx@vance.edu>
---
Changes in v2:
  - Provide more docu for crowbar-alloc-function.
  - Use NULL pointers for reserved xarray entries
---
<DIFF>


P.


> 
> Regards,
> Christian.
> 
> > > 
> > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > ---
> > > >  drivers/gpu/drm/scheduler/sched_main.c | 29
> > > > ++++++++++++++++++--------
> > > >  1 file changed, 20 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index f7118497e47a..cf200b1b643e 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -871,10 +871,8 @@ EXPORT_SYMBOL(drm_sched_job_arm);
> > > >  int drm_sched_job_add_dependency(struct drm_sched_job *job,
> > > >  				 struct dma_fence *fence)
> > > >  {
> > > > +	XA_STATE(xas, &job->dependencies, 0);
> > > >  	struct dma_fence *entry;
> > > > -	unsigned long index;
> > > > -	u32 id = 0;
> > > > -	int ret;
> > > >  
> > > >  	if (!fence)
> > > >  		return 0;
> > > > @@ -883,24 +881,37 @@ int drm_sched_job_add_dependency(struct
> > > > drm_sched_job *job,
> > > >  	 * This lets the size of the array of deps scale with
> > > > the number of
> > > >  	 * engines involved, rather than the number of BOs.
> > > >  	 */
> > > > -	xa_for_each(&job->dependencies, index, entry) {
> > > > +	xa_lock(&job->dependencies);
> > > > +	xas_for_each(&xas, entry, ULONG_MAX) {
> > > >  		if (entry->context != fence->context)
> > > >  			continue;
> > > >  
> > > >  		if (dma_fence_is_later(fence, entry)) {
> > > >  			dma_fence_put(entry);
> > > > -			xa_store(&job->dependencies, index,
> > > > fence, GFP_KERNEL);
> > > > +			xas_store(&xas, fence);
> > > >  		} else {
> > > >  			dma_fence_put(fence);
> > > >  		}
> > > > -		return 0;
> > > > +		xa_unlock(&job->dependencies);
> > > > +		return xas_error(&xas);
> > > >  	}
> > > >  
> > > > -	ret = xa_alloc(&job->dependencies, &id, fence,
> > > > xa_limit_32b, GFP_KERNEL);
> > > > -	if (ret != 0)
> > > > +retry:
> > > > +	entry = xas_store(&xas, fence);
> > > > +	xa_unlock(&job->dependencies);
> > > > +
> > > > +	/* There shouldn't be any concurrent add, so no need
> > > > to loop again */
> > > 
> > > Concurrency shouldn't matter, xas_nomem() stores the pre-
> > > allocated memory in the
> > > XA_STATE not the xarray. Hence, I think we should remove the
> > > comment.
> > > 
> > > > +	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. It
> > > would be equal to:
> > > 
> > > 	while (!ptr) {
> > > 		ptr = kmalloc();
> > > 	}
> > > 
> > > Instead just take the lock and call xas_store() again.
> > > 
> > > > +	}
> > > > +
> > > > +	if (xas_error(&xas))
> > > >  		dma_fence_put(fence);
> > > > +	else
> > > > +		WARN_ON(entry);
> > > 
> > > Please don't call WARN_ON() here, this isn't fatal, we only need
> > > to return the
> > > error code.
>