[PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

Here is some patches do the optimization/fix/cleanup for the function
pci_assign_unassigned_resources().

Wei Yang (4):
  PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy
  PCI: add comment for pbus_size_mem() parameter
  PCI: trivial cleanup in pbus_size_io()
  PCI: fix the io resource alignment calculation in pbus_size_io()

 drivers/pci/setup-bus.c |   56 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 42 insertions(+), 14 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Wei Yang]

Normally, on one pci bus there would be more devices than pci buses. When
calculating the depth of pci bus, it would be more time efficient by
enumerating through the child buses instead of the child devices.

Also by doing so, the code seems more self explaining. Previously, it go
through the pci devices and check whether a bridge introduce a child bus or
not, which needs more background knowledge to understand it.

This patch caculating the depth by enumerating on pci bus hierachy in an
iterative way.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Reviewed-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |   43 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 16abaaa..b333f73 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1299,22 +1299,43 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
 
 static int __init pci_bus_get_depth(struct pci_bus *bus)
 {
-	int depth = 0;
-	struct pci_dev *dev;
+	int max_depth, depth;
+	struct pci_bus *parent, *curr;
+	struct list_head *node;
 
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		int ret;
-		struct pci_bus *b = dev->subordinate;
-		if (!b)
-			continue;
+	/* no child? */
+	if (list_empty(&bus->children))
+		return 0;
 
-		ret = pci_bus_get_depth(b);
-		if (ret + 1 > depth)
-			depth = ret + 1;
+	node = bus->children.next;
+	parent = bus;
+	max_depth = depth = 1;
+
+	while (parent) {
+		/* hit the head, go back to parent level */
+		if (node == &parent->children) {
+			node = parent->node.next;
+			parent = parent->parent;
+			depth--;
+			continue;
+		}
+		curr = list_entry(node, struct pci_bus, node);
+		/* depth first */
+		if (!list_empty(&curr->children)) {
+			node = curr->children.next;
+			parent = curr;
+			depth++;
+			if (max_depth < depth)
+				max_depth = depth;
+		}
+		/* no child, go to the sibling */
+		else
+			node = curr->node.next;
 	}
 
-	return depth;
+	return max_depth;
 }
+
 static int __init pci_get_max_depth(void)
 {
 	int depth = 0;
-- 
1.7.5.4

[PATCH 2/4] PCI: add comment for pbus_size_mem() parameter

[Wei Yang]

In the comment of pbus_size_mem(), the comment of two parameters are missed.

This patch fills in the missing description for two parameters of
pbus_size_mem().

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
---
 drivers/pci/setup-bus.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b333f73..43dbe0a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -838,6 +838,8 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
  * pbus_size_mem() - size the memory window of a given bus
  *
  * @bus : the bus
+ * @mask: mask the resource flag, then compare it with type
+ * @type: the type of free resource from bridge
  * @min_size : the minimum memory window that must to be allocated
  * @add_size : additional optional memory window
  * @realloc_head : track the additional memory window on this list
-- 
1.7.5.4

[PATCH 3/4] PCI: trivial cleanup in pbus_size_io()

[Wei Yang]

This patch did several cleanup in pbus_size_io():
1. change the type of size to resource_size_t
2. fix the warning in dev_printk() for the change of type

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
---
 drivers/pci/setup-bus.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 43dbe0a..bd0ce39d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -747,7 +747,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 {
 	struct pci_dev *dev;
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
-	unsigned long size = 0, size0 = 0, size1 = 0;
+	resource_size_t size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
 	resource_size_t min_align, io_align, align;
 
@@ -807,8 +807,9 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		add_to_list(realloc_head, bus->self, b_res, size1-size0,
 			    min_align);
 		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
-				 "%pR to %pR add_size %lx\n", b_res,
-				 &bus->busn_res, size1-size0);
+				 "%pR to %pR add_size %llx\n", b_res,
+				 &bus->busn_res,
+				 (unsigned long long)size1-size0);
 	}
 }
 
-- 
1.7.5.4

[PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
it introduce a new method to calculate the window alignment of P2P bridge.

When the io_window_1k is set,  the calculation for the io resource alignment
is different from the original one. In the original logic before 462d9303,
the alignment is no bigger than 4K even the io_window_1k is set. The logic
introduced in 462d9303 will limit the alignment to 1k in this case.

This patch fix this issue.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
---
 drivers/pci/setup-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index bd0ce39d..5c60ca0 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
  		return;
 
 	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
+	/* Don't exceed 4KiB for windows requesting 1KiB alignment */
+	if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
+		io_align = PCI_P2P_DEFAULT_IO_ALIGN;
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
-- 
1.7.5.4

Re: [PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

On Tue, Jul 02, 2013 at 07:15:41AM +0800, Gavin Shan wrote:
>On Tue, Jul 02, 2013 at 07:10:41AM +0800, Gavin Shan wrote:
>>On Mon, Jul 01, 2013 at 11:10:28PM +0800, Wei Yang wrote:
>>>Here is some patches do the optimization/fix/cleanup for the function
>>>pci_assign_unassigned_resources().
>>>
>>
>>It looks good to me. Please make sure to rebase to upstream or linux-next
>>before sending out :-)
>>
>
>You already posted it to public and just realize it.

Yes :-)

