fix broken bootup on 32-bit
Subsystems (2)
PATCH 1/1 fix broken bootup on 32-bit
mmotm 2011-03-02-16-52 is utterly broken on 32-bit: panics at boot with "Couldn't register console driver", and preceding warnings don't even print their line numbers... which leads to the vsprintf changes.
Comparing the 32-bit version of put_dec() against mod7_put_dec() in tools/put-dec/put-dec-test.c (ah, it is useful after all!) shows that someone has decided to invert the ordering of pairs of lines in the kernel version of the algorithm, making a nonsense of it.
Switch them back, and put in the conditionals from mod7_put_dec() (absent from mod6_put_dec??): my guess is that less work is better.
My testing has gone no further than booting up, and checking that the numbers in /proc/partitions come out right (which they didn't at my first attempt, when I thought just a one-liner was needed).
---
Diffed to go on the top of mmotm, but can be applied after lib-vsprintf-optimised-put_dec-function-fix.patch
lib/vsprintf.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
lib/vsprintf.c +20 -13
@@ -310,22 +310,29 @@ char *put_dec(char *buf, unsigned long l q =656 * d3 +7296 * d2 +5536 * d1 + (n &0xFFFF ); - q = q /10000 ; buf =put_dec_full4 (buf, q %10000 ); + q = q /10000 ; d1 = q +7671 * d3 +9496 * d2 +6 * d1; - q = d1 /10000 ; - buf =put_dec_full4 (buf, d1 %10000 ); - - d2 = q +4749 * d3 +42 * d2; - q = d2 /10000 ; - buf =put_dec_full4 (buf, d2 %10000 ); - - d3 = q +281 * d3; - q = d3 /10000 ; - buf =put_dec_full4 (buf, d3 %10000 ); - - buf =put_dec_full4 (buf, q); +if (d1) { + buf =put_dec_full4 (buf, d1 %10000 ); + q = d1 /10000 ; + + d2 = q +4749 * d3 +42 * d2; +if (d2) { + buf =put_dec_full4 (buf, d2 %10000 ); + q = d2 /10000 ; + + d3 = q +281 * d3; +if (d3) { + buf =put_dec_full4 (buf, d3 %10000 ); + q = d3 /10000 ; + +if (q) + buf =put_dec_full4 (buf, q); + } + } + }while (buf[-1 ] =='0' ) --buf;
PATCH 1/1 lib: vsprintf: 32-bit put_dec() fixes
This commit fixes the 32-bit put_dec() function.
I have submitted by mistake an older version of the put_dec() patch with a bug in it (which had been spotted by Denys and fixed in subsequent version), which resulted in Hugh having to find the bug once again (after experiencing boot failure).
This commit fixes the bug once and for all and introduces some additional optimisations and comments (which were present in the fixed version of put_dec() patch).
--- lib/vsprintf.c | 49 ++++++++++++++++++++++++------------------------- 1 files changed, 24 insertions(+), 25 deletions(-)
Quoted 4 lines · Hugh Dickins
mmotm 2011-03-02-16-52 is utterly broken on 32-bit: panics at boot with "Couldn't register console driver", and preceding warnings don't even print their line numbers... which leads to the vsprintf changes.
As I suspected, I sent by mistake the v3 instead of v4 of the patch. I haven't spotted any problems because I was building kernel for my 32-bit machine from a branch with a fix but than sent patch from a branch for 64-bit machine (which was not affected by the bug).
Hugh proposed version with cascades of ifs. I think this is better version because it's smaller and benchmark are not clear which version is faster (on Intel version with ifs was faster but on ARM version without ifs was).
I pushed an unified version of the whole put_dec patch, rebased on v2.6.38-rc7, to github:
git://github.com/mina86/linux-2.6.git v2.6.38-rc7+put-dec
The attached patch is rebased on top of -mm (ie. just a delta).
For the sake of elegance, I would probably recommend taking the unified patch instead of what's already in -mm plus this or Hugh's fix.
Again, sorry about all the problems.
lib/vsprintf.c +24 -25
@@ -157,7 +157,7 @@ static noinline_for_stack char *put_dec_full5(char *buf, unsigned q)* without any branches. */ - r = (q * (uint64_t )0xcccd ) >>19 ; + r = (q * (uint64_t )0xcccd ) >>19 ; *buf++ = (q -10 * r) +'0' ;/*
@@ -226,7 +226,14 @@ static noinline_for_stack char *put_dec_trunc5(char *buf, unsigned q)return buf; } -/* No inlining helps gcc to use registers better */ +/* +* This function formats all integers correctly, however on 32-bit +* processors function below is used (not this one) which handles only +* non-zero integers. So be advised never to call this function with +* num == 0. +* +* No inlining helps gcc to use registers better +*/ static noinline_for_stack char *put_dec(char *buf, unsigned long long num) {
@@ -293,36 +300,36 @@ char *put_dec_8bit(char *buf, unsigned q)* permission from the author). This performs no 64-bit division and * hence should be faster on 32-bit machines then the version of the * function above. +* +* This function formats correctly all NON-ZERO integers. Passing +* zero makes daemons come out of your closet. This is OK, since +* number(), which calls this function, has a special case for zero +* anyways. */ static noinline_for_stack char *put_dec(char *buf, unsigned long long n) {uint32_t d3, d2, d1, q; -if (n <10 ) { - *buf++ ='0' + (unsigned )n; -return buf; - } - d1 = (n >>16 ) &0xFFFF ; d2 = (n >>32 ) &0xFFFF ; d3 = (n >>48 ) &0xFFFF ; - q =656 * d3 +7296 * d2 +5536 * d1 + (n &0xFFFF ); + q =656 * d3 +7296 * d2 +5536 * d1 + (n &0xFFFF ); - q = q /10000 ; buf =put_dec_full4 (buf, q %10000 ); + q = q /10000 ; d1 = q +7671 * d3 +9496 * d2 +6 * d1; - q = d1 /10000 ; + q = d1 /10000 ; buf =put_dec_full4 (buf, d1 %10000 ); d2 = q +4749 * d3 +42 * d2; - q = d2 /10000 ; + q = d2 /10000 ; buf =put_dec_full4 (buf, d2 %10000 ); d3 = q +281 * d3; - q = d3 /10000 ; + q = d3 /10000 ; buf =put_dec_full4 (buf, d3 %10000 ); buf =put_dec_full4 (buf, q);
@@ -407,22 +414,14 @@ char *number(char *buf, char *end, unsigned long long num, spec.field_width--; } } -if (need_pfx) { - spec.field_width--; -if (spec.base ==16 ) - spec.field_width--; - } +if (need_pfx) + spec.field_width -= spec.base /8 ;/* generate full string in tmp[], in reverse order */ i =0 ; -if (num ==0 ) - tmp[i++] ='0' ; -/* Generic code, for any base: -else do { -tmp[i++] = (digits[do_div(num,base)] | locase); -} while (num != 0); -*/ -else if (spec.base !=10 ) {/* 8 or 16 */ +if (num <8 ) { + tmp[i++] ='0' + (char )num; + }else if (spec.base !=10 ) {/* 8 or 16 */ int mask = spec.base -1 ;int shift =3 ;
Signature · 1 line
1.7.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Michal Nazarewicz
Quoted 4 lines · Hugh Dickins
mmotm 2011-03-02-16-52 is utterly broken on 32-bit: panics at boot with "Couldn't register console driver", and preceding warnings don't even print their line numbers... which leads to the vsprintf changes.
Sorry. I must have submitted the wrong version of the patch. I don't know how that could happen.
I don't know by what route they got into mmotm:
I can't find them on LKML since last August.
Quoted 9 lines
Quoted 5 lines
Comparing the 32-bit version of
put_dec()againstmod7_put_dec()intools/put-dec/put-dec-test.c(ah, it is useful after all!) shows that someone has decided to invert the ordering of pairs of lines in the kernel version of the algorithm, making a nonsense of it.Again, I'm at loss of how that happened. I remember Denis pointing this out though and me fixing it. In some strange turn of events I must have submitted version without the fix.
And what other surprises may have crept in there?
I think you need to get more signoffs from competent (not me!) people, and (at least for initial submission) a configurable test of the code that can actually be run at startup in the kernel.
I nearly called the tools/put-dec testing worthless, since what it's testing is divorced from and different from what's being run in the kernel. But that's unfair, it surely helped me to a booting system.
Quoted 20 lines
Quoted 20 lines
Switch them back, and put in the conditionals from
mod7_put_dec()(absent frommod6_put_dec??): my guess is that less work is better.My testing has gone no further than booting up, and checking that the numbers in
/proc/partitionscome out right (which they didn't at my first attempt, when I thought just a one-liner was needed).Signed-off-by Hugh Dickins <[email protected]>---
Diffed to go on the top of mmotm, but can be applied after lib-vsprintf-optimised-put_dec-function-fix.patchlib/vsprintf.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 renamed from lib/vsprintf.c 2011-03-03 15:43:57.000000000 -0800 +0
-0@@ -310,22 +310,29 @@ char *put_dec(char *buf, unsigned long l
Quoted 7 lines
lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 char *put_dec(char *buf, unsigned long l · line 314
Quoted 5 lines
lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 renamed from lib/vsprintf.c 2011-03-03 15:43:57.000000000 -0800 +1
-1q = 656 * d3 + 7296 * d2 + 5536 * d1 + (n & 0xFFFF); - q = q / 10000; buf = put_dec_full4(buf, q % 10000); + q = q / 10000;This should be enough.
Looking at it again this morning, yes, that's the only reordering that matters, the rest just makes it more recognizably like what's tested.
Quoted 35 lines
Quoted 35 lines
lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 renamed from lib/vsprintf.c 2011-03-03 15:43:57.000000000 -0800 +19
-12d1 = q + 7671 * d3 + 9496 * d2 + 6 * d1; - q = d1 / 10000; - buf = put_dec_full4(buf, d1 % 10000); - - d2 = q + 4749 * d3 + 42 * d2; - q = d2 / 10000; - buf = put_dec_full4(buf, d2 % 10000); - - d3 = q + 281 * d3; - q = d3 / 10000; - buf = put_dec_full4(buf, d3 % 10000); - - buf = put_dec_full4(buf, q); + if (d1) { + buf = put_dec_full4(buf, d1 % 10000); + q = d1 / 10000; + + d2 = q + 4749 * d3 + 42 * d2; + if (d2) { + buf = put_dec_full4(buf, d2 % 10000); + q = d2 / 10000; + + d3 = q + 281 * d3; + if (d3) { + buf = put_dec_full4(buf, d3 % 10000); + q = d3 / 10000; + + if (q) + buf = put_dec_full4(buf, q); + } + } + }
Quoted 6 lines
lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 char *put_dec(char *buf, unsigned long l · line 337
Quoted 2 lines
lib/vsprintf.c 2011-03-05 03:37:14.000000000 -0800 renamed from lib/vsprintf.c 2011-03-03 15:43:57.000000000 -0800 +0
-0while (buf[-1] == '0') --buf;Note that this loop handles zeros at the beginning so it's not necessary to add the ifs. As I've said, swapping the two lines at the beginning will be enough, and I would recommend doing only that.
I realize that zeroes are handled, but I was imagining that one branch taken (for numbers up to 9999) is cheaper than four out-of-line function calls, six divisions-or-modulos by constant 10000, three multiplications by constants; oh, and a lot more once I look inside put_dec_full4().
Is that not the case? Isn't performance the justification for this magic?
Quoted 2 lines
Once again, sorry about the trouble.
Hugh