Messages in this thread Patch in this message | | | Date | Fri, 1 Jul 2022 10:35:50 +0800 | Subject | Re: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each domain | From | Lu Baolu <> |
| |
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
| |