Skip to contentSkip to conversation

PCI: Equalize hotplug memory for non/occupied slots

PATCH Superseded Jon Derrick 2 participants 4 messages Jul 25, 2018 – Aug 14, 2018 PCI

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 PCI: Equalize hotplug memory for non/occupied slots

Currently, a hotplug bridge will be given hpmemsize additional memory if available, in order to satisfy any future hotplug allocation requirements.

These calculations don't consider the current memory size of the hotplug bridge/slot, so hotplug bridges/slots which have downstream devices will get their current allocation in addition to the hpmemsize value.

This makes for possibly undesirable results with a mix of unoccupied and occupied slots (ex, with hpmemsize=2M):

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d64fffff [size=3M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6500000-d66fffff [size=2M]

This change considers the current allocation size when using the hpmemsize parameter to make the reservations predictable for the mix of unoccupied and occupied slots:

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d63fffff [size=2M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6400000-d65fffff [size=2M]

---
Original RFC here:
https://patchwork.ozlabs.org/patch/945374/

I split this bit out from the RFC while awaiting the pci string handling enhancements to handle per-device settings

Changed from RFC is a simpler algo

 drivers/pci/setup-bus.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/pci/setup-bus.c +6 -7
index 79b1824..5ae39e6 100644
@@ -831,7 +831,8 @@ static resource_size_t calculate_iosize(resource_size_t size,
 
 static resource_size_t calculate_memsize(resource_size_t size,
 		resource_size_t min_size,
-		resource_size_t size1,
+		resource_size_t add_size,
+		resource_size_t children_add_size,
 		resource_size_t old_size,
 		resource_size_t align)
 {
@@ -841,7 +842,7 @@ static resource_size_t calculate_memsize(resource_size_t size,
 		old_size = 0;
 	if (size < old_size)
 		size = old_size;
-	size = ALIGN(size + size1, align);
+	size = ALIGN(max(size, add_size) + children_add_size, align);
 	return size;
 }
 
@@ -1079,12 +1080,10 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 
 	min_align = calculate_mem_align(aligns, max_order);
 	min_align = max(min_align, window_alignment(bus, b_res->flags));
-	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
+	size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), min_align);
 	add_align = max(min_align, add_align);
-	if (children_add_size > add_size)
-		add_size = children_add_size;
-	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
-		calculate_memsize(size, min_size, add_size,
+	size1 = (!realloc_head || (realloc_head && !add_size && !children_add_size)) ? size0 :
+		calculate_memsize(size, min_size, add_size, children_add_size,
 				resource_size(b_res), add_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
Signature · 1 line
1.8.3.1
#1 lore raw

It's been a few weeks. Thoughts on this one?

Quoted 93 lines · Jon Derrick

Currently, a hotplug bridge will be given hpmemsize additional memory if
available, in order to satisfy any future hotplug allocation requirements.

These calculations don't consider the current memory size of the hotplug
bridge/slot, so hotplug bridges/slots which have downstream devices will
get their current allocation in addition to the hpmemsize value.

This makes for possibly undesirable results with a mix of unoccupied and
occupied slots (ex, with hpmemsize=2M):

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d64fffff [size=3M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6500000-d66fffff [size=2M]

This change considers the current allocation size when using the hpmemsize parameter to make the reservations predictable for the mix of
unoccupied and occupied slots:

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d63fffff [size=2M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6400000-d65fffff [size=2M]

---
Original RFC here:
https://patchwork.ozlabs.org/patch/945374/

I split this bit out from the RFC while awaiting the pci string handling
enhancements to handle per-device settings

Changed from RFC is a simpler algo

 drivers/pci/setup-bus.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/pci/setup-bus.c +2 -1
index 79b1824..5ae39e6 100644
@@ -831,7 +831,8 @@ static resource_size_t
 calculate_iosize(resource_size_t size,
 
 static resource_size_t calculate_memsize(resource_size_t size,
 		resource_size_t min_size,
-		resource_size_t size1,
+		resource_size_t add_size,
+		resource_size_t children_add_size,
 		resource_size_t old_size,
 		resource_size_t align)

{

drivers/pci/setup-bus.c +1 -1
@@ -841,7 +842,7 @@ static resource_size_t
 calculate_memsize(resource_size_t size,
 		old_size = 0;
 	if (size < old_size)
 		size = old_size;
-	size = ALIGN(size + size1, align);
+	size = ALIGN(max(size, add_size) + children_add_size,
 align);
 	return size;

}

drivers/pci/setup-bus.c +1 -4
@@ -1079,12 +1080,10 @@ static int pbus_size_mem(struct pci_bus *bus,
 unsigned long mask,
 
 	min_align = calculate_mem_align(aligns, max_order);
 	min_align = max(min_align, window_alignment(bus, b_res-
 >flags));
-	size0 = calculate_memsize(size, min_size, 0,
 resource_size(b_res), min_align);
+	size0 = calculate_memsize(size, min_size, 0, 0,
 resource_size(b_res), min_align);
 	add_align = max(min_align, add_align);
-	if (children_add_size > add_size)
-		add_size = children_add_size;
-	size1 = (!realloc_head || (realloc_head && !add_size)) ?

size0 :

drivers/pci/setup-bus.c +1 -1
-		calculate_memsize(size, min_size, add_size,
+	size1 = (!realloc_head || (realloc_head && !add_size &&

!children_add_size)) ? size0 :

drivers/pci/setup-bus.c +1 -0
+		calculate_memsize(size, min_size, add_size,

children_add_size,

drivers/pci/setup-bus.c +0 -0
 				resource_size(b_res), add_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
#2 lore raw
In reply to Jon Derrick
Quoted 16 lines · Jon Derrick

Currently, a hotplug bridge will be given hpmemsize additional memory if available, in order to satisfy any future hotplug allocation requirements.

These calculations don't consider the current memory size of the hotplug bridge/slot, so hotplug bridges/slots which have downstream devices will get their current allocation in addition to the hpmemsize value.

This makes for possibly undesirable results with a mix of unoccupied and occupied slots (ex, with hpmemsize=2M):

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d64fffff [size=3M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6500000-d66fffff [size=2M]
Quoted 8 lines

This change considers the current allocation size when using the hpmemsize parameter to make the reservations predictable for the mix of unoccupied and occupied slots:

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d63fffff [size=2M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6400000-d65fffff [size=2M]

The I/O sizing code (pbus_size_io() and calculate_iosize()) is essentially identical to the mem sizing code you're updating. I assume the same considerations would apply there? If not, please include a note in the changelog about why you changed the mem code but not the I/O code.

Quoted 55 lines

---
Original RFC here:
https://patchwork.ozlabs.org/patch/945374/

I split this bit out from the RFC while awaiting the pci string handling enhancements to handle per-device settings

Changed from RFC is a simpler algo

 drivers/pci/setup-bus.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/pci/setup-bus.c +6 -7
index 79b1824..5ae39e6 100644
@@ -831,7 +831,8 @@ static resource_size_t calculate_iosize(resource_size_t size,
 
 static resource_size_t calculate_memsize(resource_size_t size,
 		resource_size_t min_size,
-		resource_size_t size1,
+		resource_size_t add_size,
+		resource_size_t children_add_size,
 		resource_size_t old_size,
 		resource_size_t align)
 {
@@ -841,7 +842,7 @@ static resource_size_t calculate_memsize(resource_size_t size,
 		old_size = 0;
 	if (size < old_size)
 		size = old_size;
-	size = ALIGN(size + size1, align);
+	size = ALIGN(max(size, add_size) + children_add_size, align);
 	return size;
 }
 
@@ -1079,12 +1080,10 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 
 	min_align = calculate_mem_align(aligns, max_order);
 	min_align = max(min_align, window_alignment(bus, b_res->flags));
-	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
+	size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), min_align);
 	add_align = max(min_align, add_align);
-	if (children_add_size > add_size)
-		add_size = children_add_size;
-	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
-		calculate_memsize(size, min_size, add_size,
+	size1 = (!realloc_head || (realloc_head && !add_size && !children_add_size)) ? size0 :
+		calculate_memsize(size, min_size, add_size, children_add_size,
 				resource_size(b_res), add_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
Signature · 1 line
1.8.3.1
#3 lore raw
Quoted 31 lines · Bjorn Helgaas
Quoted 25 lines · Jon Derrick

Currently, a hotplug bridge will be given hpmemsize additional memory if
available, in order to satisfy any future hotplug allocation requirements.

These calculations don't consider the current memory size of the hotplug
bridge/slot, so hotplug bridges/slots which have downstream devices will
get their current allocation in addition to the hpmemsize value.

This makes for possibly undesirable results with a mix of unoccupied and
occupied slots (ex, with hpmemsize=2M):

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d64fffff [size=3M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6500000-d66fffff [size=2M]

This change considers the current allocation size when using the hpmemsize parameter to make the reservations predictable for the mix of
unoccupied and occupied slots:

Quoted 4 lines

02:03.0 PCI bridge: <-- Occupied

	Memory behind bridge: d6200000-d63fffff [size=2M]
02:04.0 PCI bridge: <-- Unoccupied
	Memory behind bridge: d6400000-d65fffff [size=2M]
Quoted 7 lines

The I/O sizing code (pbus_size_io() and calculate_iosize()) is essentially
identical to the mem sizing code you're updating. I assume the same considerations would apply there? If not, please include a note in the
changelog about why you changed the mem code but not the I/O code.

Thanks for the look Bjorn.
I'll follow-up with v2 set with an I/O patch in a few days

[snip]