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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> 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?

> 
> +/* 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()

> @@ -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)) {
		...
	}

> @@ -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.

Otherwise looks good:

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>