Re: [PATCH] iommu: narrow the search range by iterating on current bus

[Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Ping~ Hope you like it :-)

On Thu, Aug 18, 2016 at 11:00:26PM +0000, Wei Yang wrote:
>Hi, Alex & Bjorn,
>
>I tried to measure the effect of the improvement and looks good.
>
>    +---------------+-----------+-----------+-----------+
>    |Platform Info  |Before(ns) |After(ns)  |Improvement|
>    +---------------+-----------+-----------+-----------+
>    |76 pci devices |1846600    |100289     |94.5%      |
>    +---------------+-----------+-----------+-----------+
>    |16 pci devices |46931      |23189      |53.0%      |
>    +---------------+-----------+-----------+-----------+
>
>Below is my raw result and the code to measure the time used. Willing to hear
>from you :-)
>
>Raw Data collected on a machine with 32 pci devices in the system.
>==================
>total time 46931 with 32 devices 1466.59 each
>total time 23189 with 32 devices 724.656 each
>
>Raw Data collected on a machine with 76 pci devices in the system.
>==================
>total time 1846600 with 152 devices 12148.7 each
>total time 100289 with 152 devices 659.796 each
>
>Script and diff to kernel to extract the raw data.
>==================
>awk ' {x += $9; y++} END {print "total time " x " with " y " devices " x/y " each"}' narrow.orig 
>
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index d420d94d..63cda3d 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -743,6 +743,7 @@ struct iommu_group *pci_device_group(struct device *dev)
> 	struct pci_bus *bus;
> 	struct iommu_group *group = NULL;
> 	u64 devfns[4] = { 0 };
>+	struct timespec ts1, ts2;
> 
> 	if (WARN_ON(!dev_is_pci(dev)))
> 		return ERR_PTR(-EINVAL);
>@@ -782,7 +783,11 @@ struct iommu_group *pci_device_group(struct device *dev)
> 	 * Look for existing groups on device aliases.  If we alias another
> 	 * device or another device aliases us, use the same group.
> 	 */
>+	getnstimeofday(&ts1);
> 	group = get_pci_alias_group(pdev, (unsigned long *)devfns);
>+	getnstimeofday(&ts2);
>+	dev_info(dev, "ywtest: %s: get_pci_alias_group			spend %lu sec %lu nsec\n",
>+			__func__, ts2.tv_sec - ts1.tv_sec, ts2.tv_nsec - ts1.tv_nsec);
> 	if (group)
> 		return group;
> 
>@@ -791,7 +796,11 @@ struct iommu_group *pci_device_group(struct device *dev)
> 	 * slot and aliases of those functions, if any. No need to clear
> 	 * the search bitmap, the tested devfns are still valid.
> 	 */
>+	getnstimeofday(&ts1);
> 	group = get_pci_function_alias_group(pdev, (unsigned long *)devfns);
>+	getnstimeofday(&ts2);
>+	dev_info(dev, "ywtest: %s: get_pci_function_alias_group		spend %lu sec %lu nsec\n",
>+			__func__, ts2.tv_sec - ts1.tv_sec, ts2.tv_nsec - ts1.tv_nsec);
> 	if (group)
> 		return group;
> 
>On Fri, Aug 05, 2016 at 02:03:17PM +0000, Wei Yang wrote:
>>On Thu, Jul 28, 2016 at 09:01:32AM -0600, Alex Williamson wrote:
>>>On Thu, 28 Jul 2016 14:52:59 +0000
>>>Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> According to the comments and code, get_pci_function_alias_group() and
>>>> get_pci_alias_group() do the search on the same pci bus. This means we can
>>>> just iterate on the bus the pci device attaches to.
>>>> 
>>>> This patch narrows the search range by just iterating on the current bus
>>>> and fix one typo in comment.
>>>> 
>>>> Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  drivers/iommu/iommu.c |   22 ++++++++--------------
>>>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>>> 
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 3000051..0338a81 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -643,17 +643,15 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
>>>>  	if (!pdev->multifunction || pci_acs_enabled(pdev, REQ_ACS_FLAGS))
>>>>  		return NULL;
>>>>  
>>>> -	for_each_pci_dev(tmp) {
>>>> -		if (tmp == pdev || tmp->bus != pdev->bus ||
>>>> +	list_for_each_entry(tmp, &pdev->bus->devices, bus_list) {
>>>
>>>I believe this was using for_each_pci_dev() exactly because it takes a
>>>reference to the device and therefore we can use it to get a reference
>>>to the group.  How do you justify removing these reference semantics?
>>>Thanks,
>>>
>>
>>Alex,
>>
>>With some investigation, I believe these reference are important. Thanks for
>>reminding.
>>
>>Well, the pci_dev and the bus->devices list are protected by pci_bus_sem and
>>we have already had a function to walk the pci bus safely by holding this
>>semaphore. While the pci_walk_bus() will continue the process when it sees a
>>bridge device.
>>
>>So I create another patch to export a function to walk just on the local bus
>>and then calling this from get_pci_function_alias_group() and
>>get_pci_alias_group().
>>
>>Since this change is related to pci subsystem, I include Bjorn.
>>
>>Hi, Bjorn,
>>
>>Haven't seen you for a long time. Hope you would like the idea.
>>
>>>Alex
>>>
>>>> +		if (tmp == pdev ||
>>>>  		    PCI_SLOT(tmp->devfn) != PCI_SLOT(pdev->devfn) ||
>>>>  		    pci_acs_enabled(tmp, REQ_ACS_FLAGS))
>>>>  			continue;
>>>>  
>>>>  		group = get_pci_alias_group(tmp, devfns);
>>>> -		if (group) {
>>>> -			pci_dev_put(tmp);
>>>> +		if (group)
>>>>  			return group;
>>>> -		}
>>>>  	}
>>>>  
>>>>  	return NULL;
>>>> @@ -681,23 +679,19 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
>>>>  	if (group)
>>>>  		return group;
>>>>  
>>>> -	for_each_pci_dev(tmp) {
>>>> -		if (tmp == pdev || tmp->bus != pdev->bus)
>>>> +	list_for_each_entry(tmp, &pdev->bus->devices, bus_list) {
>>>> +		if (tmp == pdev)
>>>>  			continue;
>>>>  
>>>>  		/* We alias them or they alias us */
>>>>  		if (pci_devs_are_dma_aliases(pdev, tmp)) {
>>>>  			group = get_pci_alias_group(tmp, devfns);
>>>> -			if (group) {
>>>> -				pci_dev_put(tmp);
>>>> +			if (group)
>>>>  				return group;
>>>> -			}
>>>>  
>>>>  			group = get_pci_function_alias_group(tmp, devfns);
>>>> -			if (group) {
>>>> -				pci_dev_put(tmp);
>>>> +			if (group)
>>>>  				return group;
>>>> -			}
>>>>  		}
>>>>  	}
>>>>  
>>>> @@ -794,7 +788,7 @@ struct iommu_group *pci_device_group(struct device *dev)
>>>>  
>>>>  	/*
>>>>  	 * Look for existing groups on non-isolated functions on the same
>>>> -	 * slot and aliases of those funcions, if any.  No need to clear
>>>> +	 * slot and aliases of those functions, if any. No need to clear
>>>>  	 * the search bitmap, the tested devfns are still valid.
>>>>  	 */
>>>>  	group = get_pci_function_alias_group(pdev, (unsigned long *)devfns);
>>
>>-- 
>>Wei Yang
>>Help you, Help me
>
>-- 
>Wei Yang
>Help you, Help me

-- 
Wei Yang
Help you, Help me