[RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Vivek Goyal]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi,

Is there a reason why "start" and "end" field of "struct resource" are of
type unsigned long. My understanding is that "struct resource" can be used
to represent any system resource including physical memory. But unsigned
long is not suffcient to represent memory more than 4GB on PAE systems. 

Currently /proc/iomem exports physical memory also apart from io device
memory. But on i386, it truncates any memory more than 4GB. This leads
to problems for kexec/kdump.

- Kexec reads /proc/iomem to determine the system memory layout and prepares
  a memory map based on that and passes it to the kernel being kexeced. Given
  the fact that memory more than 4GB has been truncated, new kernel never
  gets to see and use that memory.

- Kdump also reads /proc/iomem to determine the physical memory layout of
  the system and encodes this informaiton in ELF headers. After a crash
  new kernel parses these ELF headers being used by previous kernel and
  vmcore is prepared accordingly. As memory more than 4GB has been truncated,
  kdump never sees that memory and never prepares ELF headers for it. Hence
  vmcore is truncated and limited to 4GB even if there is more physical
  memory in the system.

One of the possible solutions to this problem is that expand the size
of "start" and "end" to "unsigned long long". But whole of the PCI and
driver code has been written assuming start and end to be unsigned long
and compiler starts throwing warnings. 
 
I am attaching a prototype patch which does minimal changes to make memory
more than 4GB appear in /proc/iomem. It does not take care of modifying
in tree drivers to supress warnings. 

Looking for your suggestion what's the best way to handle this issue. If
above approach seems reasonable then I can go ahead and do the changes 
for in tree drivers to handle the warnings.

Thanks
Vivek
-   


