do_div considered harmful

do_div considered harmful

[Andries.Brouwer]

Writing this ide capacity patch an hour ago or so
I split off a helper sectors_to_MB() since Erik's recent
patch uses this also.
Now that I compare, he wrote
	nativeMb = do_div(nativeMb, 1000000);
to divide nativeMb by 1000000.
Similarly, I find in fs/cifs/inode.c
	inode->i_blocks = do_div(findData.NumOfBytes, inode->i_blksize);

So, it seems natural to expect that do_div() gives the quotient.
But it gives the remainder.
(Strange, Erik showed correct output.)

Since the semantics of this object are very unlike that of a C function,
I wonder whether we should write DO_DIV instead, or DO_DIV_AND_REM
to show that a remainder is returned.

Andries

Re: do_div considered harmful

[Andrew Morton]

Andries.Brouwer@cwi.nl wrote:
>
> Writing this ide capacity patch an hour ago or so
> I split off a helper sectors_to_MB() since Erik's recent
> patch uses this also.
> Now that I compare, he wrote
> 	nativeMb = do_div(nativeMb, 1000000);
> to divide nativeMb by 1000000.
> Similarly, I find in fs/cifs/inode.c
> 	inode->i_blocks = do_div(findData.NumOfBytes, inode->i_blksize);

This should be

	int blocksize = 1 << inode->i_blkbits;

	inode->i_blocks = (findData.NumOfBytes + blocksize - 1)
				>> inode->i_blkbits;
				

and inode.i_blksize should probably be removed from the kernel.

> So, it seems natural to expect that do_div() gives the quotient.
> But it gives the remainder.
> (Strange, Erik showed correct output.)
> 
> Since the semantics of this object are very unlike that of a C function,
> I wonder whether we should write DO_DIV instead, or DO_DIV_AND_REM
> to show that a remainder is returned.

Sometimes the slash-star operator comes in handy.


--- 25/include/asm-i386/div64.h~do_div-comment	2003-08-03 19:20:58.000000000 -0700
+++ 25-akpm/include/asm-i386/div64.h	2003-08-03 19:21:11.000000000 -0700
@@ -1,6 +1,16 @@
 #ifndef __I386_DIV64
 #define __I386_DIV64
 
+/*
+ * The semantics of do_div() are:
+ *
+ * uint32_t do_div(uint64_t *n, uint32_t base)
+ * {
+ * 	uint32_t remainder = *n % base;
+ * 	*n = *n / base;
+ * 	return remainder;
+ * }
+ */
 #define do_div(n,base) ({ \
 	unsigned long __upper, __low, __high, __mod; \
 	asm("":"=a" (__low), "=d" (__high):"A" (n)); \

_

Re: do_div considered harmful

[Linus Torvalds]

On Mon, 4 Aug 2003 Andries.Brouwer@cwi.nl wrote:
>
> Now that I compare, he wrote
> 	nativeMb = do_div(nativeMb, 1000000);
> to divide nativeMb by 1000000.
> 
> So, it seems natural to expect that do_div() gives the quotient.
> But it gives the remainder.
> (Strange, Erik showed correct output.)

Actually, the above is "undefined behaviour", since it has _two_ 
assignments in the same thing. Exactly because "do_div()" modifies both 
the first argument _and_ returns a value. So depending on the 
implementation of do_div() (whether there are any sequence points etc) and 
on random compiler behaviour (if there are no sequence points in do_div() 
internally), in the example above "nativeMb" migth be totally undefined 
after the above.

And yes, as a special case, it might be the divisor.

I agree that "do_div()" has strange semantics and is very likely misnamed,
but they are kind of forced upon us by the fact that C functions cannot
return two values. Renaming do_div() at this point is just going to make
it harder to write kernel- portable source, so I suspect we're better off
just commenting it, and making people aware of how do_div() works.

Not very many people should use "do_div()" directly (and a quick grep
shows that not very many people do). It's generally a mistake to do so, I
suspect. The thing was originally written explicitly for "printk()" and
nothing else.

		Linus

Re: i_blksize

[Andries Brouwer]

On Sun, Aug 03, 2003 at 07:29:19PM -0700, Andrew Morton wrote:

> and inode.i_blksize should probably be removed from the kernel.

I see that Linus has added a comment and does not want to rename,
so that settles that half of that letter.

You propose to remove i_blksize.
Yes, a good plan, half an hour's work.
It is used in stat only, so we have to produce some value for stat.
Do you want to replace
	inode->i_blksize
by
	inode->i_sb->s_optimal_io_size
?

Andries

Re: i_blksize

[Andrew Morton]

Andries Brouwer <aebr@win.tue.nl> wrote:
>
> You propose to remove i_blksize.
>  Yes, a good plan, half an hour's work.
>  It is used in stat only, so we have to produce some value for stat.
>  Do you want to replace
>  	inode->i_blksize
>  by
>  	inode->i_sb->s_optimal_io_size
>  ?

No, that's different.  You are referring to stat.st_blksize.  That is a
different animal, and we can leave that alone.

inode->i_blksize is equal to (1 << inode->i_blkbits) always, all the time. 
It is just duplicated nonsense and usually leads to poorer code -
multiplications instead of shifts.

It should be a pretty easy incremental set of patches to ease i_blksize out
of the kernel.

Re: do_div considered harmful

[Andries.Brouwer]

> Sometimes the slash-star operator comes in handy.

Certainly an improvement. Maybe it will help a little.

But really, do_div is not a C function, it has very
nonintuitive behaviour.
The two bugs are: (i) the name is wrong, it returns a
remainder but the name only talks about dividing, and
(ii) worst of all, it changes its first argument.

> + * uint32_t do_div(uint64_t *n, uint32_t base)

And the comment that you added, copied from elsewhere,
is confusing too. The first parameter is not uint64_t *.
This thing cannot be described in C.

I would still like to replace do_div by DO_DIV_AND_REM
as a big fat warning - this is a macro, read the definition.
And the common case, where no remainder is used, by DO_DIV
#define DO_DIV(a,b)	(void) DO_DIV_AND_REM(a,b)

Andries

[Perhaps DO_DIV64, to also indicate a type.]

[The same problem happens with sector_div. I recall having
to fix sector_div calls in scsi code. These functions are
bugs waiting to happen. Nobody expects that after
	res = func(a,b);
the value of a has changed.]

Re: do_div considered harmful

[Steve French]

I have made a change to fix this in fs/cifs/inode.c and a similar 
problem in fs/cifs/file.c
(and a change to fs/cifs/cifsfs.c to better match the blocksize bits and 
blocksize default).
It is (version 0.8.7 of the cifs vfs) in 
bk://cifs.bkbits.net/linux-2.5cifs and I am testing it now.

 >> Similarly, I find in fs/cifs/inode.c>
 >>          inode->i_blocks = do_div(findData.NumOfBytes, 
inode->i_blksize);
 >
 >This should be
 >
 >        int blocksize = 1 << inode->i_blkbits;
 >
 >         inode->i_blocks = (findData.NumOfBytes + blocksize - 1)
 >                                    >> inode->i_blkbits;