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
[email protected] 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)); \
_
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;
On Mon, 4 Aug 2003 [email protected] 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
> 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.]
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
Andries Brouwer <[email protected]> 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.