[-- Attachment #2: struct-resource-start-field-expansion.patch --]
[-- Type: text/plain, Size: 2559 bytes --]



Signed-off-by: Vivek Goyal <vgoyal@in.ibm.com>
---

 arch/i386/kernel/setup.c |    2 --
 include/linux/ioport.h   |    2 +-
 kernel/resource.c        |    8 ++++----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff -puN include/linux/ioport.h~struct-resource-start-field-expansion include/linux/ioport.h
--- linux-2.6.16-rc6-1M/include/linux/ioport.h~struct-resource-start-field-expansion	2006-03-15 14:25:46.000000000 -0500
+++ linux-2.6.16-rc6-1M-root/include/linux/ioport.h	2006-03-15 14:25:46.000000000 -0500
@@ -15,7 +15,7 @@
  */
 struct resource {
 	const char *name;
-	unsigned long start, end;
+	unsigned long long start, end;
 	unsigned long flags;
 	struct resource *parent, *sibling, *child;
 };
diff -puN kernel/resource.c~struct-resource-start-field-expansion kernel/resource.c
--- linux-2.6.16-rc6-1M/kernel/resource.c~struct-resource-start-field-expansion	2006-03-15 14:25:46.000000000 -0500
+++ linux-2.6.16-rc6-1M-root/kernel/resource.c	2006-03-15 14:25:46.000000000 -0500
@@ -33,7 +33,7 @@ EXPORT_SYMBOL(ioport_resource);
 struct resource iomem_resource = {
 	.name	= "PCI mem",
 	.start	= 0UL,
-	.end	= ~0UL,
+	.end	= ~0ULL,
 	.flags	= IORESOURCE_MEM,
 };
 
@@ -83,7 +83,7 @@ static int r_show(struct seq_file *m, vo
 	for (depth = 0, p = r; depth < MAX_IORES_LEVEL; depth++, p = p->parent)
 		if (p->parent == root)
 			break;
-	seq_printf(m, "%*s%0*lx-%0*lx : %s\n",
+	seq_printf(m, "%*s%0*Lx-%0*Lx : %s\n",
 			depth * 2, "",
 			width, r->start,
 			width, r->end,
@@ -151,8 +151,8 @@ __initcall(ioresources_init);
 /* Return the conflict entry if you can't request it */
 static struct resource * __request_resource(struct resource *root, struct resource *new)
 {
-	unsigned long start = new->start;
-	unsigned long end = new->end;
+	unsigned long long start = new->start;
+	unsigned long long end = new->end;
 	struct resource *tmp, **p;
 
 	if (end < start)
diff -puN arch/i386/kernel/setup.c~struct-resource-start-field-expansion arch/i386/kernel/setup.c
--- linux-2.6.16-rc6-1M/arch/i386/kernel/setup.c~struct-resource-start-field-expansion	2006-03-15 14:25:46.000000000 -0500
+++ linux-2.6.16-rc6-1M-root/arch/i386/kernel/setup.c	2006-03-15 14:25:46.000000000 -0500
@@ -1286,8 +1286,6 @@ legacy_init_iomem_resources(struct resou
 	probe_roms();
 	for (i = 0; i < e820.nr_map; i++) {
 		struct resource *res;
-		if (e820.map[i].addr + e820.map[i].size > 0x100000000ULL)
-			continue;
 		res = alloc_bootmem_low(sizeof(struct resource));
 		switch (e820.map[i].type) {
 		case E820_RAM:	res->name = "System RAM"; break;
_

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 1:31 PM, Vivek Goyal wrote:

> Hi,
>
> Is there a reason why "start" and "end" field of "struct resource"  
> are of
> type unsigned long. My understanding is that "struct resource" can  
> be used
> to represent any system resource including physical memory. But  
> unsigned
> long is not suffcient to represent memory more than 4GB on PAE  
> systems.
>
> Currently /proc/iomem exports physical memory also apart from io  
> device
> memory. But on i386, it truncates any memory more than 4GB. This leads
> to problems for kexec/kdump.
>
> - Kexec reads /proc/iomem to determine the system memory layout and  
> prepares
>   a memory map based on that and passes it to the kernel being  
> kexeced. Given
>   the fact that memory more than 4GB has been truncated, new kernel  
> never
>   gets to see and use that memory.
>
> - Kdump also reads /proc/iomem to determine the physical memory  
> layout of
>   the system and encodes this informaiton in ELF headers. After a  
> crash
>   new kernel parses these ELF headers being used by previous kernel  
> and
>   vmcore is prepared accordingly. As memory more than 4GB has been  
> truncated,
>   kdump never sees that memory and never prepares ELF headers for  
> it. Hence
>   vmcore is truncated and limited to 4GB even if there is more  
> physical
>   memory in the system.
>
> One of the possible solutions to this problem is that expand the size
> of "start" and "end" to "unsigned long long". But whole of the PCI and
> driver code has been written assuming start and end to be unsigned  
> long
> and compiler starts throwing warnings.
>
> I am attaching a prototype patch which does minimal changes to make  
> memory
> more than 4GB appear in /proc/iomem. It does not take care of  
> modifying
> in tree drivers to supress warnings.
>
> Looking for your suggestion what's the best way to handle this  
> issue. If
> above approach seems reasonable then I can go ahead and do the changes
> for in tree drivers to handle the warnings.
>
> Thanks
> Vivek
> -

Your patch is insufficient.  You really need to audit all the users  
of "struct resource".  If you search the lkml archives you will see  
that Deepak Saxena, GregKH, and myself have taken stabs at this.  You  
should start with Greg's patch which I'm guessing is rather out of  
date now.

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Arjan van de Ven]

> One of the possible solutions to this problem is that expand the size
> of "start" and "end" to "unsigned long long". But whole of the PCI and
> driver code has been written assuming start and end to be unsigned long
> and compiler starts throwing warnings. 


please use dma_addr_t then instead of unsigned long long

this is the right size on all platforms afaik (could a ppc64 person
verify this?> ;)

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 1:57 PM, Arjan van de Ven wrote:

>
>> One of the possible solutions to this problem is that expand the size
>> of "start" and "end" to "unsigned long long". But whole of the PCI  
>> and
>> driver code has been written assuming start and end to be unsigned  
>> long
>> and compiler starts throwing warnings.
>
>
> please use dma_addr_t then instead of unsigned long long
>
> this is the right size on all platforms afaik (could a ppc64 person
> verify this?> ;)

Actually we really just want "start" and "end" to be u64 on all  
platforms.  Linus was ok with this change but no one has gone through  
and fixed everything that would be required for it.

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 2:01 PM, Kumar Gala wrote:

>
> On Mar 15, 2006, at 1:57 PM, Arjan van de Ven wrote:
>
>>
>>> One of the possible solutions to this problem is that expand the  
>>> size
>>> of "start" and "end" to "unsigned long long". But whole of the  
>>> PCI and
>>> driver code has been written assuming start and end to be  
>>> unsigned long
>>> and compiler starts throwing warnings.
>>
>>
>> please use dma_addr_t then instead of unsigned long long
>>
>> this is the right size on all platforms afaik (could a ppc64 person
>> verify this?> ;)
>
> Actually we really just want "start" and "end" to be u64 on all  
> platforms.  Linus was ok with this change but no one has gone  
> through and fixed everything that would be required for it.

As my memory comes back to me on this.  I also believe that Andrew  
asked me for size comparisons between a kernel using 32-bit start/end  
and 64-bit start/end on a 32-bit machine for a allyesconfig build.  I  
don't believe I ever got around to doing this or reporting the  
numbers to him.  It would be useful to have both code size  
differences and run time (if possible).

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Kumar Gala <galak@kernel.crashing.org> writes:

> On Mar 15, 2006, at 1:57 PM, Arjan van de Ven wrote:
>
>>
>>> One of the possible solutions to this problem is that expand the size
>>> of "start" and "end" to "unsigned long long". But whole of the PCI  and
>>> driver code has been written assuming start and end to be unsigned  long
>>> and compiler starts throwing warnings.
>>
>>
>> please use dma_addr_t then instead of unsigned long long
>>
>> this is the right size on all platforms afaik (could a ppc64 person
>> verify this?> ;)
>
> Actually we really just want "start" and "end" to be u64 on all  platforms.
> Linus was ok with this change but no one has gone through  and fixed everything
> that would be required for it.

Since it is faster to ask :)

How is it that other pieces of code have problems?
Warnings or something nasty?

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 2:13 PM, Eric W. Biederman wrote:

> Kumar Gala <galak@kernel.crashing.org> writes:
>
>> On Mar 15, 2006, at 1:57 PM, Arjan van de Ven wrote:
>>
>>>
>>>> One of the possible solutions to this problem is that expand the  
>>>> size
>>>> of "start" and "end" to "unsigned long long". But whole of the  
>>>> PCI  and
>>>> driver code has been written assuming start and end to be  
>>>> unsigned  long
>>>> and compiler starts throwing warnings.
>>>
>>>
>>> please use dma_addr_t then instead of unsigned long long
>>>
>>> this is the right size on all platforms afaik (could a ppc64 person
>>> verify this?> ;)
>>
>> Actually we really just want "start" and "end" to be u64 on all   
>> platforms.
>> Linus was ok with this change but no one has gone through  and  
>> fixed everything
>> that would be required for it.
>
> Since it is faster to ask :)
>
> How is it that other pieces of code have problems?
> Warnings or something nasty?

Warnings primarily, however I think some places have assumptions  
about size that have to be looked at.

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Vivek Goyal]

On Wed, Mar 15, 2006 at 01:13:56PM -0700, Eric W. Biederman wrote:
> Kumar Gala <galak@kernel.crashing.org> writes:
> 
> > On Mar 15, 2006, at 1:57 PM, Arjan van de Ven wrote:
> >
> >>
> >>> One of the possible solutions to this problem is that expand the size
> >>> of "start" and "end" to "unsigned long long". But whole of the PCI  and
> >>> driver code has been written assuming start and end to be unsigned  long
> >>> and compiler starts throwing warnings.
> >>
> >>
> >> please use dma_addr_t then instead of unsigned long long
> >>
> >> this is the right size on all platforms afaik (could a ppc64 person
> >> verify this?> ;)
> >
> > Actually we really just want "start" and "end" to be u64 on all  platforms.
> > Linus was ok with this change but no one has gone through  and fixed everything
> > that would be required for it.
> 
> Since it is faster to ask :)
> 
> How is it that other pieces of code have problems?
> Warnings or something nasty?

Few problems which I have noticed so far.

- Many printk() warnings. Wherever start and end are being printed,
  the format specifier being used is %lx. Needs to be changed to %Lx.

- Some folks save a pointer of type (unsigned long *) to start and end field
  and then try to operate on it. This pointer type shall have to be changed
  to something like u64*.

	unsigned long *port, *end, *tport, *tend;
	port = &dev->res.port_resource[idx].start;

- Some folks cast "start" to a pointer and then use it. Compiler gives warning. 

	addr_reg = (void __iomem *) addr->start;

-vivek

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Kumar Gala <galak@kernel.crashing.org> writes:

> Warnings primarily, however I think some places have assumptions  about size
> that have to be looked at.

Ok.  So nothing too bad just a thorough audit.  Although any driver that
has a real problem with a 64bit type is already broken on every 64bit
arch.

And we do need the 64bit type so we can handle 64bit pci resources on
x86 and ppc32 at some point.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Benjamin LaHaise]

On Wed, Mar 15, 2006 at 02:31:14PM -0500, Vivek Goyal wrote:
> Is there a reason why "start" and "end" field of "struct resource" are of
> type unsigned long. My understanding is that "struct resource" can be used
> to represent any system resource including physical memory. But unsigned
> long is not suffcient to represent memory more than 4GB on PAE systems. 
> and compiler starts throwing warnings. 

Please make this depend on the kernel being compiled with PAE.  We don't 
need to bloat 32 bit kernels needlessly.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Vivek Goyal <vgoyal@in.ibm.com> writes:

> Few problems which I have noticed so far.
>
> - Many printk() warnings. Wherever start and end are being printed,
>   the format specifier being used is %lx. Needs to be changed to %Lx.

Sane, but we need to check the 64bit case as well.

> - Some folks save a pointer of type (unsigned long *) to start and end field
>   and then try to operate on it. This pointer type shall have to be changed
>   to something like u64*.
>
> 	unsigned long *port, *end, *tport, *tend;
> 	port = &dev->res.port_resource[idx].start;

Weird.

> - Some folks cast "start" to a pointer and then use it. Compiler gives warning.
>
> 	addr_reg = (void __iomem *) addr->start;

I'm not familiar enough with that part of the code off the top of my head
but that except for a few helper functions that kind of behavior should
be pretty much forbidden.

This feels like entering the guts of ugly barely working drivers at
the moment.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 2:53 PM, Benjamin LaHaise wrote:

> On Wed, Mar 15, 2006 at 02:31:14PM -0500, Vivek Goyal wrote:
>> Is there a reason why "start" and "end" field of "struct resource"  
>> are of
>> type unsigned long. My understanding is that "struct resource" can  
>> be used
>> to represent any system resource including physical memory. But  
>> unsigned
>> long is not suffcient to represent memory more than 4GB on PAE  
>> systems.
>> and compiler starts throwing warnings.
>
> Please make this depend on the kernel being compiled with PAE.  We  
> don't
> need to bloat 32 bit kernels needlessly.

I disagree.  I think we need to look to see what the "bloat" is  
before we go and make start/end config dependent.

It seems clear that drivers dont handle the fact that "start"/"end"  
change an 32-bit vs 64-bit archs to begin with.  By making this even  
more config dependent seems to be asking for more trouble.

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Benjamin LaHaise]

On Wed, Mar 15, 2006 at 03:05:30PM -0600, Kumar Gala wrote:
> I disagree.  I think we need to look to see what the "bloat" is  
> before we go and make start/end config dependent.

Eh?  32 bit kernels get used in embedded systems, which includes those 
with only 8MB of RAM.  The upper 32 bits will never be anything other 
than 0.

> It seems clear that drivers dont handle the fact that "start"/"end"  
> change an 32-bit vs 64-bit archs to begin with.  By making this even  
> more config dependent seems to be asking for more trouble.

You can't get a non-32 bit value on a 32 bit platform, so why should a 
driver be expected to handle anything?

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Kumar Gala <galak@kernel.crashing.org> writes:

> On Mar 15, 2006, at 2:53 PM, Benjamin LaHaise wrote:
>
>> On Wed, Mar 15, 2006 at 02:31:14PM -0500, Vivek Goyal wrote:
>>> Is there a reason why "start" and "end" field of "struct resource"  are of
>>> type unsigned long. My understanding is that "struct resource" can  be used
>>> to represent any system resource including physical memory. But  unsigned
>>> long is not suffcient to represent memory more than 4GB on PAE  systems.
>>> and compiler starts throwing warnings.
>>
>> Please make this depend on the kernel being compiled with PAE.  We  don't
>> need to bloat 32 bit kernels needlessly.
>
> I disagree.  I think we need to look to see what the "bloat" is  before we go
> and make start/end config dependent.
>
> It seems clear that drivers dont handle the fact that "start"/"end"  change an
> 32-bit vs 64-bit archs to begin with.  By making this even  more config
> dependent seems to be asking for more trouble.

Especially as all resource access are rare slow path operations.

Depending on PAE and the like look like an optimization to consider after
getting the parts working.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[linux-os (Dick Johnson)]

On Wed, 15 Mar 2006, Kumar Gala wrote:

>
> On Mar 15, 2006, at 2:53 PM, Benjamin LaHaise wrote:
>
>> On Wed, Mar 15, 2006 at 02:31:14PM -0500, Vivek Goyal wrote:
>>> Is there a reason why "start" and "end" field of "struct resource"
>>> are of
>>> type unsigned long. My understanding is that "struct resource" can
>>> be used
>>> to represent any system resource including physical memory. But
>>> unsigned
>>> long is not suffcient to represent memory more than 4GB on PAE
>>> systems.
>>> and compiler starts throwing warnings.
>>
>> Please make this depend on the kernel being compiled with PAE.  We
>> don't
>> need to bloat 32 bit kernels needlessly.
>
> I disagree.  I think we need to look to see what the "bloat" is
> before we go and make start/end config dependent.
>
> It seems clear that drivers dont handle the fact that "start"/"end"
> change an 32-bit vs 64-bit archs to begin with.  By making this even
> more config dependent seems to be asking for more trouble.
>
> - kumar

ix86 machines need to make many operations just to increment
or decrement a 64-bit variable, plus the operations are not
atomic so they need to be protected. The bloat of using 64-bit
objects in 32-bit machines is very real and a tremendous problem.
That's why all the stuff in the kernel wasn't 'long long' to
begin with. It's execution bloat, a.k.a., time expansion that
is the problem.

Bump a 32-bit variable, addressed from an index register:

 	incl	(%ebx)

Bump a 64-bit variable, addressed the same way.

 	addl	$1,(%ebx)	; Need to add because inc doesn't carry
 	adcl	$0,4(%ebx)	; Add 0 plus the carry-flag

If an interrupt or context-switch comes between the two operations,
the result is undefined, NotGood(tm).

Cheers,
Dick Johnson
Penguin : Linux version 2.6.15.4 on an i686 machine (5589.54 BogoMips).
Warning : 98.36% of all statistics are fiction, book release in April.
_
\x1a\x04

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Benjamin LaHaise]

