Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4

From: Yinghai Lu <yinghai@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@elte.hu>,
	Jesse Barnes <jbarnes@virtuousgeek.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-pci@vger.kernel.org, yannick.roehlly@free.fr
Subject: Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4
Date: Sat, 18 Apr 2009 15:44:20 -0700	[thread overview]
Message-ID: <49EA57C4.1000603@kernel.org> (raw)
In-Reply-To: <alpine.LFD.2.00.0904181142530.4042@localhost.localdomain>

Linus Torvalds wrote:
> 
> On Sat, 18 Apr 2009, Yinghai Lu wrote:
>> except need to change 
>>> +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
>> ==> > +		reserve_region_with_split(&iomem_resource, start, end - 1, "RAM buffer");
> 
> Yes, I sent out a later email pointing that out.
> 
>> it will make sure dynmical allocating code will not use those range.
>>
>> and could make e820_setup_gap much simple.
> 
> ACK. In fact:
> 
>> Index: linux-2.6/arch/x86/kernel/e820.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/e820.c
>> +++ linux-2.6/arch/x86/kernel/e820.c
>> @@ -635,14 +635,12 @@ __init void e820_setup_gap(void)
>>  #endif
>>  
>>  	/*
>> -	 * See how much we want to round up: start off with
>> -	 * rounding to the next 1MB area.
>> +	 * e820_reserve_resources_late will protect stolen RAM
>> +	 * so just round it to 1M
>>  	 */
>>  	round = 0x100000;
>> -	while ((gapsize >> 4) > round)
>> -		round += round;
>> -	/* Fun with two's complement */
>> -	pci_mem_start = (gapstart + round) & -round;
>> +
>> +	pci_mem_start = roundup(gapstart, round);
> 
> You can just remove "round" entirely. It's no longer a variable, it's just 
> an odd way of saying 1M ;)
> 
>> Ingo, can you put those two patches in tip?
> 
> I would suggest that we first change "reserve_region_with_split()" to not 
> recurse into the region.
> 
> That function isn't used by anything else (we ended up using 
> "expand_to_fit()" instead in the one place that migth have used it), and 
> now th eone caller we do have would not want the recursion - if there 
> already exists a resource at the top level, we want to just avoid it.
> 
> This - again TOTALLY UNTESTED - patch removes the "recurse into conflicts" 
> code. Comments? Testing?
> 
> 			Linus
> ---
>  kernel/resource.c |   46 ++++++++++++----------------------------------
>  1 files changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index fd5d7d5..ac5f3a3 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -533,43 +533,21 @@ static void __init __reserve_region_with_split(struct resource *root,
>  	res->end = end;
>  	res->flags = IORESOURCE_BUSY;
>  
> -	for (;;) {
> -		conflict = __request_resource(parent, res);
> -		if (!conflict)
> -			break;
> -		if (conflict != parent) {
> -			parent = conflict;
> -			if (!(conflict->flags & IORESOURCE_BUSY))
> -				continue;
> -		}
> -
> -		/* Uhhuh, that didn't work out.. */
> -		kfree(res);
> -		res = NULL;
> -		break;
> -	}
> -
> -	if (!res) {
> -		/* failed, split and try again */
> -
> -		/* conflict covered whole area */
> -		if (conflict->start <= start && conflict->end >= end)
> -			return;
> +	conflict = __request_resource(parent, res);
> +	if (!conflict)
> +		return;
>  
> -		if (conflict->start > start)
> -			__reserve_region_with_split(root, start, conflict->start-1, name);
> -		if (!(conflict->flags & IORESOURCE_BUSY)) {
> -			resource_size_t common_start, common_end;
> +	/* failed, split and try again */
> +	kfree(res);
>  
> -			common_start = max(conflict->start, start);
> -			common_end = min(conflict->end, end);
> -			if (common_start < common_end)
> -				__reserve_region_with_split(root, common_start, common_end, name);
> -		}
> -		if (conflict->end < end)
> -			__reserve_region_with_split(root, conflict->end+1, end, name);
> -	}
> +	/* conflict covered whole area */
> +	if (conflict->start <= start && conflict->end >= end)
> +		return;
>  
> +	if (conflict->start > start)
> +		__reserve_region_with_split(root, start, conflict->start-1, name);
> +	if (conflict->end < end)
> +		__reserve_region_with_split(root, conflict->end+1, end, name);
>  }
>  
>  void __init reserve_region_with_split(struct resource *root,

with
[    0.000000] BIOS-provided physical RAM map:
[    0.000000]  BIOS-e820: 0000000000000100 - 0000000000097400 (usable)
[    0.000000]  BIOS-e820: 0000000000097400 - 00000000000a0000 (reserved)
[    0.000000]  BIOS-e820: 0000000000100000 - 00000000b7fa0000 (usable)
[    0.000000]  BIOS-e820: 00000000b7fae000 - 00000000b7fb0000 (usable)
[    0.000000]  BIOS-e820: 00000000b7fb0000 - 00000000b7fbe000 (ACPI data)
[    0.000000]  BIOS-e820: 00000000b7fbe000 - 00000000b7ff0000 (ACPI NVS)
[    0.000000]  BIOS-e820: 00000000b7ff0000 - 00000000b8000000 (reserved)
[    0.000000]  BIOS-e820: 00000000e0000000 - 00000000f0000000 (reserved)
[    0.000000]  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
[    0.000000]  BIOS-e820: 00000000fee00000 - 00000000fef00000 (reserved)
[    0.000000]  BIOS-e820: 00000000ff700000 - 0000000100000000 (reserved)
[    0.000000]  BIOS-e820: 0000000100000000 - 0000002048000000 (usable)

got

00000100-000973ff : System RAM
00097400-0009ffff : reserved
000a0000-000bffff : PCI Bus #00
000c0000-000cffff : pnp 00:0c
000e0000-000fffff : pnp 00:0c
00100000-b7f9ffff : System RAM
  00200000-00c68f6b : Kernel code
  00c68f6c-01332f7f : Kernel data
  015a6000-01fcaa57 : Kernel bss
  20000000-23ffffff : GART
b7fa0000-b7fadfff : RAM buffer
b7fae000-b7faffff : System RAM
b7fb0000-b7fbdfff : ACPI Tables
b7fbe000-b7feffff : ACPI Non-volatile Storage
b7ff0000-b7ffffff : reserved
...

YH