Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2/5/22 1:10 PM, Fenghua Yu wrote: Hi, Baolu, On Sat, Feb 05, 2022 at 11:50:59AM +0800, Lu Baolu wrote: Hi Fenghua, On 2022/1/29 4:28, Fenghua Yu wrote: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 92fea3fbbb11..ef03b2176bbd 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain, link_failed: spin_unlock_irqrestore(&device_domain_lock, flags); if (list_empty(&domain->subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); return ret; } @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain *domain, spin_unlock_irqrestore(&device_domain_lock, flags); if (list_empty(&domain->subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); } static int prepare_domain_attach_device(struct iommu_domain *domain, The domain->default_pasid is not relevant to SVA and it's being cleaned up by another series. No need to take care of it in this series. Because ioasid_put() is renamed to ioasid_free() in this patch, without above changes, this series cannot be compiled. Thomas and I discussed how to handle aux_domain while you will remove the entire aux_domain code (https://lore.kernel.org/lkml/87zgnf29op.ffs@tglx/). The above changes are minimal and temporary changes to compile this series. The changes will be removed along with the entire aux_domain by your removing aux_domain series later in 5.18. Okay. Make sense to me. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Baolu, On Sat, Feb 05, 2022 at 11:50:59AM +0800, Lu Baolu wrote: > Hi Fenghua, > > On 2022/1/29 4:28, Fenghua Yu wrote: > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 92fea3fbbb11..ef03b2176bbd 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain > > *domain, > > link_failed: > > spin_unlock_irqrestore(&device_domain_lock, flags); > > if (list_empty(&domain->subdevices) && domain->default_pasid > 0) > > - ioasid_put(domain->default_pasid); > > + ioasid_free(domain->default_pasid); > > return ret; > > } > > @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain > > *domain, > > spin_unlock_irqrestore(&device_domain_lock, flags); > > if (list_empty(&domain->subdevices) && domain->default_pasid > 0) > > - ioasid_put(domain->default_pasid); > > + ioasid_free(domain->default_pasid); > > } > > static int prepare_domain_attach_device(struct iommu_domain *domain, > > The domain->default_pasid is not relevant to SVA and it's being cleaned > up by another series. No need to take care of it in this series. Because ioasid_put() is renamed to ioasid_free() in this patch, without above changes, this series cannot be compiled. Thomas and I discussed how to handle aux_domain while you will remove the entire aux_domain code (https://lore.kernel.org/lkml/87zgnf29op.ffs@tglx/). The above changes are minimal and temporary changes to compile this series. The changes will be removed along with the entire aux_domain by your removing aux_domain series later in 5.18. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi Fenghua, On 2022/1/29 4:28, Fenghua Yu wrote: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 92fea3fbbb11..ef03b2176bbd 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain, link_failed: spin_unlock_irqrestore(&device_domain_lock, flags); if (list_empty(&domain->subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); return ret; } @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain *domain, spin_unlock_irqrestore(&device_domain_lock, flags); if (list_empty(&domain->subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); } static int prepare_domain_attach_device(struct iommu_domain *domain, The domain->default_pasid is not relevant to SVA and it's being cleaned up by another series. No need to take care of it in this series. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On Sat, Feb 05, 2022 at 12:56:00AM +0100, Thomas Gleixner wrote: > On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > > To avoid complexity of updating each thread's PASID status (e.g. sending > > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > > allocated and assigned to an mm, the PASID stays with the mm for the > > rest of the mm's lifetime. A reference to the PASID is taken on > > allocating the PASID. Binding/unbinding the PASID won't change refcount. > > The reference is dropped on mm exit and thus the PASID is freed. > > > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > > the PASID operations handle the pasid member in mm_struct and should be > > part of mm operations. Because IOASID's reference count is not used any > > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > > are deleted and ioasid_put() is renamed to ioasid_free(). > > > > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time. > > With cgroups and other controls that might limit the number of process > > creation, the limited number of PASIDs is not a realistic issue for > > lazy PASID free. > > Please take a step back and think hard about it whether that changelog > makes sense to you a year from now. > > Let me walk you through: > > > To avoid complexity of updating each thread's PASID status (e.g. sending > > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > > allocated and assigned to an mm, the PASID stays with the mm for the > > rest of the mm's lifetime. > > You are missing the oportunity to tell a story about the history of this > decision here: > > PASIDs are process wide. It was attempted to use refcounted PASIDs to > free them when the last thread drops the refcount. This turned out to > be complex and error prone. Given the fact that the PASID space is 20 > bits, which allows up to 1M processes to have a PASID associated > concurrently, PASID resource exhaustion is not a realistic concern. > > Therefore it was decided to simplify the approach and stick with lazy > on demand PASID allocation, but drop the eager free approach and make > a allocated PASID lifetime bound to the life time of the process. > > > A reference to the PASID is taken on allocating the > > PASID. Binding/unbinding the PASID won't change refcount. The > > reference is dropped on mm exit and thus the PASID is freed. > > There is no refcount in play anymore, right? So how does this part of > the changelog make any sense? > > This is followed by: > > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > > the PASID operations handle the pasid member in mm_struct and should be > > part of mm operations. Because IOASID's reference count is not used any > > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > > are deleted and ioasid_put() is renamed to ioasid_free(). > > which does not provide much rationale and just blurbs about _what_ the > patch is doing and not about the why and the connection to the > above. And the refcount removal section contradicts the stale text > above. > > So this paragraph should be something like this: > > Get rid of the refcounting mechanisms and replace/rename the > interfaces to reflect this new approach. > > Hmm? Sure. I will update the commit message with your comments. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > To avoid complexity of updating each thread's PASID status (e.g. sending > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > allocated and assigned to an mm, the PASID stays with the mm for the > rest of the mm's lifetime. A reference to the PASID is taken on > allocating the PASID. Binding/unbinding the PASID won't change refcount. > The reference is dropped on mm exit and thus the PASID is freed. > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > the PASID operations handle the pasid member in mm_struct and should be > part of mm operations. Because IOASID's reference count is not used any > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > are deleted and ioasid_put() is renamed to ioasid_free(). > > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time. > With cgroups and other controls that might limit the number of process > creation, the limited number of PASIDs is not a realistic issue for > lazy PASID free. Please take a step back and think hard about it whether that changelog makes sense to you a year from now. Let me walk you through: > To avoid complexity of updating each thread's PASID status (e.g. sending > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > allocated and assigned to an mm, the PASID stays with the mm for the > rest of the mm's lifetime. You are missing the oportunity to tell a story about the history of this decision here: PASIDs are process wide. It was attempted to use refcounted PASIDs to free them when the last thread drops the refcount. This turned out to be complex and error prone. Given the fact that the PASID space is 20 bits, which allows up to 1M processes to have a PASID associated concurrently, PASID resource exhaustion is not a realistic concern. Therefore it was decided to simplify the approach and stick with lazy on demand PASID allocation, but drop the eager free approach and make a allocated PASID lifetime bound to the life time of the process. > A reference to the PASID is taken on allocating the > PASID. Binding/unbinding the PASID won't change refcount. The > reference is dropped on mm exit and thus the PASID is freed. There is no refcount in play anymore, right? So how does this part of the changelog make any sense? This is followed by: > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > the PASID operations handle the pasid member in mm_struct and should be > part of mm operations. Because IOASID's reference count is not used any > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > are deleted and ioasid_put() is renamed to ioasid_free(). which does not provide much rationale and just blurbs about _what_ the patch is doing and not about the why and the connection to the above. And the refcount removal section contradicts the stale text above. So this paragraph should be something like this: Get rid of the refcounting mechanisms and replace/rename the interfaces to reflect this new approach. Hmm? Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
To avoid complexity of updating each thread's PASID status (e.g. sending
IPI to update IA32_PASID MSR) on allocating and freeing PASID, once
allocated and assigned to an mm, the PASID stays with the mm for the
rest of the mm's lifetime. A reference to the PASID is taken on
allocating the PASID. Binding/unbinding the PASID won't change refcount.
The reference is dropped on mm exit and thus the PASID is freed.
Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because
the PASID operations handle the pasid member in mm_struct and should be
part of mm operations. Because IOASID's reference count is not used any
more and removed, unused ioasid_get() and iommu_sva_free_pasid()
are deleted and ioasid_put() is renamed to ioasid_free().
20-bit PASID allows up to 1M processes bound to PASIDs at the same time.
With cgroups and other controls that might limit the number of process
creation, the limited number of PASIDs is not a realistic issue for
lazy PASID free.
Suggested-by: Dave Hansen
Signed-off-by: Fenghua Yu
Reviewed-by: Tony Luck
---
v3:
- Rename mm_pasid_get() to mm_pasid_set() (Thomas).
- Remove ioasid_get() because it's not used any more when the IOASID
is freed on mm exit (Thomas).
- Remove PASID's refcount exercise in ioasid_put() and rename
ioasid_put() to ioasid_free() (Thomas).
v2:
- Free PASID on mm exit instead of in exit(2) or unbind() (Thomas, AndyL,
PeterZ)
- Add mm_pasid_init(), mm_pasid_get(), and mm_pasid_drop() functions in mm.
So the mm's PASID operations are generic for both X86 and ARM
(Dave Hansen)
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +--
drivers/iommu/intel/iommu.c | 4 +-
drivers/iommu/intel/svm.c | 9 -
drivers/iommu/ioasid.c| 38 ++
drivers/iommu/iommu-sva-lib.c | 39 ++-
drivers/iommu/iommu-sva-lib.h | 1 -
include/linux/ioasid.h| 12 +-
include/linux/sched/mm.h | 16
kernel/fork.c | 1 +
9 files changed, 38 insertions(+), 87 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index a737ba5f727e..22ddd05bbdcd 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -340,14 +340,12 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct
*mm)
bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
if (IS_ERR(bond->smmu_mn)) {
ret = PTR_ERR(bond->smmu_mn);
- goto err_free_pasid;
+ goto err_free_bond;
}
list_add(&bond->list, &master->bonds);
return &bond->sva;
-err_free_pasid:
- iommu_sva_free_pasid(mm);
err_free_bond:
kfree(bond);
return ERR_PTR(ret);
@@ -377,7 +375,6 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
if (refcount_dec_and_test(&bond->refs)) {
list_del(&bond->list);
arm_smmu_mmu_notifier_put(bond->smmu_mn);
- iommu_sva_free_pasid(bond->mm);
kfree(bond);
}
mutex_unlock(&sva_lock);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 92fea3fbbb11..ef03b2176bbd 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
link_failed:
spin_unlock_irqrestore(&device_domain_lock, flags);
if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
- ioasid_put(domain->default_pasid);
+ ioasid_free(domain->default_pasid);
return ret;
}
@@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain
*domain,
spin_unlock_irqrestore(&device_domain_lock, flags);
if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
- ioasid_put(domain->default_pasid);
+ ioasid_free(domain->default_pasid);
}
static int prepare_domain_attach_device(struct iommu_domain *domain,
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 5b5d69b04fcc..51ac2096b3da 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -514,11 +514,6 @@ static int intel_svm_alloc_pasid(struct device *dev,
struct mm_struct *mm,
return iommu_sva_alloc_pasid(mm, PASID_MIN, max_pasid - 1);
}
-static void intel_svm_free_pasid(struct mm_struct *mm)
-{
- iommu_sva_free_pasid(mm);
-}
-
static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
struct device *dev,
struct mm_struct *mm,
@@ -662,8 +657,6 @@ static int intel_svm_unbind_mm(struct device *dev, u32
pasid)
kfree(svm);