On Wed, Mar 15, 2006 at 02:29:32PM -0700, Eric W. Biederman wrote:
> If the impact is very slight or unmeasurable this means the option
> needs to fall under CONFIG_EMBEDDED, where you can change if
> every last bit of RAM counts but otherwise you won't care.

But we have a data type that is correct for this usage: dma_addr_t.

> Having > 32bit values on a 32bit platform is not the issue.
> 
> Some drivers appear to puke simply because the value is 64bit.  Which
> means the driver will have problems on any 64bit kernel.  That kind
> of behavior is worth purging.

Forcing it to be a 64 bit value doesn't fix that problem, so that isn't 
a valid excuse for adding bloat.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Wed, Mar 15, 2006 at 03:05:30PM -0600, Kumar Gala wrote:
>> I disagree.  I think we need to look to see what the "bloat" is  
>> before we go and make start/end config dependent.
>
> Eh?  32 bit kernels get used in embedded systems, which includes those 
> with only 8MB of RAM.  The upper 32 bits will never be anything other 
> than 0.

If the impact is very slight or unmeasurable this means the option
needs to fall under CONFIG_EMBEDDED, where you can change if
every last bit of RAM counts but otherwise you won't care.

>> It seems clear that drivers dont handle the fact that "start"/"end"  
>> change an 32-bit vs 64-bit archs to begin with.  By making this even  
>> more config dependent seems to be asking for more trouble.
>
> You can't get a non-32 bit value on a 32 bit platform, so why should a 
> driver be expected to handle anything?

