Re: [PATCH rc 2/2] iommufd/fault: Use a separate spinlock to protect fault->deliver list

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Jan 15, 2025 at 05:24:44AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Wednesday, January 15, 2025 7:29 AM
> > 
> > @@ -445,12 +445,38 @@ struct iommufd_fault {
> > 
> >  	/* The lists of outstanding faults protected by below mutex. */
> >  	struct mutex mutex;
> > +	spinlock_t lock; /* protects the deliver list */
> >  	struct list_head deliver;
> >  	struct xarray response;
> 
> Move 'mutex' together with response then?

Ack.

> > 
> > +/* Extract the first node out of the fault->deliver list */
> > +static inline struct iopf_group *
> > +iommufd_fault_deliver_extract(struct iommufd_fault *fault)
> 
> Probably simpler be iommufd_fault_fetch()

We have deliver and response two lists. So I think that "deliver"
would be necessary. Yet, I can do "fetch" v.s. "extract".

> > @@ -102,17 +102,19 @@ static void iommufd_auto_response_faults(struct
> > iommufd_hw_pagetable *hwpt,
> >  					 struct iommufd_attach_handle
> > *handle)
> >  {
> >  	struct iommufd_fault *fault = hwpt->fault;
> > -	struct iopf_group *group, *next;
> > +	struct iopf_group *group;
> >  	unsigned long index;
> > 
> >  	if (!fault)
> >  		return;
> > 
> >  	mutex_lock(&fault->mutex);
> > -	list_for_each_entry_safe(group, next, &fault->deliver, node) {
> > -		if (group->attach_handle != &handle->handle)
> > +	for (group = iommufd_fault_deliver_extract(fault); group;
> > +	     group = iommufd_fault_deliver_extract(fault)) {
> 
> 	while (group = iommufd_fault_fetch(fault)) {
> 		...
> 	}

Ah, right...how didn't I see this lol.

> 
> > @@ -266,17 +268,20 @@ static ssize_t iommufd_fault_fops_read(struct file
> > *filep, char __user *buf,
> >  		return -ESPIPE;
> > 
> >  	mutex_lock(&fault->mutex);
> > -	while (!list_empty(&fault->deliver) && count > done) {
> > -		group = list_first_entry(&fault->deliver,
> > -					 struct iopf_group, node);
> > -
> > -		if (group->fault_count * fault_size > count - done)
> > +	for (group = iommufd_fault_deliver_extract(fault); group;
> > +	     group = iommufd_fault_deliver_extract(fault)) {
> > +		if (done >= count ||
> > +		    group->fault_count * fault_size > count - done) {
> > +			iommufd_fault_deliver_restore(fault, group);
> >  			break;
> > +		}
> > 
> >  		rc = xa_alloc(&fault->response, &group->cookie, group,
> >  			      xa_limit_32b, GFP_KERNEL);
> > -		if (rc)
> > +		if (rc) {
> > +			iommufd_fault_deliver_restore(fault, group);
> >  			break;
> > +		}
> 
> The scope of mutex can be reduced to just protect the smaller trunk
> touching fault->response.

Ack.

> Otherwise looks good:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Thanks!
Nicolin