Re: acpi based pci gap calculation - v3

[Alok Kataria]

Hi Jesse,

On Wed, 2008-07-16 at 17:03 -0700, Jesse Barnes wrote:
> On Wednesday, July 16, 2008 9:33 am Andi Kleen wrote:
> > > The only problem there is that linux-next doesn't get nearly the sort of
> > > testing coverage we need for this kind of change.
> >
> > Normally I tend to wait for one -mm release, which seems to be tested
> > by a reasonable number of people. If it survives that it is good
> > to be tested in Linus' tree.
> >
> > Just stuffing this in in literally the last minute doesn't seem
> > like a good idea.
> 
> Well it's hardly last minute given that the merge window only opened a couple
> of days ago...
> 
> But beyond that, now that I've thought about it a bit more I'm not even sure
> the patch is really correct (though it works on my test machines).  Shouldn't
> we be looking at _PRS not _CRS? 

IMO, the current resource allocations will give us a better idea of
which region is free.
Besides, from what i read PRS is optional and not all BIOS's may export
that.

>  And ideally we should try to find even more
> space, not less.  This patch made one of my machines lose quite a bit of
> space:
> 
> ...
> Allocating PCI resources starting at c0000000 (gap: bf000000:40f00000)
> ...
> ACPI: PCI resources should start at c0000000 (gap: bf000000:31000000)
> ...
> 

Yep, we should try to find more space but we should also make sure that
this space doesn't clash with any other resource.
As explained in my first mail while posting v1 of these patches, the
kernel ignores the memory hotplug range while calculating this gap for
PCI, and consequently these regions collide. 
With this patch we have put some constraints like looking only in the
producer regions rather than all regions which are right now not
reserved/used by the e820 map.

However, looking back again i think we can improve this further in some
cases.
I have made some more changes in the e820_search_gap algorithm to find
the *biggest* un-utilized gap in the 32bit address range, and have added
some debug print messages.
Can you please try this patch on your system and mail me your full
dmesg.

Thanks,
Alok

---

 arch/x86/kernel/e820.c |   19 +++++++++++++++++++
 arch/x86/pci/acpi.c    |    4 ++++
 2 files changed, 23 insertions(+), 0 deletions(-)


diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index a5383ae..298aeec 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -539,6 +539,8 @@ __init int e820_search_gap(unsigned long *gapstart, unsigned long *gapsize,
 
 	last = (end_addr && end_addr < MAX_GAP_END) ? end_addr : MAX_GAP_END;
 
+	printk("E820_DEBUG: Searching for gap between (0x%08lx - 0x%08llx)\n",
+							 start_addr, end_addr);
 	while (--i >= 0) {
 		unsigned long long start = e820.map[i].addr;
 		unsigned long long end = start + e820.map[i].size;
@@ -556,9 +558,26 @@ __init int e820_search_gap(unsigned long *gapstart, unsigned long *gapsize,
 			if (gap >= *gapsize) {
 				*gapsize = gap;
 				*gapstart = end;
+				printk("E820_DEBUG: Found gap starting at "
+					"0x%08llx size 0x%08llx\n", end, gap);
 				found = 1;
 			}
 		}
+		if (start > start_addr) {
+			unsigned long gap, prev_end;
+			prev_end = e820.map[i-1].addr + e820.map[i-1].size;
+			if (start_addr > prev_end) {
+				gap = start - start_addr;
+				if (gap >=*gapsize) {
+					*gapsize = gap;
+                                *gapstart = start_addr;
+                                printk("E820_DEBUG: Found gap at start starting at "
+                                        "0x%08llx size 0x%08llx\n", end, gap);
+                                found = 1;
+				start = start_addr;
+                        	}
+			}
+		}
 		if (start < last)
 			last = start;
 	}
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index fff2c42..1a88149 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -131,8 +131,12 @@ static acpi_status search_gap(struct acpi_resource *resource, void *data)
 		/*
 		 * We want space only in the 32bit address range
 		 */
+		printk("ACPI_DEBUG start_addr 0x%08lx gapsize 0x%08lx "
+			"address_length 0x%08lx\n", start_addr, gap->gapsize,
+			addr.address_length);
 		if (start_addr < UINT_MAX) {
 			end_addr = start_addr + addr.address_length;
+			printk("\t\tend_addr is 0x%08lx\n", end_addr);
 			e820_search_gap(&gap->gapstart, &gap->gapsize,
 					start_addr, end_addr);
 		}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/