[PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Mel Gorman]

(Resending with Andrew's email address correct this time)

For the last few -mm releases, kernels built for an old RS6000 failed to
compile with the message;

arch/powerpc/kernel/built-in.o(.init.text+0x77b4): In function `vrsqrtefp':
: undefined reference to `__udivdi3'
arch/powerpc/kernel/built-in.o(.init.text+0x7800): In function `vrsqrtefp':
: undefined reference to `__udivdi3'
make: *** [.tmp_vmlinux1] Error 1

2.6.17-rc5 is not affected but I didn't search for the culprit patch in
-mm. The following patch adds an implementation of __udivdi3 for plain old
ppc32. This may not be the correct fix as Google tells me that __udivdi3
has been replaced by calls to do_div() in a number of cases. There was no
obvious way to do that for vrsqrtefp, hence this workaround. The patch should
be acked, rejected or replaced by a ppc expert.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-rc5-clean/arch/powerpc/lib/Makefile linux-2.6.17-rc5-udivdi3_ppc/arch/powerpc/lib/Makefile
--- linux-2.6.17-rc5-clean/arch/powerpc/lib/Makefile	2006-05-25 02:50:17.000000000 +0100
+++ linux-2.6.17-rc5-udivdi3_ppc/arch/powerpc/lib/Makefile	2006-05-26 13:17:15.000000000 +0100
@@ -4,7 +4,7 @@
 
 ifeq ($(CONFIG_PPC_MERGE),y)
 obj-y			:= string.o strcase.o
-obj-$(CONFIG_PPC32)	+= div64.o copy_32.o checksum_32.o
+obj-$(CONFIG_PPC32)	+= div64.o copy_32.o checksum_32.o udivdi3.o
 endif
 
 obj-y			+= bitops.o
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-rc5-clean/arch/powerpc/lib/udivdi3.c linux-2.6.17-rc5-udivdi3_ppc/arch/powerpc/lib/udivdi3.c
--- linux-2.6.17-rc5-clean/arch/powerpc/lib/udivdi3.c	2006-05-26 13:17:22.000000000 +0100
+++ linux-2.6.17-rc5-udivdi3_ppc/arch/powerpc/lib/udivdi3.c	2006-05-26 13:25:26.000000000 +0100
@@ -0,0 +1,15 @@
+/*
+ * Simple __udivdi3 function which doesn't use FPU.
+ */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <asm-generic/div64.h>
+
+u64 __udivdi3(u64 n, u64 d)
+{
+	if (d & ~0xffffffff)
+		panic("Need true 64-bit/64-bit division");
+	return __div64_32(&n, (u32)d);
+}
+
-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Andrew Morton]

mel@csn.ul.ie (Mel Gorman) wrote:
>
> (Resending with Andrew's email address correct this time)
> 
>  For the last few -mm releases, kernels built for an old RS6000 failed to
>  compile with the message;
> 
>  arch/powerpc/kernel/built-in.o(.init.text+0x77b4): In function `vrsqrtefp':
>  : undefined reference to `__udivdi3'
>  arch/powerpc/kernel/built-in.o(.init.text+0x7800): In function `vrsqrtefp':
>  : undefined reference to `__udivdi3'
>  make: *** [.tmp_vmlinux1] Error 1

A function with a name like that doesn't _deserve_ to compile.

But actually vrsqrtefp() doesn't call __udivdi3 - the error lies somewhere
else in the kernel and the toolchain gets it wrong, so we don't know where.

The way I usually hunt this problem down is to build the .s files (make
arch/powerpc/kernel/foo.s) and then grep around, find the offending C
function.

If the problem is specific to powerpc then a

	diffstat 2.6.17.rc4-mm3 | grep powerpc

will narrow down the number of files to be searched by rather a lot.

>  2.6.17-rc5 is not affected but I didn't search for the culprit patch in
>  -mm. The following patch adds an implementation of __udivdi3 for plain old
>  ppc32. This may not be the correct fix as Google tells me that __udivdi3
>  has been replaced by calls to do_div() in a number of cases. There was no
>  obvious way to do that for vrsqrtefp, hence this workaround. The patch should
>  be acked, rejected or replaced by a ppc expert.

Yes, we've traditionally avoided adding the 64-bit divide library functions.

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Mel Gorman]

