Re: [PATCH v2] drm/xe: Remove unwanted mutex locking

[Matthew Brost <matthew.brost@intel.com>]

On Wed, May 29, 2024 at 04:49:19PM -0400, Rodrigo Vivi wrote:
> On Wed, May 29, 2024 at 11:33:19AM -0700, Niranjana Vishwanathapura wrote:
> > Do not hold xef->exec_queue.lock mutex while parsing the xarray
> > xef->exec_queue.xa in xe_file_close() as it is not needed and
> > will cause an unwanted dependency between this lock and the vm->lock.
> > 
> > This lock protects the exec queue lookup and reference taking which
> > doesn't apply to this code path. When FD is closing, IOCTLs presumably
> > can't be modifying the xarray.
> > 
> > v2: Update commit text
> > 
> > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_device.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index f04b11e45c2d..4cca16f2d4ed 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -96,12 +96,11 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
> >  	struct xe_exec_queue *q;
> >  	unsigned long idx;
> >  
> > -	mutex_lock(&xef->exec_queue.lock);
> > +	/* No locking needed here */
> 
> could you please expand this message here to avoid git-blame+git-show
> when trying to understand 'why not?'!?
> 

Good catch and agree. Frequently look at code which git blame fails as
code is often moved around to a different file / location and then
finding the original commit message with reasoning is hard.

Matt 

> Thanks,
> Rodrigo.
> 
> >  	xa_for_each(&xef->exec_queue.xa, idx, q) {
> >  		xe_exec_queue_kill(q);
> >  		xe_exec_queue_put(q);
> >  	}
> > -	mutex_unlock(&xef->exec_queue.lock);
> >  	xa_destroy(&xef->exec_queue.xa);
> >  	mutex_destroy(&xef->exec_queue.lock);
> >  	mutex_lock(&xef->vm.lock);
> > -- 
> > 2.43.0
> >