Re: 2.5.75 as-iosched.c & asm-generic/div64.h breakage

Re: 2.5.75 as-iosched.c & asm-generic/div64.h breakage

[Mikael Pettersson]

On Fri, 11 Jul 2003 14:01:58 -0700, Andrew Morton wrote:
> Mikael Pettersson <mikpe@csd.uu.se> 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

Re: 2.5.75 as-iosched.c & asm-generic/div64.h breakage (PATCH)

[Bernardo Innocenti]

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