This patch set is based on 3.10-rc2, so not the latest one.

Next time I will rebased the patch on the latest one and then send out.

>
>Thanks,
>Gavin

-- 
Richard Yang
Help you, Help me

Re: [PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

All,

Any comment on these patch set is welcome~

Not sure those are reasonable or not?

On Mon, Jul 01, 2013 at 11:10:28PM +0800, Wei Yang wrote:
>Here is some patches do the optimization/fix/cleanup for the function
>pci_assign_unassigned_resources().
>
>Wei Yang (4):
>  PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy
>  PCI: add comment for pbus_size_mem() parameter
>  PCI: trivial cleanup in pbus_size_io()
>  PCI: fix the io resource alignment calculation in pbus_size_io()
>
> drivers/pci/setup-bus.c |   56 +++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 42 insertions(+), 14 deletions(-)
>
>-- 
>1.7.5.4

-- 
Richard Yang
Help you, Help me

Re: [PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Bjorn Helgaas]

On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> Normally, on one pci bus there would be more devices than pci buses. When
> calculating the depth of pci bus, it would be more time efficient by
> enumerating through the child buses instead of the child devices.
>
> Also by doing so, the code seems more self explaining. Previously, it go
> through the pci devices and check whether a bridge introduce a child bus or
> not, which needs more background knowledge to understand it.
>
> This patch caculating the depth by enumerating on pci bus hierachy in an
> iterative way.

Your code does have the advantage of not being recursive, but the
original code is significantly shorter and, in my opinion, much more
readable.  This is not in a performance path, so I don't see any
advantage in optimizing.

> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> Reviewed-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |   43 ++++++++++++++++++++++++++++++++-----------
>  1 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 16abaaa..b333f73 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1299,22 +1299,43 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>
>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>  {
> -       int depth = 0;
> -       struct pci_dev *dev;
> +       int max_depth, depth;
> +       struct pci_bus *parent, *curr;
> +       struct list_head *node;
>
> -       list_for_each_entry(dev, &bus->devices, bus_list) {
> -               int ret;
> -               struct pci_bus *b = dev->subordinate;
> -               if (!b)
> -                       continue;
> +       /* no child? */
> +       if (list_empty(&bus->children))
> +               return 0;
>
> -               ret = pci_bus_get_depth(b);
> -               if (ret + 1 > depth)
> -                       depth = ret + 1;
> +       node = bus->children.next;
> +       parent = bus;
> +       max_depth = depth = 1;
> +
> +       while (parent) {
> +               /* hit the head, go back to parent level */
> +               if (node == &parent->children) {
> +                       node = parent->node.next;
> +                       parent = parent->parent;
> +                       depth--;
> +                       continue;
> +               }
> +               curr = list_entry(node, struct pci_bus, node);
> +               /* depth first */
> +               if (!list_empty(&curr->children)) {
> +                       node = curr->children.next;
> +                       parent = curr;
> +                       depth++;
> +                       if (max_depth < depth)
> +                               max_depth = depth;
> +               }
> +               /* no child, go to the sibling */
> +               else
> +                       node = curr->node.next;
>         }
>
> -       return depth;
> +       return max_depth;
>  }
> +
>  static int __init pci_get_max_depth(void)
>  {
>         int depth = 0;
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> it introduce a new method to calculate the window alignment of P2P bridge.
>
> When the io_window_1k is set,  the calculation for the io resource alignment
> is different from the original one. In the original logic before 462d9303,
> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> introduced in 462d9303 will limit the alignment to 1k in this case.
>
> This patch fix this issue.
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  drivers/pci/setup-bus.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index bd0ce39d..5c60ca0 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                 return;
>
>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;

Please explain why we need this change, with some actual values that
show the problem.  We need to know what the problem is, not merely
that the code behaves differently than it did before 462d9303.

It appears to me that this change will break the ability to use 1K
windows.  For example, assume a bridge that supports 1K windows.
Assume we're using the default pcibios_window_alignment().  Currently
window_alignment() on the secondary bus returns
PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.

With your change, I think io_align will be bumped back up to 4K in
this case, so we'll lose the ability to allocate a 1K window.

>         list_for_each_entry(dev, &bus->devices, bus_list) {
>                 int i;
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Wei Yang]

On Mon, Jul 08, 2013 at 02:46:17PM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> Normally, on one pci bus there would be more devices than pci buses. When
>> calculating the depth of pci bus, it would be more time efficient by
>> enumerating through the child buses instead of the child devices.
>>
>> Also by doing so, the code seems more self explaining. Previously, it go
>> through the pci devices and check whether a bridge introduce a child bus or
>> not, which needs more background knowledge to understand it.
>>
>> This patch caculating the depth by enumerating on pci bus hierachy in an
>> iterative way.
>
>Your code does have the advantage of not being recursive, but the
>original code is significantly shorter and, in my opinion, much more
>readable.  This is not in a performance path, so I don't see any
>advantage in optimizing.

Thanks for your comments~

Yes, this doesn't benefit a lot on the performance.

The benefit of this code is not only the recursive approach, but on 
calculating the depth of pci bus tree by iterating on pci bus tree instead of
the pci device tree. 

In my mind, there is less pci bus structrue than the pci device structure, and 
hope the code would be more self explaining for the audience by going through 
the pci bus tree to calculate the pci bus tree depth.

My original code looks like below. Do you think this one is more friendly to
the audience?

@@ -1300,21 +1300,19 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
 static int __init pci_bus_get_depth(struct pci_bus *bus)
 {
        int depth = 0;
-       struct pci_dev *dev;
+       struct pci_bus *child_bus;

-       list_for_each_entry(dev, &bus->devices, bus_list) {
+       list_for_each_entry(child_bus, &bus->children, node){
                int ret;
-               struct pci_bus *b = dev->subordinate;
-               if (!b)
-                       continue;

-               ret = pci_bus_get_depth(b);
+               ret = pci_bus_get_depth(child_bus);
                if (ret + 1 > depth)
                        depth = ret + 1;
        }

        return depth;
 }

And yes again, this change will not bring much improvement for the performance.
If this is not good, just ignore the code. :-)

>
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> Reviewed-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/setup-bus.c |   43 ++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 32 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 16abaaa..b333f73 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -1299,22 +1299,43 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>>
>>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>>  {
>> -       int depth = 0;
>> -       struct pci_dev *dev;
>> +       int max_depth, depth;
>> +       struct pci_bus *parent, *curr;
>> +       struct list_head *node;
>>
>> -       list_for_each_entry(dev, &bus->devices, bus_list) {
>> -               int ret;
>> -               struct pci_bus *b = dev->subordinate;
>> -               if (!b)
>> -                       continue;
>> +       /* no child? */
>> +       if (list_empty(&bus->children))
>> +               return 0;
>>
>> -               ret = pci_bus_get_depth(b);
>> -               if (ret + 1 > depth)
>> -                       depth = ret + 1;
>> +       node = bus->children.next;
>> +       parent = bus;
>> +       max_depth = depth = 1;
>> +
>> +       while (parent) {
>> +               /* hit the head, go back to parent level */
>> +               if (node == &parent->children) {
>> +                       node = parent->node.next;
>> +                       parent = parent->parent;
>> +                       depth--;
>> +                       continue;
>> +               }
>> +               curr = list_entry(node, struct pci_bus, node);
>> +               /* depth first */
>> +               if (!list_empty(&curr->children)) {
>> +                       node = curr->children.next;
>> +                       parent = curr;
>> +                       depth++;
>> +                       if (max_depth < depth)
>> +                               max_depth = depth;
>> +               }
>> +               /* no child, go to the sibling */
>> +               else
>> +                       node = curr->node.next;
>>         }
>>
>> -       return depth;
>> +       return max_depth;
>>  }
>> +
>>  static int __init pci_get_max_depth(void)
>>  {
>>         int depth = 0;
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>> it introduce a new method to calculate the window alignment of P2P bridge.
>>
>> When the io_window_1k is set,  the calculation for the io resource alignment
>> is different from the original one. In the original logic before 462d9303,
>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>> introduced in 462d9303 will limit the alignment to 1k in this case.
>>
>> This patch fix this issue.
>>
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>> ---
>>  drivers/pci/setup-bus.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index bd0ce39d..5c60ca0 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>                 return;
>>
>>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
>> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
>> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
>
>Please explain why we need this change, with some actual values that
>show the problem.  We need to know what the problem is, not merely
>that the code behaves differently than it did before 462d9303.

Yep, sorry for not listing the exact problem value.

Assume: 
	1. pcibios_window_alignment() return 1.
	2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
	3. one of the child device has an IO resource with size of 2K.

Result comparison: 

                    Before 462d9303             After 462d9303
    min_align       1k                          1k
                                        | 
                             after loop | 
                                        V
    min_align       2k                          2k
                                        |  
                         check boundary |
                                        V
    min_align       2k                          1k

In this case, with 462d9303 the min_align will be set back to 1k even one of
the child require 2k alignment.

>
>It appears to me that this change will break the ability to use 1K
>windows.  For example, assume a bridge that supports 1K windows.
>Assume we're using the default pcibios_window_alignment().  Currently
>window_alignment() on the secondary bus returns
>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>
>With your change, I think io_align will be bumped back up to 4K in
>this case, so we'll lose the ability to allocate a 1K window.

After applying the change:

Assume: 
	1. pcibios_window_alignment() return 1.
	2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
	3. one of the child device has an IO resource with size of 2K.

Result comparison: 

                    with  462d9303             with this patch
    min_align       1k                          1k
    io_align        1k                          4k
                                        | 
                             after loop | 
                                        V
    min_align       2k                          2k
    io_align        1k                          4k
                                        |  
                         check boundary |
                                        V
    min_align       1k                          2k
    io_align        1k                          1k

With this patch, in the same case as above, the min_align is 2k after
calculation.

In my mind, the min_align is the lower bound, io_align is the upper bound. The
final result of min_align should be in this range.

Is my understanding correct? or I missed something important?

>
>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>                 int i;
>>
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>>> it introduce a new method to calculate the window alignment of P2P bridge.
>>>
>>> When the io_window_1k is set,  the calculation for the io resource alignment
>>> is different from the original one. In the original logic before 462d9303,
>>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>>> introduced in 462d9303 will limit the alignment to 1k in this case.
>>>
>>> This patch fix this issue.
>>>
>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>> ---
>>>  drivers/pci/setup-bus.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>> index bd0ce39d..5c60ca0 100644
>>> --- a/drivers/pci/setup-bus.c
>>> +++ b/drivers/pci/setup-bus.c
>>> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>>                 return;
>>>
>>>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>>> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
>>> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
>>> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
>>
>>Please explain why we need this change, with some actual values that
>>show the problem.  We need to know what the problem is, not merely
>>that the code behaves differently than it did before 462d9303.
>
> Yep, sorry for not listing the exact problem value.
>
> Assume:
>         1. pcibios_window_alignment() return 1.
>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>         3. one of the child device has an IO resource with size of 2K.
>
> Result comparison:
>
>                     Before 462d9303             After 462d9303
>     min_align       1k                          1k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>                                         |
>                          check boundary |
>                                         V
>     min_align       2k                          1k
>
> In this case, with 462d9303 the min_align will be set back to 1k even one of
> the child require 2k alignment.
>
>>
>>It appears to me that this change will break the ability to use 1K
>>windows.  For example, assume a bridge that supports 1K windows.
>>Assume we're using the default pcibios_window_alignment().  Currently
>>window_alignment() on the secondary bus returns
>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>
>>With your change, I think io_align will be bumped back up to 4K in
>>this case, so we'll lose the ability to allocate a 1K window.
>
> After applying the change:
>
> Assume:
>         1. pcibios_window_alignment() return 1.
>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>         3. one of the child device has an IO resource with size of 2K.

What happens if no child has an I/O resource larger than 1K?  Can we
allocate a 1K window with 1K alignment in that case?

> Result comparison:
>
>                     with  462d9303             with this patch
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>     io_align        1k                          4k
>                                         |
>                          check boundary |
>                                         V
>     min_align       1k                          2k
>     io_align        1k                          1k
>
> With this patch, in the same case as above, the min_align is 2k after
> calculation.
>
> In my mind, the min_align is the lower bound, io_align is the upper bound. The
> final result of min_align should be in this range.
>
> Is my understanding correct? or I missed something important?
>
>>
>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>                 int i;
>>>
>>> --
>>> 1.7.5.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Richard Yang
> Help you, Help me
>

Re: [PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Bjorn Helgaas]

On Mon, Jul 8, 2013 at 8:38 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Mon, Jul 08, 2013 at 02:46:17PM -0600, Bjorn Helgaas wrote:
>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>> Normally, on one pci bus there would be more devices than pci buses. When
>>> calculating the depth of pci bus, it would be more time efficient by
>>> enumerating through the child buses instead of the child devices.
>>>
>>> Also by doing so, the code seems more self explaining. Previously, it go
>>> through the pci devices and check whether a bridge introduce a child bus or
>>> not, which needs more background knowledge to understand it.
>>>
>>> This patch caculating the depth by enumerating on pci bus hierachy in an
>>> iterative way.
>>
>>Your code does have the advantage of not being recursive, but the
>>original code is significantly shorter and, in my opinion, much more
>>readable.  This is not in a performance path, so I don't see any
>>advantage in optimizing.
>
> Thanks for your comments~
>
> Yes, this doesn't benefit a lot on the performance.
>
> The benefit of this code is not only the recursive approach, but on
> calculating the depth of pci bus tree by iterating on pci bus tree instead of
> the pci device tree.
>
> In my mind, there is less pci bus structrue than the pci device structure, and
> hope the code would be more self explaining for the audience by going through
> the pci bus tree to calculate the pci bus tree depth.
>
> My original code looks like below. Do you think this one is more friendly to
> the audience?

Yes.

> @@ -1300,21 +1300,19 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>  {
>         int depth = 0;
> -       struct pci_dev *dev;
> +       struct pci_bus *child_bus;
>
> -       list_for_each_entry(dev, &bus->devices, bus_list) {
> +       list_for_each_entry(child_bus, &bus->children, node){
>                 int ret;
> -               struct pci_bus *b = dev->subordinate;
> -               if (!b)
> -                       continue;
>
> -               ret = pci_bus_get_depth(b);
> +               ret = pci_bus_get_depth(child_bus);
>                 if (ret + 1 > depth)
>                         depth = ret + 1;
>         }
>
>         return depth;
>  }
>
> And yes again, this change will not bring much improvement for the performance.
> If this is not good, just ignore the code. :-)
>
>>
>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>> Reviewed-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>> ---
>>>  drivers/pci/setup-bus.c |   43 ++++++++++++++++++++++++++++++++-----------
>>>  1 files changed, 32 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>> index 16abaaa..b333f73 100644
>>> --- a/drivers/pci/setup-bus.c
>>> +++ b/drivers/pci/setup-bus.c
>>> @@ -1299,22 +1299,43 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>>>
>>>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>>>  {
>>> -       int depth = 0;
>>> -       struct pci_dev *dev;
>>> +       int max_depth, depth;
>>> +       struct pci_bus *parent, *curr;
>>> +       struct list_head *node;
>>>
>>> -       list_for_each_entry(dev, &bus->devices, bus_list) {
>>> -               int ret;
>>> -               struct pci_bus *b = dev->subordinate;
>>> -               if (!b)
>>> -                       continue;
>>> +       /* no child? */
>>> +       if (list_empty(&bus->children))
>>> +               return 0;
>>>
>>> -               ret = pci_bus_get_depth(b);
>>> -               if (ret + 1 > depth)
>>> -                       depth = ret + 1;
>>> +       node = bus->children.next;
>>> +       parent = bus;
>>> +       max_depth = depth = 1;
>>> +
>>> +       while (parent) {
>>> +               /* hit the head, go back to parent level */
>>> +               if (node == &parent->children) {
>>> +                       node = parent->node.next;
>>> +                       parent = parent->parent;
>>> +                       depth--;
>>> +                       continue;
>>> +               }
>>> +               curr = list_entry(node, struct pci_bus, node);
>>> +               /* depth first */
>>> +               if (!list_empty(&curr->children)) {
>>> +                       node = curr->children.next;
>>> +                       parent = curr;
>>> +                       depth++;
>>> +                       if (max_depth < depth)
>>> +                               max_depth = depth;
>>> +               }
>>> +               /* no child, go to the sibling */
>>> +               else
>>> +                       node = curr->node.next;
>>>         }
>>>
>>> -       return depth;
>>> +       return max_depth;
>>>  }
>>> +
>>>  static int __init pci_get_max_depth(void)
>>>  {
>>>         int depth = 0;
>>> --
>>> 1.7.5.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Richard Yang
> Help you, Help me
>

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>>>> it introduce a new method to calculate the window alignment of P2P bridge.
>>>>
>>>> When the io_window_1k is set,  the calculation for the io resource alignment
>>>> is different from the original one. In the original logic before 462d9303,
>>>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>>>> introduced in 462d9303 will limit the alignment to 1k in this case.
>>>>
>>>> This patch fix this issue.
>>>>
>>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>>> ---
>>>>  drivers/pci/setup-bus.c |    4 ++++
>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>>> index bd0ce39d..5c60ca0 100644
>>>> --- a/drivers/pci/setup-bus.c
>>>> +++ b/drivers/pci/setup-bus.c
>>>> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>>>                 return;
>>>>
>>>>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>>>> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
>>>> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
>>>> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
>>>
>>>Please explain why we need this change, with some actual values that
>>>show the problem.  We need to know what the problem is, not merely
>>>that the code behaves differently than it did before 462d9303.
>>
>> Yep, sorry for not listing the exact problem value.
>>
>> Assume:
>>         1. pcibios_window_alignment() return 1.
>>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>>         3. one of the child device has an IO resource with size of 2K.
>>
>> Result comparison:
>>
>>                     Before 462d9303             After 462d9303
>>     min_align       1k                          1k
>>                                         |
>>                              after loop |
>>                                         V
>>     min_align       2k                          2k
>>                                         |
>>                          check boundary |
>>                                         V
>>     min_align       2k                          1k
>>
>> In this case, with 462d9303 the min_align will be set back to 1k even one of
>> the child require 2k alignment.
>>
>>>
>>>It appears to me that this change will break the ability to use 1K
>>>windows.  For example, assume a bridge that supports 1K windows.
>>>Assume we're using the default pcibios_window_alignment().  Currently
>>>window_alignment() on the secondary bus returns
>>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>>
>>>With your change, I think io_align will be bumped back up to 4K in
>>>this case, so we'll lose the ability to allocate a 1K window.
>>
>> After applying the change:
>>
>> Assume:
>>         1. pcibios_window_alignment() return 1.
>>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>>         3. one of the child device has an IO resource with size of 2K.
>
>What happens if no child has an I/O resource larger than 1K?  Can we
>allocate a 1K window with 1K alignment in that case?
>

Yes, it could. The result comparison would look like this.
Since no child has an I/O resource larger than 1k, the min_align will remain
1k after loop. And because io_align(4K) is larger than min_align(1k), the
final min_align would be 1k.

In this case, the code from commit 462d9303 and my patch both works.

 Result comparison:
                     with  462d9303             with this patch
     min_align       1k                          1k
     io_align        1k                          4k
                                         |
                              after loop |
                                         V
     min_align       1k                          1k
     io_align        1k                          4k
                                         |
                          check boundary |
                                         V
     min_align       1k                          1k
     io_align        1k                          4k

>> Result comparison:
>>
>>                     with  462d9303             with this patch
>>     min_align       1k                          1k
>>     io_align        1k                          4k
>>                                         |
>>                              after loop |
>>                                         V
>>     min_align       2k                          2k
>>     io_align        1k                          4k
>>                                         |
>>                          check boundary |
>>                                         V
>>     min_align       1k                          2k
>>     io_align        1k                          4k
>>
>> With this patch, in the same case as above, the min_align is 2k after
>> calculation.
>>
>> In my mind, the min_align is the lower bound, io_align is the upper bound. The
>> final result of min_align should be in this range.
>>
>> Is my understanding correct? or I missed something important?
>>
>>>
>>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>>                 int i;
>>>>
>>>> --
>>>> 1.7.5.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>--
>>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>the body of a message to majordomo@vger.kernel.org
>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Richard Yang
>> Help you, Help me
>>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Richard Yang
Help you, Help me

Re: [PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Wei Yang]

On Tue, Jul 09, 2013 at 01:27:42PM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 8, 2013 at 8:38 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Mon, Jul 08, 2013 at 02:46:17PM -0600, Bjorn Helgaas wrote:
>>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>>> Normally, on one pci bus there would be more devices than pci buses. When
>>>> calculating the depth of pci bus, it would be more time efficient by
>>>> enumerating through the child buses instead of the child devices.
>>>>
>>>> Also by doing so, the code seems more self explaining. Previously, it go
>>>> through the pci devices and check whether a bridge introduce a child bus or
>>>> not, which needs more background knowledge to understand it.
>>>>
>>>> This patch caculating the depth by enumerating on pci bus hierachy in an
>>>> iterative way.
>>>
>>>Your code does have the advantage of not being recursive, but the
>>>original code is significantly shorter and, in my opinion, much more
>>>readable.  This is not in a performance path, so I don't see any
>>>advantage in optimizing.
>>
>> Thanks for your comments~
>>
>> Yes, this doesn't benefit a lot on the performance.
>>
>> The benefit of this code is not only the recursive approach, but on
>> calculating the depth of pci bus tree by iterating on pci bus tree instead of
>> the pci device tree.
>>
>> In my mind, there is less pci bus structrue than the pci device structure, and
>> hope the code would be more self explaining for the audience by going through
>> the pci bus tree to calculate the pci bus tree depth.
>>
>> My original code looks like below. Do you think this one is more friendly to
>> the audience?
>
>Yes.

Thanks for your comment.

If you agree, I will re-sent the patch with this recursive version.

>
>> @@ -1300,21 +1300,19 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>>  {
>>         int depth = 0;
>> -       struct pci_dev *dev;
>> +       struct pci_bus *child_bus;
>>
>> -       list_for_each_entry(dev, &bus->devices, bus_list) {
>> +       list_for_each_entry(child_bus, &bus->children, node){
>>                 int ret;
>> -               struct pci_bus *b = dev->subordinate;
>> -               if (!b)
>> -                       continue;
>>
>> -               ret = pci_bus_get_depth(b);
>> +               ret = pci_bus_get_depth(child_bus);
>>                 if (ret + 1 > depth)
>>                         depth = ret + 1;
>>         }
>>
>>         return depth;
>>  }
>>
>> And yes again, this change will not bring much improvement for the performance.
>> If this is not good, just ignore the code. :-)
>>
>>>
>>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>>> Reviewed-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>>> ---
>>>>  drivers/pci/setup-bus.c |   43 ++++++++++++++++++++++++++++++++-----------
>>>>  1 files changed, 32 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>>> index 16abaaa..b333f73 100644
>>>> --- a/drivers/pci/setup-bus.c
>>>> +++ b/drivers/pci/setup-bus.c
>>>> @@ -1299,22 +1299,43 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
>>>>
>>>>  static int __init pci_bus_get_depth(struct pci_bus *bus)
>>>>  {
>>>> -       int depth = 0;
>>>> -       struct pci_dev *dev;
>>>> +       int max_depth, depth;
>>>> +       struct pci_bus *parent, *curr;
>>>> +       struct list_head *node;
>>>>
>>>> -       list_for_each_entry(dev, &bus->devices, bus_list) {
>>>> -               int ret;
>>>> -               struct pci_bus *b = dev->subordinate;
>>>> -               if (!b)
>>>> -                       continue;
>>>> +       /* no child? */
>>>> +       if (list_empty(&bus->children))
>>>> +               return 0;
>>>>
>>>> -               ret = pci_bus_get_depth(b);
>>>> -               if (ret + 1 > depth)
>>>> -                       depth = ret + 1;
>>>> +       node = bus->children.next;
>>>> +       parent = bus;
>>>> +       max_depth = depth = 1;
>>>> +
>>>> +       while (parent) {
>>>> +               /* hit the head, go back to parent level */
>>>> +               if (node == &parent->children) {
>>>> +                       node = parent->node.next;
>>>> +                       parent = parent->parent;
>>>> +                       depth--;
>>>> +                       continue;
>>>> +               }
>>>> +               curr = list_entry(node, struct pci_bus, node);
>>>> +               /* depth first */
>>>> +               if (!list_empty(&curr->children)) {
>>>> +                       node = curr->children.next;
>>>> +                       parent = curr;
>>>> +                       depth++;
>>>> +                       if (max_depth < depth)
>>>> +                               max_depth = depth;
>>>> +               }
>>>> +               /* no child, go to the sibling */
>>>> +               else
>>>> +                       node = curr->node.next;
>>>>         }
>>>>
>>>> -       return depth;
>>>> +       return max_depth;
>>>>  }
>>>> +
>>>>  static int __init pci_get_max_depth(void)
>>>>  {
>>>>         int depth = 0;
>>>> --
>>>> 1.7.5.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Richard Yang
>> Help you, Help me
>>

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Wed, Jul 10, 2013 at 09:34:42AM +0800, Wei Yang wrote:
>On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
>>On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>>>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>>>>> it introduce a new method to calculate the window alignment of P2P bridge.
>>>>>
>>>>> When the io_window_1k is set,  the calculation for the io resource alignment
>>>>> is different from the original one. In the original logic before 462d9303,
>>>>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>>>>> introduced in 462d9303 will limit the alignment to 1k in this case.
>>>>>
>>>>> This patch fix this issue.
>>>>>
>>>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>>>> ---
>>>>>  drivers/pci/setup-bus.c |    4 ++++
>>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>>>> index bd0ce39d..5c60ca0 100644
>>>>> --- a/drivers/pci/setup-bus.c
>>>>> +++ b/drivers/pci/setup-bus.c
>>>>> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>>>>                 return;
>>>>>
>>>>>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>>>>> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
>>>>> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
>>>>> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
>>>>
>>>>Please explain why we need this change, with some actual values that
>>>>show the problem.  We need to know what the problem is, not merely
>>>>that the code behaves differently than it did before 462d9303.
>>>
>>> Yep, sorry for not listing the exact problem value.
>>>
>>> Assume:
>>>         1. pcibios_window_alignment() return 1.
>>>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>>>         3. one of the child device has an IO resource with size of 2K.
>>>
>>> Result comparison:
>>>
>>>                     Before 462d9303             After 462d9303
>>>     min_align       1k                          1k
>>>                                         |
>>>                              after loop |
>>>                                         V
>>>     min_align       2k                          2k
>>>                                         |
>>>                          check boundary |
>>>                                         V
>>>     min_align       2k                          1k
>>>
>>> In this case, with 462d9303 the min_align will be set back to 1k even one of
>>> the child require 2k alignment.
>>>
>>>>
>>>>It appears to me that this change will break the ability to use 1K
>>>>windows.  For example, assume a bridge that supports 1K windows.
>>>>Assume we're using the default pcibios_window_alignment().  Currently
>>>>window_alignment() on the secondary bus returns
>>>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>>>
>>>>With your change, I think io_align will be bumped back up to 4K in
>>>>this case, so we'll lose the ability to allocate a 1K window.
>>>
>>> After applying the change:
>>>
>>> Assume:
>>>         1. pcibios_window_alignment() return 1.
>>>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>>>         3. one of the child device has an IO resource with size of 2K.
>>
>>What happens if no child has an I/O resource larger than 1K?  Can we
>>allocate a 1K window with 1K alignment in that case?
>>
>
>Yes, it could. The result comparison would look like this.
>Since no child has an I/O resource larger than 1k, the min_align will remain
>1k after loop. And because io_align(4K) is larger than min_align(1k), the
>final min_align would be 1k.
>
>In this case, the code from commit 462d9303 and my patch both works.
>
> Result comparison:
>                     with  462d9303             with this patch
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                              after loop |
>                                         V
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                          check boundary |
>                                         V
>     min_align       1k                          1k
>     io_align        1k                          4k
>

Bjorn,

Sorry for distubing you again.

Is my analysis correct or I still miss some point?

>>> Result comparison:
>>>
>>>                     with  462d9303             with this patch
>>>     min_align       1k                          1k
>>>     io_align        1k                          4k
>>>                                         |
>>>                              after loop |
>>>                                         V
>>>     min_align       2k                          2k
>>>     io_align        1k                          4k
>>>                                         |
>>>                          check boundary |
>>>                                         V
>>>     min_align       1k                          2k
>>>     io_align        1k                          4k
>>>
>>> With this patch, in the same case as above, the min_align is 2k after
>>> calculation.
>>>
>>> In my mind, the min_align is the lower bound, io_align is the upper bound. The
>>> final result of min_align should be in this range.
>>>
>>> Is my understanding correct? or I missed something important?
>>>
>>>>
>>>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>>>                 int i;
>>>>>
>>>>> --
>>>>> 1.7.5.4
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>--
>>>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>>the body of a message to majordomo@vger.kernel.org
>>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Richard Yang
>>> Help you, Help me
>>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>-- 
>Richard Yang
>Help you, Help me

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Wed, Jul 10, 2013 at 09:34:42AM +0800, Wei Yang wrote:
> On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
> >On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
> >>>On Mon, Jul 1, 2013 at 9:10 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >>>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> >>>> it introduce a new method to calculate the window alignment of P2P bridge.
> >>>>
> >>>> When the io_window_1k is set,  the calculation for the io resource alignment
> >>>> is different from the original one. In the original logic before 462d9303,
> >>>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> >>>> introduced in 462d9303 will limit the alignment to 1k in this case.
> >>>>
> >>>> This patch fix this issue.
> >>>>
> >>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> >>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> >>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> >>>> ---
> >>>>  drivers/pci/setup-bus.c |    4 ++++
> >>>>  1 files changed, 4 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> >>>> index bd0ce39d..5c60ca0 100644
> >>>> --- a/drivers/pci/setup-bus.c
> >>>> +++ b/drivers/pci/setup-bus.c
> >>>> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >>>>                 return;
> >>>>
> >>>>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> >>>> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
> >>>> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
> >>>> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
> >>>
> >>>Please explain why we need this change, with some actual values that
> >>>show the problem.  We need to know what the problem is, not merely
> >>>that the code behaves differently than it did before 462d9303.
> >>
> >> Yep, sorry for not listing the exact problem value.
> >>
> >> Assume:
> >>         1. pcibios_window_alignment() return 1.
> >>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
> >>         3. one of the child device has an IO resource with size of 2K.
> >>
> >> Result comparison:
> >>
> >>                     Before 462d9303             After 462d9303
> >>     min_align       1k                          1k
> >>                                         |
> >>                              after loop |
> >>                                         V
> >>     min_align       2k                          2k
> >>                                         |
> >>                          check boundary |
> >>                                         V
> >>     min_align       2k                          1k
> >>
> >> In this case, with 462d9303 the min_align will be set back to 1k even one of
> >> the child require 2k alignment.
> >>
> >>>
> >>>It appears to me that this change will break the ability to use 1K
> >>>windows.  For example, assume a bridge that supports 1K windows.
> >>>Assume we're using the default pcibios_window_alignment().  Currently
> >>>window_alignment() on the secondary bus returns
> >>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
> >>>
> >>>With your change, I think io_align will be bumped back up to 4K in
> >>>this case, so we'll lose the ability to allocate a 1K window.
> >>
> >> After applying the change:
> >>
> >> Assume:
> >>         1. pcibios_window_alignment() return 1.
> >>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
> >>         3. one of the child device has an IO resource with size of 2K.
> >
> >What happens if no child has an I/O resource larger than 1K?  Can we
> >allocate a 1K window with 1K alignment in that case?
> >
> 
> Yes, it could. The result comparison would look like this.
> Since no child has an I/O resource larger than 1k, the min_align will remain
> 1k after loop. And because io_align(4K) is larger than min_align(1k), the
> final min_align would be 1k.
> 
> In this case, the code from commit 462d9303 and my patch both works.
> 
>  Result comparison:
>                      with  462d9303             with this patch
>      min_align       1k                          1k
>      io_align        1k                          4k
>                                          |
>                               after loop |
>                                          V
>      min_align       1k                          1k
>      io_align        1k                          4k
>                                          |
>                           check boundary |
>                                          V
>      min_align       1k                          1k
>      io_align        1k                          4k
> 
> >> Result comparison:
> >>
> >>                     with  462d9303             with this patch
> >>     min_align       1k                          1k
> >>     io_align        1k                          4k
> >>                                         |
> >>                              after loop |
> >>                                         V
> >>     min_align       2k                          2k
> >>     io_align        1k                          4k
> >>                                         |
> >>                          check boundary |
> >>                                         V
> >>     min_align       1k                          2k
> >>     io_align        1k                          4k
> >>
> >> With this patch, in the same case as above, the min_align is 2k after
> >> calculation.
> >>
> >> In my mind, the min_align is the lower bound, io_align is the upper bound. The
> >> final result of min_align should be in this range.
> >>
> >> Is my understanding correct? or I missed something important?

Since Gavin has reviewed this, I'm OK with it.  If you resend the series
with the updated changelogs and so on, I'll apply it.

Bjorn

> >>
> >>>
> >>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
> >>>>                 int i;
> >>>>
> >>>> --
> >>>> 1.7.5.4
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >>>> the body of a message to majordomo@vger.kernel.org
> >>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> --
> >> Richard Yang
> >> Help you, Help me
> >>
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Richard Yang
> Help you, Help me
> 

[PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Wei Yang]

Normally, on one pci bus there would be more devices than pci buses. When
calculating the depth of pci bus, it would be more time efficient by
enumerating through the child buses instead of the child devices.

Also by doing so, the code seems more self explaining. Previously, it go
through the pci devices and check whether a bridge introduce a child bus or
not, which needs more background knowledge to understand it.

This patch caculating the depth by enumerating on pci bus hierachy.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d254e23..6dbe562 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1300,15 +1300,12 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
 static int __init pci_bus_get_depth(struct pci_bus *bus)
 {
 	int depth = 0;
-	struct pci_dev *dev;
+	struct pci_bus *child_bus;
 
-	list_for_each_entry(dev, &bus->devices, bus_list) {
+	list_for_each_entry(child_bus, &bus->children, node){
 		int ret;
-		struct pci_bus *b = dev->subordinate;
-		if (!b)
-			continue;
 
-		ret = pci_bus_get_depth(b);
+		ret = pci_bus_get_depth(child_bus);
 		if (ret + 1 > depth)
 			depth = ret + 1;
 	}
-- 
1.7.5.4