Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <595c314b-2d16-aeef-d808-d1cb713ba175@linux.intel.com>
Date:   Fri, 1 Jul 2022 10:35:50 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Cc:     baolu.lu@linux.intel.com, David Woodhouse <dwmw2@infradead.org>,
        Jerry Snitselaar <jsnitsel@redhat.com>,
        Mike Travis <mike.travis@hpe.com>,
        Dimitri Sivanich <sivanich@hpe.com>,
        "Anderson, Russ" <russ.anderson@hpe.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each
 domain
Content-Language: en-US
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        Joerg Roedel <joro@8bytes.org>, Steve Wahl <steve.wahl@hpe.com>
References: <20220625125204.2199437-1-baolu.lu@linux.intel.com>
 <20220625125204.2199437-4-baolu.lu@linux.intel.com>
 <BN9PR11MB5276A78546BD2B38068385E18CBA9@BN9PR11MB5276.namprd11.prod.outlook.com>
From:   Lu Baolu <baolu.lu@linux.intel.com>
In-Reply-To: <BN9PR11MB5276A78546BD2B38068385E18CBA9@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/30/22 4:28 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Saturday, June 25, 2022 8:52 PM
>>
>> +struct iommu_domain_info {
>> +	struct intel_iommu *iommu;
>> +	unsigned int refcnt;
>> +	u16 did;
>> +};
>> +
>>   struct dmar_domain {
>>   	int	nid;			/* node id */
>> -
>> -	unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
>> -					/* Refcount of devices per iommu */
>> -
>> -
>> -	u16		iommu_did[DMAR_UNITS_SUPPORTED];
>> -					/* Domain ids per IOMMU. Use u16
>> since
>> -					 * domain ids are 16 bit wide
>> according
>> -					 * to VT-d spec, section 9.3 */
> 
> It'd make sense to keep the comments when moving above fields.

Sure. Updated.

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 56c3d1a9e155..fae45bbb0c7f 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -527,8 +527,10 @@ struct context_entry {

  struct iommu_domain_info {
         struct intel_iommu *iommu;
-       unsigned int refcnt;
-       u16 did;
+       unsigned int refcnt;            /* Refcount of devices per iommu */
+       u16 did;                        /* Domain ids per IOMMU. Use u16 
since
+                                        * domain ids are 16 bit wide 
according
+                                        * to VT-d spec, section 9.3 */
  };


> 
>> +	spin_lock(&iommu->lock);
>> +	curr = xa_load(&domain->iommu_array, iommu->seq_id);
>> +	if (curr) {
>> +		curr->refcnt++;
>> +		kfree(info);
>> +		goto success;
> 
> Not a fan of adding a label for success. Just putting two lines (unlock+
> return) here is clearer.

Updated.

> 
>> +	ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> +			      info, GFP_ATOMIC));
> 
> check xa_err in separate line.

Sure. Updated as below:

@@ -1778,13 +1780,14 @@ static int domain_attach_iommu(struct 
dmar_domain *domain,
         info->did       = num;
         info->iommu     = iommu;
         domain->nid     = iommu->node;
-       ret = xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
-                             info, GFP_ATOMIC));
-       if (ret)
+       curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
+                         NULL, info, GFP_ATOMIC);
+       if (curr) {
+               ret = xa_err(curr) ? : -EBUSY;
                 goto err_clear;
+       }
         domain_update_iommu_cap(domain);

> 
>>
>>   static void domain_detach_iommu(struct dmar_domain *domain,
>>   				struct intel_iommu *iommu)
>>   {
>> -	int num;
>> +	struct iommu_domain_info *info;
>>
>>   	spin_lock(&iommu->lock);
>> -	domain->iommu_refcnt[iommu->seq_id] -= 1;
>> -	if (domain->iommu_refcnt[iommu->seq_id] == 0) {
>> -		num = domain->iommu_did[iommu->seq_id];
>> -		clear_bit(num, iommu->domain_ids);
>> +	info = xa_load(&domain->iommu_array, iommu->seq_id);
>> +	if (--info->refcnt == 0) {
>> +		clear_bit(info->did, iommu->domain_ids);
>> +		xa_erase(&domain->iommu_array, iommu->seq_id);
>>   		domain_update_iommu_cap(domain);
>> -		domain->iommu_did[iommu->seq_id] = 0;
>> +		kfree(info);
> 
> domain->nid may still point to the node of the removed iommu.

Good catch! I should assign it to NUMA_NO_NODE, so that it could be
updated in the next domain_update_iommu_cap(). Updated as below:

@@ -1806,6 +1809,7 @@ static void domain_detach_iommu(struct dmar_domain 
*domain,
         if (--info->refcnt == 0) {
                 clear_bit(info->did, iommu->domain_ids);
                 xa_erase(&domain->iommu_array, iommu->seq_id);
+               domain->nid = NUMA_NO_NODE;
                 domain_update_iommu_cap(domain);
                 kfree(info);
         }

Best regards,
baolu