Having > 32bit values on a 32bit platform is not the issue.

Some drivers appear to puke simply because the value is 64bit.  Which
means the driver will have problems on any 64bit kernel.  That kind
of behavior is worth purging.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Kumar Gala]

On Mar 15, 2006, at 3:13 PM, Benjamin LaHaise wrote:

> On Wed, Mar 15, 2006 at 03:05:30PM -0600, Kumar Gala wrote:
>> I disagree.  I think we need to look to see what the "bloat" is
>> before we go and make start/end config dependent.
>
> Eh?  32 bit kernels get used in embedded systems, which includes those
> with only 8MB of RAM.  The upper 32 bits will never be anything other
> than 0.

Why do people equate embedded with small amounts of memory.  I know  
of embedded systems which use 32-bit PowerPCs that have >4G of system  
memory.

>> It seems clear that drivers dont handle the fact that "start"/"end"
>> change an 32-bit vs 64-bit archs to begin with.  By making this even
>> more config dependent seems to be asking for more trouble.
>
> You can't get a non-32 bit value on a 32 bit platform, so why should a
> driver be expected to handle anything?

I dont follow.  I would say that most drivers shouldn't care about  
the fact that they are on a 32-bit platform or 64-bit platform.  The  
point is that drivers have made assumptions about being on 32-bit  
platforms which breaks when a 32-bit platform supports a larger  
physical address space.

