UP went into unexpected trashing
Quoted 8 lines · Rusty Trivial Russell
include/asm-i386/bitops.h 2002-11-08 18:47:20.000000000 +1100 renamed from include/asm-i386/bitops.h.orig 2002-11-08 18:47:20.000000000 +1100 +0
-2@@ -311,12 +311,13 @@ "repe; scasl\n\t" "jz 1f\n\t" "leal -4(%%edi),%%edi\n\t" - "bsfl (%%edi),%%eax\n" - "1:\tsubl %%ebx,%%edi\n\t"
include/asm-i386/bitops.h 2002-11-08 18:47:20.000000000 +1100 line 321
Quoted 10 lines
include/asm-i386/bitops.h 2002-11-08 18:47:20.000000000 +1100 renamed from include/asm-i386/bitops.h.orig 2002-11-08 18:47:20.000000000 +1100 +5
-2+ "bsfl (%%edi),%%edx\n" + "subl %%ebx,%%edi\n\t" "shll $3,%%edi\n\t" - "addl %%edi,%%eax" + "addl %%edi,%%edx\n\t" + "1:\tmovl %%edx,%%eax\n\t" :"=a" (res), "=&c" (d0), "=&D" (d1) - :"1" ((size + 31) >> 5), "2" (addr), "b" (addr)); + :"1" ((size + 31) >> 5), "2" (addr), "b" (addr), "d" (size)); return res;
EDX is modified, should not you list "=d" as output, with new variable (d2)?
Or better, drop last line of assembly code, and say that (res) is in "d", and list "a" as clobbered or dummy output register.
And BTW, if you'll do
unsigned long b = 0x8000;find_first_bit(&b, 1);
return value will be 15 even with patched function. So either more fixing is needed, or code which calls find_first_bit() with value which is not multiple of 32 should take special care that last dword does not contain set bits after last interesting bit.
So maybe callers should just treat any return value >= size as "not found", leaving older smaller code in place.
Best regards,
Petr Vandrovec [email protected]
Quoted 4 lines · Petr Vandrovec
On 8 Nov 02 at 19:33, Rusty Trivial Russell wrote:
So maybe callers should just treat any return value >= size as "not found", leaving older smaller code in place.
Or add a check in there. I can't figure out a way to avoid the extra conditional branch anyway :)
Thanks
Dipankar
include/asm-i386/bitops.h +1 -1
@@ -317,7 +317,7 @@ "addl %%edi,%%eax" :"=a" (res), "=&c" (d0), "=&D" (d1) :"1" ((size + 31) >> 5), "2" (addr), "b" (addr)); -return res; +return (size > res) ? res : size; }/**
Quoted 3 lines · Dipankar Sarma
Or add a check in there. I can't figure out a way to avoid the extra conditional branch anyway :)
I'd actually rather change the calling convention, and say that the only valid test is for testing the return value "being in range".
And it's not strictly even a calling convention _change_ - it's always been that way. See the original user, ie the minix filesystem:
if ((j = minix_find_first_zero_bit(bh->b_data, 8192)) < 8192) {
...and it's really a documentation fix/update to just tell people about this.
Linus
In message <[email protected]> you write:
Quoted 8 lines · Dipankar Sarma
Quoted 3 lines
Or add a check in there. I can't figure out a way to avoid the extra conditional branch anyway :)
I'd actually rather change the calling convention, and say that the only valid test is for testing the return value "being in range".
At least gcc 3.0 doesn't do the optimization for you, unfortunately 8(
So I agree, although I'd prefer the stricter definition.
Rusty.
PS. Previous documentation was written by a bad monkey, so I've fixed
it to actually say something useful, too.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
include/asm-i386/bitops.h +18 -10
@@ -263,10 +263,10 @@ static __inline__ int variable_test_bit(/** * find_first_zero_bit - find the first zero bit in a memory region * @addr: The address to start the search at -* @size: The maximum size to search +* @size: The maximum size to search (may be rounded up by BITS_PER_LONG) * -* Returns the bit-number of the first zero bit, not the number of the byte -* containing a bit. +* Returns the bit-number of the first zero bit (not the number of the byte +* containing the bit) or a value >= size if none found. */ static __inline__int find_first_zero_bit (unsigned long * addr,unsigned size) {
@@ -295,10 +295,10 @@ static __inline__ int find_first_zero_bi/** * find_first_bit - find the first set bit in a memory region * @addr: The address to start the search at -* @size: The maximum size to search +* @size: The maximum size to search (may be rounded up by BITS_PER_LONG) * -* Returns the bit-number of the first set bit, not the number of the byte -* containing a bit. +* Returns the bit-number of the first set bit (not the number of the byte +* containing the bit) or a value >= size if none found. */ static __inline__int find_first_bit (unsigned long * addr,unsigned size) {
@@ -321,10 +321,14 @@ static __inline__ int find_first_bit(uns }/** -* find_next_zero_bit - find the first zero bit in a memory region +* find_next_zero_bit - find the next zero bit in a memory region * @addr: The address to base the search on * @offset: The bitnumber to start searching at -* @size: The maximum size to search +* @size: The maximum size to search (may be rounded up by BITS_PER_LONG) +* +* Returns the bit-number of the first zero bit >= offset (not the +* number of the byte containing the bit), or a value >= size if none +* found. */ static __inline__int find_next_zero_bit (unsigned long * addr,int size,int offset) {
@@ -354,10 +358,14 @@ static __inline__ int find_next_zero_bit }/** -* find_next_bit - find the first set bit in a memory region +* find_next_bit - find the next set bit in a memory region * @addr: The address to base the search on * @offset: The bitnumber to start searching at -* @size: The maximum size to search +* @size: The maximum size to search (may be rounded up by BITS_PER_LONG) +* +* Returns the bit-number of the first bit set >= offset (not the +* number of the byte containing the bit) or a value >= size if none +* found. */ static __inline__int find_next_bit (unsigned long *addr,int size,int offset) {
Quoted 4 lines · Rusty Russell
- * Returns the bit-number of the first zero bit, not the number of the byte
- * containing a bit.
+ * Returns the bit-number of the first zero bit (not the number of the byte
+ * containing the bit) or a value >= size if none found.
Ok.
However, what was the original codepath that doesn't follow this and was the cause of the headache (ie the "unexpected trashing"?) Let's fix that user of the functions too, not just the documentation..
Linus
In message <[email protected]> you wri te:
Quoted 6 lines · Rusty Russell
Quoted 1 line
+ * containing the bit) or a value >= size if none found.
However, what was the original codepath that doesn't follow this and was the cause of the headache (ie the "unexpected trashing"?)
It was the "cpu masks become an unsigned long array" patch, which was in Andrew Morton's mm tree, but you never took.
Quoted 1 line
Let's fix that user of the functions too, not just the documentation..
You're not adopting the Viro doctine are you? "On each new issue, begin by assuming your peers are idiots". 8)
It's not becoming,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.