On (26/05/06 09:49), Andrew Morton didst pronounce:
> mel@csn.ul.ie (Mel Gorman) wrote:
> >
> > (Resending with Andrew's email address correct this time)
> > 
> >  For the last few -mm releases, kernels built for an old RS6000 failed to
> >  compile with the message;
> > 
> >  arch/powerpc/kernel/built-in.o(.init.text+0x77b4): In function `vrsqrtefp':
> >  : undefined reference to `__udivdi3'
> >  arch/powerpc/kernel/built-in.o(.init.text+0x7800): In function `vrsqrtefp':
> >  : undefined reference to `__udivdi3'
> >  make: *** [.tmp_vmlinux1] Error 1
> 
> A function with a name like that doesn't _deserve_ to compile.
> 

heh

> But actually vrsqrtefp() doesn't call __udivdi3 - the error lies somewhere
> else in the kernel and the toolchain gets it wrong, so we don't know where.
> 

It explains why I couldn't find where vrsqrtefp() was.

> The way I usually hunt this problem down is to build the .s files (make
> arch/powerpc/kernel/foo.s) and then grep around, find the offending C
> function.
> 

That was a good idea, thanks. It showed that check_for_io_childs() in
arch/powerpc/kernel/pci_32.c was the real problem. A quick look showed
that is was doing divisions on resource_type_t which was 64 bits with my
.config. Selecting CONFIG_RESOURCES_32BIT made the problem go away. This
option is introduced by the kconfigurable-resources-* patches so I've cc'd
Vivek Goyal to comment on the potential fixes below.

> >  2.6.17-rc5 is not affected but I didn't search for the culprit patch in
> >  -mm. The following patch adds an implementation of __udivdi3 for plain old
> >  ppc32. This may not be the correct fix as Google tells me that __udivdi3
> >  has been replaced by calls to do_div() in a number of cases. There was no
> >  obvious way to do that for vrsqrtefp, hence this workaround. The patch should
> >  be acked, rejected or replaced by a ppc expert.
> 
> Yes, we've traditionally avoided adding the 64-bit divide library functions.

Understandable. The fixes are obvious in this case. Easiest is for users to
set CONFIG_RESOURCES_32BIT manually although the kernel build error is not
very obvious. A fairly easy bodge is to default CONFIG_RESOURCES_32BIT to Y
when building PPC32 but a determined user may still fail to build a kernel. A
slightly riskier option is this patch that replaces a potential 64 bit divide
with a do_div call. The patch has been successfully boot-tested on a RS6000.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-rc4-mm3-clean/arch/powerpc/kernel/pci_32.c linux-2.6.17-rc4-mm3-resources-32/arch/powerpc/kernel/pci_32.c
--- linux-2.6.17-rc4-mm3-clean/arch/powerpc/kernel/pci_32.c	2006-05-29 15:55:52.000000000 +0100
+++ linux-2.6.17-rc4-mm3-resources-32/arch/powerpc/kernel/pci_32.c	2006-05-29 15:53:43.000000000 +0100
@@ -1115,7 +1115,9 @@ check_for_io_childs(struct pci_bus *bus,
 	int	rc = 0;
 
 #define push_end(res, size) do { unsigned long __sz = (size) ; \
-	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
+	resource_size_t farEnd = (res->end + __sz); \
+	do_div(farEnd, (__sz + 1)); \
+	res->end = farEnd * (__sz + 1) + __sz; \
     } while (0)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Segher Boessenkool]

>>>  arch/powerpc/kernel/built-in.o(.init.text+0x77b4): In function 
>>> `vrsqrtefp':
>>>  : undefined reference to `__udivdi3'
>>>  arch/powerpc/kernel/built-in.o(.init.text+0x7800): In function 
>>> `vrsqrtefp':
>>>  : undefined reference to `__udivdi3'
>>>  make: *** [.tmp_vmlinux1] Error 1
>>
>> A function with a name like that doesn't _deserve_ to compile.

Would vector_reciprocal_square_root_estimate_floating_point() be any 
better...
Anyway, this is just a machine insn mnemonic, so the function name is 
fine
I believe.

>  #define push_end(res, size) do { unsigned long __sz = (size) ; \
> -	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
> +	resource_size_t farEnd = (res->end + __sz); \
> +	do_div(farEnd, (__sz + 1)); \
> +	res->end = farEnd * (__sz + 1) + __sz; \
>      } while (0)