- kumar

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Wed, Mar 15, 2006 at 03:05:30PM -0600, Kumar Gala wrote:
>> I disagree.  I think we need to look to see what the "bloat" is  
>> before we go and make start/end config dependent.
>
> Eh?  32 bit kernels get used in embedded systems, which includes those 
> with only 8MB of RAM.  The upper 32 bits will never be anything other 
> than 0.

If the impact is very slight or unmeasurable this means the option
needs to fall under CONFIG_EMBEDDED, where you can change if
every last bit of RAM counts but otherwise you won't care.

>> It seems clear that drivers dont handle the fact that "start"/"end"  
>> change an 32-bit vs 64-bit archs to begin with.  By making this even  
>> more config dependent seems to be asking for more trouble.
>
> You can't get a non-32 bit value on a 32 bit platform, so why should a 
> driver be expected to handle anything?

Having > 32bit values on a 32bit platform is not the issue.

Some drivers appear to puke simply because the value is 64bit.  Which
means the driver will have problems on any 64bit kernel.  That kind
of behavior is worth purging.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[hackmiester / Hunter Fuller]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On Wednesday 15 March 2006 15:30, Kumar Gala  wrote:
> On Mar 15, 2006, at 3:13 PM, Benjamin LaHaise wrote:
> > On Wed, Mar 15, 2006 at 03:05:30PM -0600, Kumar Gala wrote:
> >> I disagree.  I think we need to look to see what the "bloat" is
> >> before we go and make start/end config dependent.
> >
> > Eh?  32 bit kernels get used in embedded systems, which includes those
> > with only 8MB of RAM.  The upper 32 bits will never be anything other
> > than 0.
>
> Why do people equate embedded with small amounts of memory.
They don't. I believe Kumar said "which includes those that have 8mB" and not 
"which all have 8mB" :)
 > I know 
