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

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 10/17/2024 7:02 PM, Jason Gunthorpe wrote:
> 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..

Correct. Once these two series gets merged will remove this check.

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

Right. We can do follow up series.

> 
> Anyhow, it is already a big improvement to narrow the scope of this
> lock quite a bit
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks
-Vasant