Size here is a) a misnomer (size + 1 is the actual size) and b) always 
a power
of two minus one.  So instead, do

#define push_end(res, mask) res->end = -(-res->end & ~(unsigned 
long)mask)

(with a do { } while(0) armour if you prefer).


Segher

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Mel Gorman]

On Mon, 29 May 2006, Segher Boessenkool wrote:

>>>>  arch/powerpc/kernel/built-in.o(.init.text+0x77b4): In function 
>>>> `vrsqrtefp':
>>>>  : undefined reference to `__udivdi3'
>>>>  arch/powerpc/kernel/built-in.o(.init.text+0x7800): In function 
>>>> `vrsqrtefp':
>>>>  : undefined reference to `__udivdi3'
>>>>  make: *** [.tmp_vmlinux1] Error 1
>>> 
>>> A function with a name like that doesn't _deserve_ to compile.
>
> Would vector_reciprocal_square_root_estimate_floating_point() be any 
> better...
> Anyway, this is just a machine insn mnemonic, so the function name is fine
> I believe.
>
>>  #define push_end(res, size) do { unsigned long __sz = (size) ; \
>> -	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
>> +	resource_size_t farEnd = (res->end + __sz); \
>> +	do_div(farEnd, (__sz + 1)); \
>> +	res->end = farEnd * (__sz + 1) + __sz; \
>>      } while (0)
>
> Size here is a) a misnomer (size + 1 is the actual size) and b) always a 
> power
> of two minus one.  So instead, do
>
> #define push_end(res, mask) res->end = -(-res->end & ~(unsigned long)mask)
>
> (with a do { } while(0) armour if you prefer).
>

It's not doing the same as the old code so is your suggested fix a correct 
replacement?

For example, given 0xfff for size the current code rounds res->end to the 
next 0x1000 boundary and adds 0xfff. Your propose fix just rounds it to 
the next 0x1000 if I'm reading it correctly but is what the code was meant 
to do in the first place? Using masks, the equivilant of the current code 
is something like;

#define push_end(res, mask) do { \
 	res->end = -(-res->end & ~(unsigned long)mask); \
 	res->end += mask; \
} while (0)

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Segher Boessenkool]

>>>  #define push_end(res, size) do { unsigned long __sz = (size) ; \
>>> -	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
>>> +	resource_size_t farEnd = (res->end + __sz); \
>>> +	do_div(farEnd, (__sz + 1)); \
>>> +	res->end = farEnd * (__sz + 1) + __sz; \
>>>      } while (0)
>>
>> Size here is a) a misnomer (size + 1 is the actual size) and b) 
>> always a power
>> of two minus one.  So instead, do
>>
>> #define push_end(res, mask) res->end = -(-res->end & ~(unsigned 
>> long)mask)
>>
>> (with a do { } while(0) armour if you prefer).
>>
>
> It's not doing the same as the old code so is your suggested fix a 
> correct replacement?
>
> For example, given 0xfff for size the current code rounds res->end to 
> the next 0x1000 boundary and adds 0xfff. Your propose fix just rounds 
> it to the next 0x1000 if I'm reading it correctly but is what the code 
> was meant to do in the first place? Using masks, the equivilant of the 
> current code is something like;
>
> #define push_end(res, mask) do { \
> 	res->end = -(-res->end & ~(unsigned long)mask); \
> 	res->end += mask; \
> } while (0)

Yeah forgot a bit, this looks fine.


Segher

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Mel Gorman]

On (29/05/06 19:56), Segher Boessenkool didst pronounce:
> >>> #define push_end(res, size) do { unsigned long __sz = (size) ; \
> >>>-	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
> >>>+	resource_size_t farEnd = (res->end + __sz); \
> >>>+	do_div(farEnd, (__sz + 1)); \
> >>>+	res->end = farEnd * (__sz + 1) + __sz; \
> >>>     } while (0)
> >>
> >>Size here is a) a misnomer (size + 1 is the actual size) and b) 
> >>always a power
> >>of two minus one.  So instead, do
> >>
> >>#define push_end(res, mask) res->end = -(-res->end & ~(unsigned 
> >>long)mask)
> >>
> >>(with a do { } while(0) armour if you prefer).
> >>
> >
> >It's not doing the same as the old code so is your suggested fix a 
> >correct replacement?
> >
> >For example, given 0xfff for size the current code rounds res->end to 
> >the next 0x1000 boundary and adds 0xfff. Your propose fix just rounds 
> >it to the next 0x1000 if I'm reading it correctly but is what the code 
> >was meant to do in the first place? Using masks, the equivilant of the 
> >current code is something like;
> >
> >#define push_end(res, mask) do { \
> >	res->end = -(-res->end & ~(unsigned long)mask); \
> >	res->end += mask; \
> >} while (0)
> 
> Yeah forgot a bit, this looks fine.
> 