> of embedded systems which use 32-bit PowerPCs that have >4G of system
> memory.
>
> >> It seems clear that drivers dont handle the fact that "start"/"end"
> >> change an 32-bit vs 64-bit archs to begin with.  By making this even
> >> more config dependent seems to be asking for more trouble.
> >
> > You can't get a non-32 bit value on a 32 bit platform, so why should a
> > driver be expected to handle anything?
>
> I dont follow.  I would say that most drivers shouldn't care about
> the fact that they are on a 32-bit platform or 64-bit platform.  The
> point is that drivers have made assumptions about being on 32-bit
> platforms which breaks when a 32-bit platform supports a larger
> physical address space.
Some platforms are way too different from a 32 bit one to have a driver 
support both... so in some cases this wouldn't be good.
>
> - kumar
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
--hackmiester
Walk a mile in my shoes and you will be a mile away in a new pair of shoes.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFD/yYl3ApzN91C7BcRAoVVAJ97uhjh30nQ4hd9bQ90gJqiwsLEfgCeKSrg
bVfqEeJ09WhO6Y51WHEHb6o=
=VTUd
-----END PGP SIGNATURE-----

-----BEGIN GEEK CODE BLOCK-----
Version: Geek Code v3.1 (PHP)
GCS/CM/E/IT d-@ s: a- C++$ UBLS*++++$ P+ L+++$ E- W++$ !N-- !o+ K-- !w-- !O-
M++$ V-- PS@ PE@ Y--? PGP++ !t--- 5--? !X-- !R-- tv-- b+ DI++ D++ G+ e++++
h---- r+++ z++++
------END GEEK CODE BLOCK------

Quick contact info:
Work: hfuller@stpaulsmobile.net
Personal: hackmiester@hackmiester.com
Large files/spam: hackmiester@gmail.com
GTalk:hackmiester/AIM:hackmiester1337/Y!:hackm1ester/IRC:irc.7sinz.net/7sinz

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

