* [PATCH] iommu/vt-d: Link cache tags of same iommu unit together @ 2024-12-16 3:38 Zhenzhong Duan 2024-12-18 3:08 ` Baolu Lu 0 siblings, 1 reply; 5+ messages in thread From: Zhenzhong Duan @ 2024-12-16 3:38 UTC (permalink / raw) To: linux-kernel, iommu Cc: dwmw2, baolu.lu, joro, will, robin.murphy, yi.l.liu, chao.p.peng, Zhenzhong Duan Cache tag invalidation requests for a domain are accumulated until a different iommu unit is found when traversing the cache_tags linked list. But cache tags of same iommu unit can be distributed in the linked list, this make batched flush less efficient. E.g., one device backed by iommu0 is attached to a domain in between two devices attaching backed by iommu1. Group cache tags together for same iommu unit in cache_tag_assign() to maximize the performance of batched flush. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- drivers/iommu/intel/cache.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c index e5b89f728ad3..726052a841e0 100644 --- a/drivers/iommu/intel/cache.c +++ b/drivers/iommu/intel/cache.c @@ -48,6 +48,8 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, struct intel_iommu *iommu = info->iommu; struct cache_tag *tag, *temp; unsigned long flags; + struct cache_tag *temp2 = list_entry(&domain->cache_tags, + struct cache_tag, node); tag = kzalloc(sizeof(*tag), GFP_KERNEL); if (!tag) @@ -73,8 +75,15 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, trace_cache_tag_assign(temp); return 0; } + if (temp->iommu == iommu) + temp2 = temp; } - list_add_tail(&tag->node, &domain->cache_tags); + /* + * Link cache tags of same iommu unit together, so consponding + * flush ops can be batched for iommu unit. + */ + list_add(&tag->node, &temp2->node); + spin_unlock_irqrestore(&domain->cache_lock, flags); trace_cache_tag_assign(tag); -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together 2024-12-16 3:38 [PATCH] iommu/vt-d: Link cache tags of same iommu unit together Zhenzhong Duan @ 2024-12-18 3:08 ` Baolu Lu 2024-12-18 5:22 ` Duan, Zhenzhong 0 siblings, 1 reply; 5+ messages in thread From: Baolu Lu @ 2024-12-18 3:08 UTC (permalink / raw) To: Zhenzhong Duan, linux-kernel, iommu Cc: dwmw2, joro, will, robin.murphy, yi.l.liu, chao.p.peng On 12/16/24 11:38, Zhenzhong Duan wrote: > Cache tag invalidation requests for a domain are accumulated until a > different iommu unit is found when traversing the cache_tags linked list. > But cache tags of same iommu unit can be distributed in the linked list, > this make batched flush less efficient. E.g., one device backed by iommu0 > is attached to a domain in between two devices attaching backed by iommu1. > > Group cache tags together for same iommu unit in cache_tag_assign() to > maximize the performance of batched flush. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > drivers/iommu/intel/cache.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c > index e5b89f728ad3..726052a841e0 100644 > --- a/drivers/iommu/intel/cache.c > +++ b/drivers/iommu/intel/cache.c > @@ -48,6 +48,8 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, > struct intel_iommu *iommu = info->iommu; > struct cache_tag *tag, *temp; > unsigned long flags; > + struct cache_tag *temp2 = list_entry(&domain->cache_tags, > + struct cache_tag, node); Is this valid for a list head? > > tag = kzalloc(sizeof(*tag), GFP_KERNEL); > if (!tag) > @@ -73,8 +75,15 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, > trace_cache_tag_assign(temp); > return 0; > } > + if (temp->iommu == iommu) > + temp2 = temp; > } > - list_add_tail(&tag->node, &domain->cache_tags); > + /* > + * Link cache tags of same iommu unit together, so consponding > + * flush ops can be batched for iommu unit. > + */ > + list_add(&tag->node, &temp2->node); > + > spin_unlock_irqrestore(&domain->cache_lock, flags); > trace_cache_tag_assign(tag); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together 2024-12-18 3:08 ` Baolu Lu @ 2024-12-18 5:22 ` Duan, Zhenzhong 2024-12-18 9:06 ` Baolu Lu 0 siblings, 1 reply; 5+ messages in thread From: Duan, Zhenzhong @ 2024-12-18 5:22 UTC (permalink / raw) To: Baolu Lu, linux-kernel@vger.kernel.org, iommu@lists.linux.dev Cc: dwmw2@infradead.org, joro@8bytes.org, will@kernel.org, robin.murphy@arm.com, Liu, Yi L, Peng, Chao P >-----Original Message----- >From: Baolu Lu <baolu.lu@linux.intel.com> >Subject: Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together > >On 12/16/24 11:38, Zhenzhong Duan wrote: >> Cache tag invalidation requests for a domain are accumulated until a >> different iommu unit is found when traversing the cache_tags linked list. >> But cache tags of same iommu unit can be distributed in the linked list, >> this make batched flush less efficient. E.g., one device backed by iommu0 >> is attached to a domain in between two devices attaching backed by iommu1. >> >> Group cache tags together for same iommu unit in cache_tag_assign() to >> maximize the performance of batched flush. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> drivers/iommu/intel/cache.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c >> index e5b89f728ad3..726052a841e0 100644 >> --- a/drivers/iommu/intel/cache.c >> +++ b/drivers/iommu/intel/cache.c >> @@ -48,6 +48,8 @@ static int cache_tag_assign(struct dmar_domain *domain, >u16 did, >> struct intel_iommu *iommu = info->iommu; >> struct cache_tag *tag, *temp; >> unsigned long flags; >> + struct cache_tag *temp2 = list_entry(&domain->cache_tags, >> + struct cache_tag, node); > >Is this valid for a list head? Yes, it's not valid for list head but it's intentional, just want to avoid unnecessary temp2 check. If I don't do that way, patch will be: --- a/drivers/iommu/intel/cache.c +++ b/drivers/iommu/intel/cache.c @@ -48,6 +48,7 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, struct intel_iommu *iommu = info->iommu; struct cache_tag *tag, *temp; unsigned long flags; + struct cache_tag *temp2 = NULL; tag = kzalloc(sizeof(*tag), GFP_KERNEL); if (!tag) @@ -73,8 +74,18 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, trace_cache_tag_assign(temp); return 0; } + if (temp->iommu == iommu) + temp2 = temp; } - list_add_tail(&tag->node, &domain->cache_tags); + /* + * Link cache tags of same iommu unit together, so consponding + * flush ops can be batched for iommu unit. + */ + if (temp2) + list_add(&tag->node, &temp2->node); + else + list_add_tail((&tag->node, &domain->cache_tags); + spin_unlock_irqrestore(&domain->cache_lock, flags); trace_cache_tag_assign(tag); Thanks Zhenzhong > >> >> tag = kzalloc(sizeof(*tag), GFP_KERNEL); >> if (!tag) >> @@ -73,8 +75,15 @@ static int cache_tag_assign(struct dmar_domain >*domain, u16 did, >> trace_cache_tag_assign(temp); >> return 0; >> } >> + if (temp->iommu == iommu) >> + temp2 = temp; >> } >> - list_add_tail(&tag->node, &domain->cache_tags); >> + /* >> + * Link cache tags of same iommu unit together, so consponding >> + * flush ops can be batched for iommu unit. >> + */ >> + list_add(&tag->node, &temp2->node); >> + >> spin_unlock_irqrestore(&domain->cache_lock, flags); >> trace_cache_tag_assign(tag); >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together 2024-12-18 5:22 ` Duan, Zhenzhong @ 2024-12-18 9:06 ` Baolu Lu 2024-12-18 9:12 ` Duan, Zhenzhong 0 siblings, 1 reply; 5+ messages in thread From: Baolu Lu @ 2024-12-18 9:06 UTC (permalink / raw) To: Duan, Zhenzhong, linux-kernel@vger.kernel.org, iommu@lists.linux.dev Cc: baolu.lu, dwmw2@infradead.org, joro@8bytes.org, will@kernel.org, robin.murphy@arm.com, Liu, Yi L, Peng, Chao P On 2024/12/18 13:22, Duan, Zhenzhong wrote: >> -----Original Message----- >> From: Baolu Lu<baolu.lu@linux.intel.com> >> Subject: Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together >> >> On 12/16/24 11:38, Zhenzhong Duan wrote: >>> Cache tag invalidation requests for a domain are accumulated until a >>> different iommu unit is found when traversing the cache_tags linked list. >>> But cache tags of same iommu unit can be distributed in the linked list, >>> this make batched flush less efficient. E.g., one device backed by iommu0 >>> is attached to a domain in between two devices attaching backed by iommu1. >>> >>> Group cache tags together for same iommu unit in cache_tag_assign() to >>> maximize the performance of batched flush. >>> >>> Signed-off-by: Zhenzhong Duan<zhenzhong.duan@intel.com> >>> --- >>> drivers/iommu/intel/cache.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c >>> index e5b89f728ad3..726052a841e0 100644 >>> --- a/drivers/iommu/intel/cache.c >>> +++ b/drivers/iommu/intel/cache.c >>> @@ -48,6 +48,8 @@ static int cache_tag_assign(struct dmar_domain *domain, >> u16 did, >>> struct intel_iommu *iommu = info->iommu; >>> struct cache_tag *tag, *temp; >>> unsigned long flags; >>> + struct cache_tag *temp2 = list_entry(&domain->cache_tags, >>> + struct cache_tag, node); >> Is this valid for a list head? > Yes, it's not valid for list head but it's intentional, just want to > avoid unnecessary temp2 check. If I don't do that way, patch will be: > > --- a/drivers/iommu/intel/cache.c > +++ b/drivers/iommu/intel/cache.c > @@ -48,6 +48,7 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, > struct intel_iommu *iommu = info->iommu; > struct cache_tag *tag, *temp; > unsigned long flags; > + struct cache_tag *temp2 = NULL; > > tag = kzalloc(sizeof(*tag), GFP_KERNEL); > if (!tag) > @@ -73,8 +74,18 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, > trace_cache_tag_assign(temp); > return 0; > } > + if (temp->iommu == iommu) > + temp2 = temp; > } > - list_add_tail(&tag->node, &domain->cache_tags); > + /* > + * Link cache tags of same iommu unit together, so consponding > + * flush ops can be batched for iommu unit. > + */ > + if (temp2) > + list_add(&tag->node, &temp2->node); > + else > + list_add_tail((&tag->node, &domain->cache_tags); > + > spin_unlock_irqrestore(&domain->cache_lock, flags); > trace_cache_tag_assign(tag); Perhaps we can make it like this? diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c index 09694cca8752..cf0cca94d165 100644 --- a/drivers/iommu/intel/cache.c +++ b/drivers/iommu/intel/cache.c @@ -47,6 +47,7 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, struct device_domain_info *info = dev_iommu_priv_get(dev); struct intel_iommu *iommu = info->iommu; struct cache_tag *tag, *temp; + struct list_head *prev; unsigned long flags; tag = kzalloc(sizeof(*tag), GFP_KERNEL); @@ -65,6 +66,7 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, tag->dev = iommu->iommu.dev; spin_lock_irqsave(&domain->cache_lock, flags); + prev = &domain->cache_tags; list_for_each_entry(temp, &domain->cache_tags, node) { if (cache_tage_match(temp, did, iommu, dev, pasid, type)) { temp->users++; @@ -73,8 +75,15 @@ static int cache_tag_assign(struct dmar_domain *domain, u16 did, trace_cache_tag_assign(temp); return 0; } + if (temp->iommu == iommu) + prev = &temp->node; } - list_add_tail(&tag->node, &domain->cache_tags); + /* + * Link cache tags of same iommu unit together, so consponding + * flush ops can be batched for iommu unit. + */ + list_add(&tag->node, prev); + spin_unlock_irqrestore(&domain->cache_lock, flags); trace_cache_tag_assign(tag); Thanks, baolu ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together 2024-12-18 9:06 ` Baolu Lu @ 2024-12-18 9:12 ` Duan, Zhenzhong 0 siblings, 0 replies; 5+ messages in thread From: Duan, Zhenzhong @ 2024-12-18 9:12 UTC (permalink / raw) To: Baolu Lu, linux-kernel@vger.kernel.org, iommu@lists.linux.dev Cc: dwmw2@infradead.org, joro@8bytes.org, will@kernel.org, robin.murphy@arm.com, Liu, Yi L, Peng, Chao P >-----Original Message----- >From: Baolu Lu <baolu.lu@linux.intel.com> >Subject: Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together > >On 2024/12/18 13:22, Duan, Zhenzhong wrote: >>> -----Original Message----- >>> From: Baolu Lu<baolu.lu@linux.intel.com> >>> Subject: Re: [PATCH] iommu/vt-d: Link cache tags of same iommu unit >together >>> >>> On 12/16/24 11:38, Zhenzhong Duan wrote: >>>> Cache tag invalidation requests for a domain are accumulated until a >>>> different iommu unit is found when traversing the cache_tags linked list. >>>> But cache tags of same iommu unit can be distributed in the linked list, >>>> this make batched flush less efficient. E.g., one device backed by iommu0 >>>> is attached to a domain in between two devices attaching backed by iommu1. >>>> >>>> Group cache tags together for same iommu unit in cache_tag_assign() to >>>> maximize the performance of batched flush. >>>> >>>> Signed-off-by: Zhenzhong Duan<zhenzhong.duan@intel.com> >>>> --- >>>> drivers/iommu/intel/cache.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c >>>> index e5b89f728ad3..726052a841e0 100644 >>>> --- a/drivers/iommu/intel/cache.c >>>> +++ b/drivers/iommu/intel/cache.c >>>> @@ -48,6 +48,8 @@ static int cache_tag_assign(struct dmar_domain >*domain, >>> u16 did, >>>> struct intel_iommu *iommu = info->iommu; >>>> struct cache_tag *tag, *temp; >>>> unsigned long flags; >>>> + struct cache_tag *temp2 = list_entry(&domain->cache_tags, >>>> + struct cache_tag, node); >>> Is this valid for a list head? >> Yes, it's not valid for list head but it's intentional, just want to >> avoid unnecessary temp2 check. If I don't do that way, patch will be: >> >> --- a/drivers/iommu/intel/cache.c >> +++ b/drivers/iommu/intel/cache.c >> @@ -48,6 +48,7 @@ static int cache_tag_assign(struct dmar_domain *domain, >u16 did, >> struct intel_iommu *iommu = info->iommu; >> struct cache_tag *tag, *temp; >> unsigned long flags; >> + struct cache_tag *temp2 = NULL; >> >> tag = kzalloc(sizeof(*tag), GFP_KERNEL); >> if (!tag) >> @@ -73,8 +74,18 @@ static int cache_tag_assign(struct dmar_domain >*domain, u16 did, >> trace_cache_tag_assign(temp); >> return 0; >> } >> + if (temp->iommu == iommu) >> + temp2 = temp; >> } >> - list_add_tail(&tag->node, &domain->cache_tags); >> + /* >> + * Link cache tags of same iommu unit together, so consponding >> + * flush ops can be batched for iommu unit. >> + */ >> + if (temp2) >> + list_add(&tag->node, &temp2->node); >> + else >> + list_add_tail((&tag->node, &domain->cache_tags); >> + >> spin_unlock_irqrestore(&domain->cache_lock, flags); >> trace_cache_tag_assign(tag); > >Perhaps we can make it like this? Good suggestion! Will do. Thanks Zhenzhong > >diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c >index 09694cca8752..cf0cca94d165 100644 >--- a/drivers/iommu/intel/cache.c >+++ b/drivers/iommu/intel/cache.c >@@ -47,6 +47,7 @@ static int cache_tag_assign(struct dmar_domain >*domain, u16 did, > struct device_domain_info *info = dev_iommu_priv_get(dev); > struct intel_iommu *iommu = info->iommu; > struct cache_tag *tag, *temp; >+ struct list_head *prev; > unsigned long flags; > > tag = kzalloc(sizeof(*tag), GFP_KERNEL); >@@ -65,6 +66,7 @@ static int cache_tag_assign(struct dmar_domain >*domain, u16 did, > tag->dev = iommu->iommu.dev; > > spin_lock_irqsave(&domain->cache_lock, flags); >+ prev = &domain->cache_tags; > list_for_each_entry(temp, &domain->cache_tags, node) { > if (cache_tage_match(temp, did, iommu, dev, pasid, type)) { > temp->users++; >@@ -73,8 +75,15 @@ static int cache_tag_assign(struct dmar_domain >*domain, u16 did, > trace_cache_tag_assign(temp); > return 0; > } >+ if (temp->iommu == iommu) >+ prev = &temp->node; > } >- list_add_tail(&tag->node, &domain->cache_tags); >+ /* >+ * Link cache tags of same iommu unit together, so consponding >+ * flush ops can be batched for iommu unit. >+ */ >+ list_add(&tag->node, prev); >+ > spin_unlock_irqrestore(&domain->cache_lock, flags); > trace_cache_tag_assign(tag); > >Thanks, >baolu ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-18 9:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-16 3:38 [PATCH] iommu/vt-d: Link cache tags of same iommu unit together Zhenzhong Duan 2024-12-18 3:08 ` Baolu Lu 2024-12-18 5:22 ` Duan, Zhenzhong 2024-12-18 9:06 ` Baolu Lu 2024-12-18 9:12 ` Duan, Zhenzhong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.