Ok. The following patch has been successfully boot tested on the same machine
with 64 bit resource_size_t. Because the mask is assumed to be a
power-of-two - 1, I added a comment and a BUG_ON() to be sure.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-rc4-mm3-clean/arch/powerpc/kernel/pci_32.c linux-2.6.17-rc4-mm3-resources-32/arch/powerpc/kernel/pci_32.c
--- linux-2.6.17-rc4-mm3-clean/arch/powerpc/kernel/pci_32.c	2006-05-29 15:55:52.000000000 +0100
+++ linux-2.6.17-rc4-mm3-resources-32/arch/powerpc/kernel/pci_32.c	2006-05-29 19:09:36.000000000 +0100
@@ -1114,8 +1114,16 @@ check_for_io_childs(struct pci_bus *bus,
 	int	i;
 	int	rc = 0;
 
-#define push_end(res, size) do { unsigned long __sz = (size) ; \
-	res->end = ((res->end + __sz) / (__sz + 1)) * (__sz + 1) + __sz; \
+	/*
+	 * Assuming mask is a power of two - 1, push_end
+	 * moves res->end to the end of the next
+	 * mask-aligned boundary.
+	 * e.g. res->end of 0x1fff moves to 0x2fff
+	 */
+#define push_end(res, mask) do { \
+	BUG_ON( ((mask+1) & mask) != 0); \
+	res->end = -(-res->end & ~(unsigned long)mask); \
+	res->end += mask; \
     } while (0)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Paul Mackerras]

Mel Gorman writes:

> +	res->end = -(-res->end & ~(unsigned long)mask); \
> +	res->end += mask; \

I think this is equivalent to

	res->end = (res->end + mask) | mask;

and I have to say the latter seems more understandable to me (and
doesn't need a cast) ...

Regards,
Paul.

Re: [PATCH] Compile failure fix for ppc on 2.6.17-rc4-mm3 (2nd attempt)

[Mel Gorman]

On (09/06/06 19:36), Paul Mackerras didst pronounce:
> Mel Gorman writes:
> 
> > +	res->end = -(-res->end & ~(unsigned long)mask); \
> > +	res->end += mask; \
> 
> I think this is equivalent to
> 
> 	res->end = (res->end + mask) | mask;
> 
> and I have to say the latter seems more understandable to me (and
> doesn't need a cast) ...
> 

Makes sense. The patch on top of the latest -mm is below if you want to push
it. It has not been boot tested as building with the latest -mm is broken
at the moment for older powerpcs because of the git-powerpc.patch patch .
That patch assumes that mm_context_t is a struct with a vdso_base which is
not always the case (it's an unsigned long in include/asm-ppc/mmu.h).

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-rc6-mm2-clean/arch/powerpc/kernel/pci_32.c linux-2.6.17-rc6-mm2-resources/arch/powerpc/kernel/pci_32.c
--- linux-2.6.17-rc6-mm2-clean/arch/powerpc/kernel/pci_32.c	2006-06-13 17:25:39.000000000 +0100
+++ linux-2.6.17-rc6-mm2-resources/arch/powerpc/kernel/pci_32.c	2006-06-13 17:30:20.000000000 +0100
@@ -1121,9 +1121,8 @@ check_for_io_childs(struct pci_bus *bus,
 	 * e.g. res->end of 0x1fff moves to 0x2fff
 	 */
 #define push_end(res, mask) do {				\
-	BUG_ON(((mask+1) & mask) != 0);			\
-	res->end = -(-res->end & ~(unsigned long)mask);		\
-	res->end += mask;					\
+	BUG_ON(((mask+1) & mask) != 0);				\
+	res->end = (res->end + mask) | mask;			\
     } while (0)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab