2003-07-11 22:19:23

by Matthew Wilcox

[permalink] [raw]
Subject: do_div vs sector_t


# 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


2003-07-11 22:27:44

by Andrew Morton

[permalink] [raw]
Subject: Re: do_div vs sector_t

Matthew Wilcox <[email protected]> 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; \
})


_



2003-07-11 22:28:51

by NeilBrown

[permalink] [raw]
Subject: Re: do_div vs sector_t

On Friday July 11, [email protected] 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

2003-07-12 00:00:38

by Nick Piggin

[permalink] [raw]
Subject: Re: do_div vs sector_t (patch)

--- 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);


Attachments:
as-do-div (510.00 B)

2003-07-12 06:37:33

by Christoph Hellwig

[permalink] [raw]
Subject: Re: do_div vs sector_t

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.

2003-07-12 06:43:46

by Andrew Morton

[permalink] [raw]
Subject: Re: do_div vs sector_t

Christoph Hellwig <[email protected]> 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.

2003-07-13 19:49:27

by Matthew Wilcox

[permalink] [raw]
Subject: Re: do_div vs sector_t

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

2003-07-13 18:59:55

by Bernardo Innocenti

[permalink] [raw]
Subject: Re: do_div vs sector_t

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


2003-07-13 18:25:20

by George Spelvin

[permalink] [raw]
Subject: Re: do_div vs sector_t

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

2003-07-13 17:11:48

by Richard Henderson

[permalink] [raw]
Subject: Re: do_div vs sector_t

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

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


r~

2003-07-13 17:24:58

by Russell King

[permalink] [raw]
Subject: Re: do_div vs sector_t

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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2003-07-14 03:53:43

by Peter Chubb

[permalink] [raw]
Subject: do_div vs sector_t

>>>>> "Matthew" == Matthew Wilcox <[email protected]> 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.