linux-kernel - RE: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SJ0PR11MB6744DB1AE24115FD44B390B492052@SJ0PR11MB6744.namprd11.prod.outlook.com>
Date: Wed, 18 Dec 2024 05:22:47 +0000
From: "Duan, Zhenzhong" <zhenzhong.duan@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>
CC: "dwmw2@...radead.org" <dwmw2@...radead.org>, "joro@...tes.org"
	<joro@...tes.org>, "will@...nel.org" <will@...nel.org>,
	"robin.murphy@....com" <robin.murphy@....com>, "Liu, Yi L"
	<yi.l.liu@...el.com>, "Peng, Chao P" <chao.p.peng@...el.com>
Subject: RE: [PATCH] iommu/vt-d: Link cache tags of same iommu unit together



>-----Original Message-----
>From: Baolu Lu <baolu.lu@...ux.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@...el.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);
>>