"linux-os \(Dick Johnson\)" <linux-os@analogic.com> writes:
> ix86 machines need to make many operations just to increment
> or decrement a 64-bit variable, plus the operations are not
> atomic so they need to be protected. The bloat of using 64-bit
> objects in 32-bit machines is very real and a tremendous problem.
> That's why all the stuff in the kernel wasn't 'long long' to
> begin with. It's execution bloat, a.k.a., time expansion that
> is the problem.
>
> Bump a 32-bit variable, addressed from an index register:
>
>  	incl	(%ebx)
>
> Bump a 64-bit variable, addressed the same way.
>
>  	addl	$1,(%ebx)	; Need to add because inc doesn't carry
>  	adcl	$0,4(%ebx)	; Add 0 plus the carry-flag
>
> If an interrupt or context-switch comes between the two operations,
> the result is undefined, NotGood(tm).

No it is well defined it just isn't atomic.  But difference.
The thing is struct resource is accessed less often the file
offsets which are already 64bit.  Basically they are only
used during driver initialization.

So while I agree in general that 64bit values should be avoided
this is one of those places where we can productively use a
64bit value, on a 32bit machine.

To keep using 32bit kernels on the newer x86 machines at some
point this will even become a requirement as 64bit BAR will
actually have 64bit values placed in them.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Wed, Mar 15, 2006 at 02:29:32PM -0700, Eric W. Biederman wrote:
>> If the impact is very slight or unmeasurable this means the option
>> needs to fall under CONFIG_EMBEDDED, where you can change if
>> every last bit of RAM counts but otherwise you won't care.
>
> But we have a data type that is correct for this usage: dma_addr_t.

Well the name is wrong.  Because these are in general not DMA addresses,
but it may have the other desired properties.  So it may be
useable.

>> Having > 32bit values on a 32bit platform is not the issue.
>> 
>> Some drivers appear to puke simply because the value is 64bit.  Which
>> means the driver will have problems on any 64bit kernel.  That kind
>> of behavior is worth purging.
>
> Forcing it to be a 64 bit value doesn't fix that problem, so that isn't 
> a valid excuse for adding bloat.

It doesn't fix it but it finds it.  Which is half the battle.  Once
the existing references are fixed up it makes it hard to introduce new
breakage like that, because more people see it.

As for bloat this assumes there will be some measurable bloat.
Resources are used in such a limited fashion I will be surprised if
you can measure any bloat.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[David S. Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Wed, 15 Mar 2006 20:57:45 +0100

> please use dma_addr_t then instead of unsigned long long
> 
> this is the right size on all platforms afaik (could a ppc64 person
> verify this?> ;)

Nope, it's 32-bit on Sparc64 for example and we definitely
want to support 64-bit BARs there.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Wed, Mar 15, 2006 at 02:29:32PM -0700, Eric W. Biederman wrote:
>> If the impact is very slight or unmeasurable this means the option
>> needs to fall under CONFIG_EMBEDDED, where you can change if
>> every last bit of RAM counts but otherwise you won't care.
>
> But we have a data type that is correct for this usage: dma_addr_t.

Actually now that I think back there are machines with < 4GiB of ram
with 64bit pci BAR values.  It is more common to have 32bit values BAR
values and > 4GiB of ram.

So I don't see dma_addr_t in general being the right choice.

Although I do suspect that in most cases dma_addr_t <= the
size of what is in struct resource.

Nor do I think struct resource is nearly as important when being
efficient, as dma_addr_t.  struct resource is only used during
driver setup which is a very rare event.  A few extra instructions
there likely will get lost in the noise and most of the will probably
be removed because they are in an __init section anyway.

Eric

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Benjamin LaHaise]

On Wed, Mar 15, 2006 at 02:59:54PM -0700, Eric W. Biederman wrote:
> Actually now that I think back there are machines with < 4GiB of ram
> with 64bit pci BAR values.  It is more common to have 32bit values BAR
> values and > 4GiB of ram.

Such machines on x86 would have to be compiled with PAE.  Ditto any other 
architecture, as you *have* to be able to represent those physical addresses, 
which requires having page tables that can map them, which requires whatever 
PAE is on the platform.

> Nor do I think struct resource is nearly as important when being
> efficient, as dma_addr_t.  struct resource is only used during
> driver setup which is a very rare event.  A few extra instructions
> there likely will get lost in the noise and most of the will probably
> be removed because they are in an __init section anyway.

Bloat for no good reason is a bad habit to get into.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Greg KH]

On Wed, Mar 15, 2006 at 02:31:14PM -0500, Vivek Goyal wrote:
> Hi,
> 
> Is there a reason why "start" and "end" field of "struct resource" are of
> type unsigned long. My understanding is that "struct resource" can be used
> to represent any system resource including physical memory. But unsigned
> long is not suffcient to represent memory more than 4GB on PAE systems. 

