do_div vs sector_t

do_div vs sector_t

[Matthew Wilcox]

# define do_div(n,base) ({                              \
        uint32_t __base = (base);                       \
        uint32_t __rem;                                 \
        if (likely(((n) >> 32) == 0)) {                 \

so if we call do_div() on a u32, the compiler emits nasal daemons.
and we do this -- in the antcipatory scheduler:

                if (aic->seek_samples) {
                        aic->seek_mean = aic->seek_total + 128;
                        do_div(aic->seek_mean, aic->seek_samples);
                }

seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit platform.
so we can't avoid calling do_div().

This almost works (the warning is harmless since gcc optimises away the call)

# define do_div(n,base) ({                                              \
        uint32_t __base = (base);                                       \
        uint32_t __rem;                                                 \
        if ((sizeof(n) < 8) || (likely(((n) >> 32) == 0))) {            \
                __rem = (uint32_t)(n) % __base;                         \
                (n) = (uint32_t)(n) / __base;                           \
        } else                                                          \
                __rem = __div64_32(&(n), __base);                       \
        __rem;                                                          \
 })

Better ideas?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: do_div vs sector_t

[Andrew Morton]

Matthew Wilcox <willy@debian.org> wrote:
>
> This almost works (the warning is harmless since gcc optimises away the call)
> 
> # define do_div(n,base) ({                                              \
>         uint32_t __base = (base);                                       \
>         uint32_t __rem;                                                 \
>         if ((sizeof(n) < 8) || (likely(((n) >> 32) == 0))) {            \
>                 __rem = (uint32_t)(n) % __base;                         \
>                 (n) = (uint32_t)(n) / __base;                           \
>         } else                                                          \
>                 __rem = __div64_32(&(n), __base);                       \
>         __rem;                                                          \
>  })

Could we just do:

diff -puN include/asm-generic/div64.h~do_div-fix-43 include/asm-generic/div64.h
--- 25/include/asm-generic/div64.h~do_div-fix-43	Fri Jul 11 15:32:33 2003
+++ 25-akpm/include/asm-generic/div64.h	Fri Jul 11 15:33:26 2003
@@ -34,7 +34,8 @@
 
 extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
 
-# define do_div(n,base) ({				\
+# define do_div(_n,base) ({				\
+	uint64_t n = _n;				\
 	uint32_t __base = (base);			\
 	uint32_t __rem;					\
 	if (likely(((n) >> 32) == 0)) {			\
@@ -42,6 +43,7 @@ extern uint32_t __div64_32(uint64_t *div
 		(n) = (uint32_t)(n) / __base;		\
 	} else 						\
 		__rem = __div64_32(&(n), __base);	\
+	_n = n;						\
 	__rem;						\
  })
 

_

Re: do_div vs sector_t

[Neil Brown]

On Friday July 11, willy@debian.org wrote:
> 
> # define do_div(n,base) ({                              \
>         uint32_t __base = (base);                       \
>         uint32_t __rem;                                 \
>         if (likely(((n) >> 32) == 0)) {                 \
> 
> so if we call do_div() on a u32, the compiler emits nasal daemons.
> and we do this -- in the antcipatory scheduler:
> 
>                 if (aic->seek_samples) {
>                         aic->seek_mean = aic->seek_total + 128;
>                         do_div(aic->seek_mean, aic->seek_samples);
>                 }
> 
> seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit platform.
> so we can't avoid calling do_div().
> 
> This almost works (the warning is harmless since gcc optimises away the call)
> 
> # define do_div(n,base) ({                                              \
>         uint32_t __base = (base);                                       \
>         uint32_t __rem;                                                 \
>         if ((sizeof(n) < 8) || (likely(((n) >> 32) == 0))) {            \
>                 __rem = (uint32_t)(n) % __base;                         \
>                 (n) = (uint32_t)(n) / __base;                           \
>         } else                                                          \
>                 __rem = __div64_32(&(n), __base);                       \
>         __rem;                                                          \
>  })
> 
> Better ideas?

sector_div, defined in blkdev.h, is probably what you want.

NeilBrown

Re: do_div vs sector_t (patch)

[Nick Piggin]

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



Neil Brown wrote:

>On Friday July 11, willy@debian.org wrote:
>
>># define do_div(n,base) ({                              \
>>        uint32_t __base = (base);                       \
>>        uint32_t __rem;                                 \
>>        if (likely(((n) >> 32) == 0)) {                 \
>>
>>so if we call do_div() on a u32, the compiler emits nasal daemons.
>>and we do this -- in the antcipatory scheduler:
>>
>>                if (aic->seek_samples) {
>>                        aic->seek_mean = aic->seek_total + 128;
>>                        do_div(aic->seek_mean, aic->seek_samples);
>>                }
>>
>>seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit platform.
>>so we can't avoid calling do_div().
>>
>>This almost works (the warning is harmless since gcc optimises away the call)
>>
>># define do_div(n,base) ({                                              \
>>        uint32_t __base = (base);                                       \
>>        uint32_t __rem;                                                 \
>>        if ((sizeof(n) < 8) || (likely(((n) >> 32) == 0))) {            \
>>                __rem = (uint32_t)(n) % __base;                         \
>>                (n) = (uint32_t)(n) / __base;                           \
>>        } else                                                          \
>>                __rem = __div64_32(&(n), __base);                       \
>>        __rem;                                                          \
>> })
>>
>>Better ideas?
>>
>
>sector_div, defined in blkdev.h, is probably what you want.
>

Looks right. Following patch alright?


[-- Attachment #2: as-do-div --]
[-- Type: text/plain, Size: 510 bytes --]

--- linux-2.5/drivers/block/as-iosched.c.orig	2003-07-12 10:12:20.000000000 +1000
+++ linux-2.5/drivers/block/as-iosched.c	2003-07-12 10:12:28.000000000 +1000
@@ -837,7 +837,7 @@ static void as_update_iohist(struct as_i
 		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);

Re: do_div vs sector_t

[Christoph Hellwig]

On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
>                         aic->seek_mean = aic->seek_total + 128;
>                         do_div(aic->seek_mean, aic->seek_samples);
>                 }
> 
> seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit platform.
> so we can't avoid calling do_div().

That's why we have sector_div, never use do_div on a sector_t.

Re: do_div vs sector_t

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
> >                         aic->seek_mean = aic->seek_total + 128;
> >                         do_div(aic->seek_mean, aic->seek_samples);
> >                 }
> > 
> > seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit platform.
> > so we can't avoid calling do_div().
> 
> That's why we have sector_div, never use do_div on a sector_t.

Thing is, the arith in there can overflow with 32-bit sector_t anyway, so
we need 64-bit quantities regardless of the CONFIG_LBD setting.

Re: do_div vs sector_t

[Richard Henderson]

On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
> Better ideas?

          if (likely(((n) >> 31 >> 1) == 0)) {


r~

Re: do_div vs sector_t

[Russell King]

On Sun, Jul 13, 2003 at 10:26:23AM -0700, Richard Henderson wrote:
> On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
> > Better ideas?
> 
>           if (likely(((n) >> 31 >> 1) == 0)) {
> 

Beware - luckily I don't have to worry about that on ARM (we do our own
thing.)  However, with this code:

int foo(unsigned long long n)
{
        if (((n) >> 31 >> 1) == 0) {
                return 1;
        } else {
                return 0;
        }
}

gcc 3.2.2 on ARM (32-bit) produces some not-very-nice code, consisting of
6 shifts and including placing one register on the stack and completely
ignoring a register which it could freely use instead.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: do_div vs sector_t

[Bernardo Innocenti]

On Sunday 13 July 2003 19:26, Richard Henderson wrote:
> On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
> > Better ideas?
>
>           if (likely(((n) >> 31 >> 1) == 0)) {

Do we still need to fix this? I've already posted a patch to disallow
calling do_div() with any divisor that doesn't look like an unsigned
64bit interger.

-- 
  // 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

Re: do_div vs sector_t

[Matthew Wilcox]

On Sun, Jul 13, 2003 at 09:14:35PM +0200, Bernardo Innocenti wrote:
> On Sunday 13 July 2003 19:26, Richard Henderson wrote:
> > On Fri, Jul 11, 2003 at 11:33:59PM +0100, Matthew Wilcox wrote:
> > > Better ideas?
> >
> >           if (likely(((n) >> 31 >> 1) == 0)) {
> 
> Do we still need to fix this? I've already posted a patch to disallow
> calling do_div() with any divisor that doesn't look like an unsigned
> 64bit interger.

No, I think the combination of sector_div() and your patch makes everything
happy-happy.  Thanks!

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

do_div vs sector_t

[Peter Chubb]

>>>>> "Matthew" == Matthew Wilcox <willy@debian.org> writes:


Matthew>                         do_div(aic->seek_mean, aic->seek_samples); }

Matthew> seek_mean is a sector_t so sometimes it's 64-bit on a 32-bit
Matthew> platform.  so we can't avoid calling do_div().

Use sector_div() instead! -- it uses a 63/32 bit divide/remainder if
sector_t is 64-bit, and 32/32 if it's 32-bit.

--
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.

Re: do_div vs sector_t

[linux]

>        if (likely(((n) >> 32) == 0)) {                 \

if (likely((n) <= 0xffffffff)) {

will work with any unsigned type for n.


GCC knows to only look at the high word to make this test.