Compiling 2.5.75 for PPC32 results in:
gcc -Wp,-MD,drivers/block/.as-iosched.o.d -D__KERNEL__ -Iinclude -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fno-strict-aliasing -fno-common -Iarch/ppc -msoft-float -pipe -ffixed-r2 -Wno-uninitialized -mmultiple -mstring -fomit-frame-pointer -nostdinc -iwithprefix include -DKBUILD_BASENAME=as_iosched -DKBUILD_MODNAME=as_iosched -c -o drivers/block/as-iosched.o drivers/block/as-iosched.c
drivers/block/as-iosched.c: In function `as_update_iohist':
drivers/block/as-iosched.c:840: warning: right shift count >= width of type
drivers/block/as-iosched.c:840: warning: passing arg 1 of `__div64_32' from incompatible pointer type
The statement is "do_div(aic->seek_mean, aic->seek_samples);".
There are two problems here:
- aic->seek_mean is a sector_t, but this type is usually only u32 on 32-bitters,
unless CONFIG_LBD is set. Thus, as-iosched.c often passes a u32 lvalue to
do_div() where a u64 lvalue is expected.
- include/asm-generic/div64.h, which is what several 32-bit archs use now, is
unsafe when its first parameter "n" is a u32 lvalue:
* The "((n) >> 32) == 0" in the initial test invokes undefined behaviour.
We want it to be false, but this isn't guaranteed.
* The call "__div64_32(&(n), __base)" passes a u32* to a function
expecting a u64*. Major breakage.
x86 survives this because it's custom do_div() implicitly casts "n" to u64
before pulling it apart, and updates "n" with an assignment from a u64, so
the compiler can compensate when "n" is a u32. asm-generic/div64.h should
do something similar, IMO.
/Mikael
Mikael Pettersson <[email protected]> wrote:
>
> drivers/block/as-iosched.c: In function `as_update_iohist':
> drivers/block/as-iosched.c:840: warning: right shift count >= width of type
> drivers/block/as-iosched.c:840: warning: passing arg 1 of `__div64_32' from incompatible pointer type
You mean that code was in -mm for all those months and no ppc32 person
bothered testing it? Bah.
Something like this? (Could be sped up for 32-bit sector_t)
diff -puN drivers/block/as-iosched.c~as-do_div-fix drivers/block/as-iosched.c
--- 25/drivers/block/as-iosched.c~as-do_div-fix Fri Jul 11 14:00:55 2003
+++ 25-akpm/drivers/block/as-iosched.c Fri Jul 11 14:00:58 2003
@@ -836,8 +836,10 @@ static void as_update_iohist(struct as_i
aic->seek_samples += 256;
aic->seek_total += 256*seek_dist;
if (aic->seek_samples) {
- aic->seek_mean = aic->seek_total + 128;
- do_div(aic->seek_mean, aic->seek_samples);
+ u64 seek_mean = aic->seek_total + 128;
+
+ do_div(seek_mean, aic->seek_samples);
+ aic->seek_mean = seek_mean;
}
aic->seek_samples = (aic->seek_samples>>1)
+ (aic->seek_samples>>2);
_
On Fri, 11 Jul 2003 14:01:58 -0700, Andrew Morton wrote:
> Mikael Pettersson <[email protected]> wrote:
> >
> > drivers/block/as-iosched.c: In function `as_update_iohist':
> > drivers/block/as-iosched.c:840: warning: right shift count >= width of type
> > drivers/block/as-iosched.c:840: warning: passing arg 1 of `__div64_32' from incompatible pointer type
>
> You mean that code was in -mm for all those months and no ppc32 person
> bothered testing it? Bah.
Apparently. I don't have time for anything but standard kernels.
> Something like this? (Could be sped up for 32-bit sector_t)
>
> diff -puN drivers/block/as-iosched.c~as-do_div-fix drivers/block/as-iosched.c
> --- 25/drivers/block/as-iosched.c~as-do_div-fix Fri Jul 11 14:00:55 2003
> +++ 25-akpm/drivers/block/as-iosched.c Fri Jul 11 14:00:58 2003
> @@ -836,8 +836,10 @@ static void as_update_iohist(struct as_i
> aic->seek_samples += 256;
> aic->seek_total += 256*seek_dist;
> if (aic->seek_samples) {
> - aic->seek_mean = aic->seek_total + 128;
> - do_div(aic->seek_mean, aic->seek_samples);
> + u64 seek_mean = aic->seek_total + 128;
> +
> + do_div(seek_mean, aic->seek_samples);
> + aic->seek_mean = seek_mean;
> }
> aic->seek_samples = (aic->seek_samples>>1)
> + (aic->seek_samples>>2);
Perhaps, but it's my opinion that do_div() needs to be made more robust,
since arch-specific data abstraction means that callers in generic code
don't always know if some value is u32 or u64.
So your change should be in do_div() itself, not in its callers.
/Mikael
Andrew Morton writes:
> Mikael Pettersson <[email protected]> wrote:
> >
> > drivers/block/as-iosched.c: In function `as_update_iohist':
> > drivers/block/as-iosched.c:840: warning: right shift count >= width of type
> > drivers/block/as-iosched.c:840: warning: passing arg 1 of `__div64_32' from incompatible pointer type
>
> You mean that code was in -mm for all those months and no ppc32 person
> bothered testing it? Bah.
We've only been getting those warnings since the changeover to the new
asm-generic/div64.h. Settle down. :)
> Something like this? (Could be sped up for 32-bit sector_t)
>
> diff -puN drivers/block/as-iosched.c~as-do_div-fix drivers/block/as-iosched.c
> --- 25/drivers/block/as-iosched.c~as-do_div-fix Fri Jul 11 14:00:55 2003
> +++ 25-akpm/drivers/block/as-iosched.c Fri Jul 11 14:00:58 2003
> @@ -836,8 +836,10 @@ static void as_update_iohist(struct as_i
> aic->seek_samples += 256;
> aic->seek_total += 256*seek_dist;
> if (aic->seek_samples) {
> - aic->seek_mean = aic->seek_total + 128;
> - do_div(aic->seek_mean, aic->seek_samples);
> + u64 seek_mean = aic->seek_total + 128;
> +
> + do_div(seek_mean, aic->seek_samples);
> + aic->seek_mean = seek_mean;
> }
There are several interesting aspects to this:
1. For some reason the LBD config option in drivers/block/Kconfig
depends on X86. (CONFIG_LBD is what makes sector_t an unsigned
long long instead of an unsigned long). I think the LBD option
should be available on all 32-bit platforms. Working out a neat
way to tell the config system that is left as an exercise for the
reader. :)
2. It seems to me that seek_total is bounded above by 1024 * the
maximum seek distance. I'm a bit concerned about that overflowing
32 bits - AFAICS I would only need a > 2GB disk (or do I mean
partition?) for that to happen. Could we make seek_total be a u64
unconditionally please?
3. I guess that adding 128 on to the seek_total is to get a
round-to-nearest effect in the division. In fact you need to add
on (seek_samples >> 1) to get that effect.
Regards,
Paul.
Hi!
Paul Mackerras wrote:
>Andrew Morton writes:
>
>
>>Mikael Pettersson <[email protected]> wrote:
>>
>>>drivers/block/as-iosched.c: In function `as_update_iohist':
>>>drivers/block/as-iosched.c:840: warning: right shift count >= width of type
>>>drivers/block/as-iosched.c:840: warning: passing arg 1 of `__div64_32' from incompatible pointer type
>>>
>>You mean that code was in -mm for all those months and no ppc32 person
>>bothered testing it? Bah.
>>
>
>We've only been getting those warnings since the changeover to the new
>asm-generic/div64.h. Settle down. :)
>
>
>>Something like this? (Could be sped up for 32-bit sector_t)
>>
>>diff -puN drivers/block/as-iosched.c~as-do_div-fix drivers/block/as-iosched.c
>>--- 25/drivers/block/as-iosched.c~as-do_div-fix Fri Jul 11 14:00:55 2003
>>+++ 25-akpm/drivers/block/as-iosched.c Fri Jul 11 14:00:58 2003
>>@@ -836,8 +836,10 @@ static void as_update_iohist(struct as_i
>> aic->seek_samples += 256;
>> aic->seek_total += 256*seek_dist;
>> if (aic->seek_samples) {
>>- aic->seek_mean = aic->seek_total + 128;
>>- do_div(aic->seek_mean, aic->seek_samples);
>>+ u64 seek_mean = aic->seek_total + 128;
>>+
>>+ do_div(seek_mean, aic->seek_samples);
>>+ aic->seek_mean = seek_mean;
>> }
>>
>
>There are several interesting aspects to this:
>
>1. For some reason the LBD config option in drivers/block/Kconfig
> depends on X86. (CONFIG_LBD is what makes sector_t an unsigned
> long long instead of an unsigned long). I think the LBD option
> should be available on all 32-bit platforms. Working out a neat
> way to tell the config system that is left as an exercise for the
> reader. :)
>
Not me :P
>
>2. It seems to me that seek_total is bounded above by 1024 * the
> maximum seek distance. I'm a bit concerned about that overflowing
> 32 bits - AFAICS I would only need a > 2GB disk (or do I mean
> partition?) for that to happen. Could we make seek_total be a u64
> unconditionally please?
>
Probably a good idea. It is only the seek distance for one
process though. But it wouldn't hurt.
>
>3. I guess that adding 128 on to the seek_total is to get a
> round-to-nearest effect in the division. In fact you need to add
> on (seek_samples >> 1) to get that effect.
>
You're right. Thanks.
On Friday 11 July 2003 23:36, Mikael Pettersson wrote:
> Perhaps, but it's my opinion that do_div() needs to be made more
> robust, since arch-specific data abstraction means that callers in
> generic code don't always know if some value is u32 or u64.
>
> So your change should be in do_div() itself, not in its callers.
Even if this sector_t case is to be done with sector_div(), I agree
that more type safety in do_div() would be a desidearable feature.
We can't turn do_div() into an inline, so we must resort to the
same "useless pointer compare" trick also employed by the min() and max()
macros. Actually, this change immediately helps finding few places where
do_div() was being used incorrectly.
I've tested on both m68knommu and i386 (replacing asm-i386/div64.h with
asm-generic/div64.h). 64bit platforms are unaffected by this patch.
Linus, please apply.
--------------------------------------------------------------------------
- __div64_32(): remove __attribute_pure__ qualifier from the prototype
since this function obviously clobbers memory through &(n);
- do_div(): add a check to ensure (n) is type-compatible with uint64_t;
- as_update_iohist(): Use sector_div() instead of do_div().
(Whether the result of the addition should always be stored in 64bits
regardless of CONFIG_LBD is still being discussed, therefore it's
unadderessed here);
- Fix all places where do_div() was being called with a bad divisor argument.
diff -Nru linux-2.5.75.orig/include/asm-generic/div64.h linux-2.5.75/include/asm-generic/div64.h
--- linux-2.5.75.orig/include/asm-generic/div64.h 2003-07-10 22:14:11.000000000 +0200
+++ linux-2.5.75/include/asm-generic/div64.h 2003-07-12 13:18:18.000000000 +0200
@@ -32,11 +32,15 @@
#elif BITS_PER_LONG == 32
-extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor) __attribute_pure__;
+extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
+/* The unnecessary pointer compare is there
+ * to check for type safety (n must be 64bit)
+ */
# define do_div(n,base) ({ \
uint32_t __base = (base); \
uint32_t __rem; \
+ (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
if (likely(((n) >> 32) == 0)) { \
__rem = (uint32_t)(n) % __base; \
(n) = (uint32_t)(n) / __base; \
diff -Nru linux-2.5.75.orig/drivers/block/as-iosched.c linux-2.5.75/drivers/block/as-iosched.c
--- linux-2.5.75.orig/drivers/block/as-iosched.c 2003-07-10 22:04:00.000000000 +0200
+++ linux-2.5.75/drivers/block/as-iosched.c 2003-07-12 13:28:01.000000000 +0200
@@ -837,7 +837,7 @@
aic->seek_total += 256*seek_dist;
if (aic->seek_samples) {
aic->seek_mean = aic->seek_total + 128;
- do_div(aic->seek_mean, aic->seek_samples);
+ sector_div(aic->seek_mean, aic->seek_samples);
}
aic->seek_samples = (aic->seek_samples>>1)
+ (aic->seek_samples>>2);
diff -Nru linux-2.5.75.orig/lib/vsprintf.c linux-2.5.75/lib/vsprintf.c
--- linux-2.5.75.orig/lib/vsprintf.c 2003-07-10 22:14:14.000000000 +0200
+++ linux-2.5.75/lib/vsprintf.c 2003-07-12 13:37:04.000000000 +0200
@@ -127,7 +127,7 @@
#define SPECIAL 32 /* 0x */
#define LARGE 64 /* use 'ABCDEF' instead of 'abcdef' */
-static char * number(char * buf, char * end, long long num, int base, int size, int precision, int type)
+static char * number(char * buf, char * end, unsigned long long num, int base, int size, int precision, int type)
{
char c,sign,tmp[66];
const char *digits;
diff -Nru linux-2.5.75.orig/mm/vmscan.c linux-2.5.75/mm/vmscan.c
--- linux-2.5.75.orig/mm/vmscan.c 2003-07-10 22:05:27.000000000 +0200
+++ linux-2.5.75/mm/vmscan.c 2003-07-12 13:20:29.000000000 +0200
@@ -147,7 +147,7 @@
pages = nr_used_zone_pages();
list_for_each_entry(shrinker, &shrinker_list, list) {
- long long delta;
+ uint64_t delta;
delta = scanned * shrinker->seeks;
delta *= (*shrinker->shrinker)(0, gfp_mask);
--
// Bernardo Innocenti - Develer S.r.l., R&D dept.
\X/ http://www.develer.com/
Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html
>>
>> 1. For some reason the LBD config option in drivers/block/Kconfig
>> depends on X86. (CONFIG_LBD is what makes sector_t an unsigned
>> long long instead of an unsigned long). I think the LBD option
>> should be available on all 32-bit platforms. Working out a neat
>> way to tell the config system that is left as an exercise for the
>> reader. :)
>>
Nick> Not me :P
No 'twas me --- because I couldn't work oput any cvlean way to do it,
and, of the 32-bit plaftforms, I could test only x86.
--
Dr Peter Chubb http://www.gelato.unsw.edu.au peterc AT gelato.unsw.edu.au
You are lost in a maze of BitKeeper repositories, all slightly different.