Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
> Currently attach device path takes protection domain lock followed by
> dev_data lock. Most of the operations in this function is specific to
> device data except pdom_attach_iommu() where it updates protection
> domain structure. Hence reduce the scope of protection domain lock.
> 
> Note that this changes the locking order. Now it takes device lock
> before taking domain lock (group->mutex -> dev_data->lock ->
> pdom->lock). dev_data->lock is used only in device attachment path.
> So changing order is fine. It will not create any issue.
> 
> Finally move numa node assignment to pdom_attach_iommu().

numa node assignment should only be done during domain
allocation. This is important because the domain can be mapped prior
to being attached and without the right nid table levels will be mis
allocated.

However, that needs your other series, so let's just leave this as an
future direction note..

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index d74d3b65c939..a738d2d7f0c4 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2016,16 +2016,23 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
>  			     struct protection_domain *pdom)
>  {
>  	struct pdom_iommu_info *pdom_iommu_info, *curr;
> +	struct io_pgtable_cfg *cfg = &pdom->iop.pgtbl.cfg;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&pdom->lock, flags);
>  
>  	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);

It would probably make sense to use the xa_lock to protect the xa
instead of overloading the pdom->lock. Then you don't get forced into
using GFP_ATOMIC here. That could be a future direction.

Anyhow, it is already a big improvement to narrow the scope of this
lock quite a bit

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason