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

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

[Wei Yang]

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) {
+		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);
-- 
1.7.9.5

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

[Alex Williamson]

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

> +		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);

[PATCH 1/2] PCI: add a function to walk on local bus

[Wei Yang]

Add a function to just walk on the local bus.

Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pci/bus.c   | 26 +++++++++++++++++++++-----
 include/linux/pci.h |  2 ++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index dd7cdbe..ccf4290 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -338,17 +338,19 @@ EXPORT_SYMBOL(pci_bus_add_devices);
  *  @top      bus whose devices should be walked
  *  @cb       callback to be called for each device found
  *  @userdata arbitrary pointer to be passed to callback.
+ *  @local    whether it just walk on the local bus
  *
  *  Walk the given bus, including any bridged devices
- *  on buses under this bus.  Call the provided callback
- *  on each device found.
+ *  on buses under this bus if "local" is false.
+ *  Call the provided callback on each device found.
  *
  *  We check the return of @cb each time. If it returns anything
  *  other than 0, we break out.
  *
  */
-void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
-		  void *userdata)
+static void __pci_walk_bus(struct pci_bus *top,
+			   int (*cb)(struct pci_dev *, void *),
+			   void *userdata, bool local)
 {
 	struct pci_dev *dev;
 	struct pci_bus *bus;
@@ -368,7 +370,7 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 			continue;
 		}
 		dev = list_entry(next, struct pci_dev, bus_list);
-		if (dev->subordinate) {
+		if (dev->subordinate && !local) {
 			/* this is a pci-pci bridge, do its devices next */
 			next = dev->subordinate->devices.next;
 			bus = dev->subordinate;
@@ -381,8 +383,22 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 	}
 	up_read(&pci_bus_sem);
 }
+
+void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
+		  void *userdata)
+{
+	__pci_walk_bus(top, cb, userdata, false);
+}
 EXPORT_SYMBOL_GPL(pci_walk_bus);
 
+void pci_walk_bus_local(struct pci_bus *top,
+			int (*cb)(struct pci_dev *, void *),
+			void *userdata)
+{
+	__pci_walk_bus(top, cb, userdata, true);
+}
+EXPORT_SYMBOL_GPL(pci_walk_bus_local);
+
 struct pci_bus *pci_bus_get(struct pci_bus *bus)
 {
 	if (bus)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b67e4df..e43df8c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1224,6 +1224,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 
 void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 		  void *userdata);
+void pci_walk_bus_local(struct pci_bus *top,
+		  int (*cb)(struct pci_dev *, void *), void *userdata);
 int pci_cfg_space_size(struct pci_dev *dev);
 unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 void pci_setup_bridge(struct pci_bus *bus);
-- 
2.5.0

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

[Wei Yang]

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 | 100 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 37 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3000051..2fae2c6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -634,29 +634,45 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
  * each function, we also need to look for aliases to or from other devices
  * that may already have a group.
  */
+
+struct alias_group_cb_data {
+	struct pci_dev *pdev;
+	struct iommu_group *group;
+	unsigned long *devfns;
+};
+
+static int __get_pci_function_alias_group(struct pci_dev *tmp, void *data)
+{
+	struct alias_group_cb_data *cb_data =
+		(struct alias_group_cb_data *)data;
+	struct pci_dev *pdev = cb_data->pdev;
+
+	if (tmp == pdev ||
+	    PCI_SLOT(tmp->devfn) != PCI_SLOT(pdev->devfn) ||
+	    pci_acs_enabled(tmp, REQ_ACS_FLAGS))
+		return 0;
+
+	cb_data->group = get_pci_alias_group(tmp, cb_data->devfns);
+	if (cb_data->group)
+		return 1;
+	else
+		return 0;
+}
+
 static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
 							unsigned long *devfns)
 {
-	struct pci_dev *tmp = NULL;
-	struct iommu_group *group;
+	struct alias_group_cb_data data;
 
 	if (!pdev->multifunction || pci_acs_enabled(pdev, REQ_ACS_FLAGS))
 		return NULL;
 
-	for_each_pci_dev(tmp) {
-		if (tmp == pdev || tmp->bus != pdev->bus ||
-		    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);
-			return group;
-		}
-	}
+	data.pdev = pdev;
+	data.group = NULL;
+	data.devfns = devfns;
+	pci_walk_bus_local(pdev->bus, __get_pci_function_alias_group, &data);
 
-	return NULL;
+	return data.group;
 }
 
 /*
@@ -668,11 +684,36 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
  * multifunction devices could have aliases between them that would cause a
  * loop.  To prevent this, we use a bitmap to track where we've been.
  */
+static int __get_pci_alias_group(struct pci_dev *tmp, void *data)
+{
+	struct alias_group_cb_data *cb_data =
+		(struct alias_group_cb_data *)data;
+	struct pci_dev *pdev = cb_data->pdev;
+
+	if (tmp == pdev)
+		return 0;
+
+	/* We alias them or they alias us */
+	if (pci_devs_are_dma_aliases(pdev, tmp)) {
+		cb_data->group = get_pci_alias_group(tmp, cb_data->devfns);
+		if (cb_data->group)
+			return 1;
+
+		cb_data->group = get_pci_function_alias_group(tmp,
+					cb_data->devfns);
+		if (cb_data->group)
+			return 1;
+	}
+
+	return 0;
+}
+
+
 static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
 					       unsigned long *devfns)
 {
-	struct pci_dev *tmp = NULL;
 	struct iommu_group *group;
+	struct alias_group_cb_data data;
 
 	if (test_and_set_bit(pdev->devfn & 0xff, devfns))
 		return NULL;
@@ -681,27 +722,12 @@ 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)
-			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);
-				return group;
-			}
-
-			group = get_pci_function_alias_group(tmp, devfns);
-			if (group) {
-				pci_dev_put(tmp);
-				return group;
-			}
-		}
-	}
+	data.pdev = pdev;
+	data.group = NULL;
+	data.devfns = devfns;
+	pci_walk_bus_local(pdev->bus, __get_pci_alias_group, &data);
 
-	return NULL;
+	return data.group;
 }
 
 struct group_for_pci_data {
@@ -794,7 +820,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);
-- 
2.5.0

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

[Wei Yang]

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

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

[Wei Yang]

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

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

[Wei Yang]

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