As Kumar has stated, people have tried to do this in the past.  Please
search the archives for the problems that will happen when you change
this.

I agree it should be fixed, but if you want to do this, you need to
audit a _lot_ of kernel code and fix it up everywhere.  Please start
with the old patches posted to lkml in the past and work from there.

good luck,

greg k-h

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> Benjamin LaHaise <bcrl@kvack.org> writes:
> 
> > On Wed, Mar 15, 2006 at 02:29:32PM -0700, Eric W. Biederman wrote:
> >> If the impact is very slight or unmeasurable this means the option
> >> needs to fall under CONFIG_EMBEDDED, where you can change if
> >> every last bit of RAM counts but otherwise you won't care.
> >
> > But we have a data type that is correct for this usage: dma_addr_t.
> 
> Well the name is wrong.  Because these are in general not DMA addresses,
> but it may have the other desired properties.  So it may be
> useable.

Yes, dma_addr_t does the right thing but has the wrong name.

I guess it should have been called bus_addr_t (?).  If so, I guess a
suitable compromise might be:

#ifndef bus_addr_t
#define bus_addr_t dma_addr_t
#endif

then use bus_addr_t in struct resource.

Yes, we'd like to see the impact on vmlinux size, please.

Many of the problems will be in printks.  Whoever does this work should
capture stderr from before- and after- allmodconfig builds and diff them.

The appropriate way to printk a bus_addr_t will be

	printk("%lld", (unsigned long long)val);

which will increase code size even if bus_addr_t is u32, sadly.


Finally, we shouldn't just slavishly fix up great piles of printks to fix
the warnings (actually they're bugs).  Lots of the printks we have are
fairly useless, and there are other ways of finding out a device's IO and
IOMEM addresses anyway.

So IMO the preferred way of fixing a printk which is generating warnings is
to delete the thing.  Chances are that if the affected code actually has a
maintainer, he won't notice anyway ;)

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Wed, 15 Mar 2006 14:13:05 -0800

> ebiederm@xmission.com (Eric W. Biederman) wrote:
> >
> > Benjamin LaHaise <bcrl@kvack.org> writes:
> > 
> > > On Wed, Mar 15, 2006 at 02:29:32PM -0700, Eric W. Biederman wrote:
> > >> If the impact is very slight or unmeasurable this means the option
> > >> needs to fall under CONFIG_EMBEDDED, where you can change if
> > >> every last bit of RAM counts but otherwise you won't care.
> > >
> > > But we have a data type that is correct for this usage: dma_addr_t.
> > 
> > Well the name is wrong.  Because these are in general not DMA addresses,
> > but it may have the other desired properties.  So it may be
> > useable.
> 
> Yes, dma_addr_t does the right thing but has the wrong name.

No it doesn't.

It's 32-bit on Sparc64 because all DMA mappings go through
the IOMMU into a 32-bit window on PCI space.

But we do most certainly want to support full 64-bit BARs
in PCI devices on sparc64.

Re: [RFC][PATCH] Expanding the size of "start" and "end" field in "struct resource"

[Eric W. Biederman]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Wed, Mar 15, 2006 at 02:59:54PM -0700, Eric W. Biederman wrote:
>> Actually now that I think back there are machines with < 4GiB of ram
>> with 64bit pci BAR values.  It is more common to have 32bit values BAR
>> values and > 4GiB of ram.
>
> Such machines on x86 would have to be compiled with PAE.  Ditto any other 
> architecture, as you *have* to be able to represent those physical addresses, 
> which requires having page tables that can map them, which requires whatever 
> PAE is on the platform.

It depends on who the user is.  In the case that inspired this thread
user space can profit from the information even when the kernel can't.
In addition it appears kernel code quality can benefit as well.

>> Nor do I think struct resource is nearly as important when being
>> efficient, as dma_addr_t.  struct resource is only used during
>> driver setup which is a very rare event.  A few extra instructions
>> there likely will get lost in the noise and most of the will probably
>> be removed because they are in an __init section anyway.
>
> Bloat for no good reason is a bad habit to get into.

I agree bloat for no good reason is a bad habit to get into.
Premature optimization is a worse habit to get into, as is making
problems unnecessarily complex.  Until working patches are produced,
and the impact can be assessed it is to soon to seriously worry about
something that back of the napkin calculations indicate suggest
will have no measurable impact.

Eric