Re: amd64 bitops fix for -Os

[Randy Dunlap]

Alexandre Oliva wrote:
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=171672
> 
> This patches fixes a bug that comes up when compiling the kernel for
> x86_64 optimizing for size.  It affects 2.6.14 for sure, but I'm
> pretty sure many earlier kernels are affected as well.
> 
> The symptom is that, as soon as some change is made to the root
> filesystem (e.g. dmesg > /var/log/dmesg), the kernel mostly hangs.  It
> was not the first time I'd run into this symptom, but this time I
> could track the problem down to enabling size optimizations in the
> kernel build.  It took some time to narrow down the culprit source
> with a binary search, compiling part of the kernel sources with -Os
> and part with -O2, but eventually it was clear that bitops itself was
> to blame, which should have been clear from the soft lockup oops I
> got.
> 
> The problem is that find_first_zero_bit() fails when called with an
> underflown size, because its inline asm assumes at least one iteration
> of scasq will run.  When this does not hold, the conditional branch
> that follows it uses flags from instructions prior to the asm
> statement.


[snip]


> Anyhow, with this patch I could run 2.6.14, as in the Fedora
> development tree, except for the change to optimize for size.

Yes, works for me too.

> 	Signed-off-by: Alexandre Oliva <oliva@xxxxxxxxxxxxxxxxx>

Possibly Andrew or Andi have already merged this into their trees.
However, I have a few comments on the patch re Linux style.
They are meant to help inform you and others -- that's all.

> --- arch/x86_64/lib/bitops.c~	2005-10-27 22:02:08.000000000 -0200
> +++ arch/x86_64/lib/bitops.c	2005-10-29 18:24:27.000000000 -0200

Diffs should start with a top-level names (even if it's entirely
phony), so that they can be applied with many scripts that are around
and expect that.

> @@ -1,5 +1,11 @@
>  #include <linux/bitops.h>
>  
> +#define BITOPS_CHECK_UNDERFLOW_RANGE 0
> +
> +#if BITOPS_CHECK_UNDERFLOW_RANGE
> +# include <linux/kernel.h>
> +#endif

We don't usually indent inside if/endif.

> @@ -13,11 +19,21 @@
>   * Returns the bit-number of the first zero bit, not the number of the byte
>   * containing a bit.
>   */
> -inline long find_first_zero_bit(const unsigned long * addr, unsigned long size)
> +static inline long
> +__find_first_zero_bit(const unsigned long * addr, unsigned long size)

The only good reason for splitting a function header is if it would
otherwise be > 80 columns, not just to put the function name at the
beginning of the line.

>  {
>  	long d0, d1, d2;
>  	long res;
>  
> +	/* We must test the size in words, not in bits, because
> +	   otherwise incoming sizes in the range -63..-1 will not run
> +	   any scasq instructions, and then the flags used by the je
> +	   instruction will have whatever random value was in place
> +	   before.  Nobody should call us like that, but
> +	   find_next_zero_bit() does when offset and size are at the
> +	   same word and it fails to find a zero itself.  */

Linux long-comment style is:
	/*
	 * line1 words ....
	 * line2
	 * line3
	 */

> @@ -30,11 +46,22 @@
>  		"  shlq $3,%%rdi\n"
>  		"  addq %%rdi,%%rdx"
>  		:"=d" (res), "=&c" (d0), "=&D" (d1), "=&a" (d2)
> -		:"0" (0ULL), "1" ((size + 63) >> 6), "2" (addr), "3" (-1ULL),
> -		 [addr] "r" (addr) : "memory");
> +		:"0" (0ULL), "1" (size), "2" (addr), "3" (-1ULL),
> +		 /* Any register here would do, but GCC tends to
> +		    prefer rbx over rsi, even though rsi is readily
> +		    available and doesn't have to be saved.  */
> +		 [addr] "S" (addr) : "memory");

Comment in the middle of the difficult-to-read asm instruction in
undesirable (IMO).

>  	return res;
>  }
>  

> @@ -74,6 +101,17 @@
>  	long d0, d1;
>  	long res;
>  
> +	/* We must test the size in words, not in bits, because
> +	   otherwise incoming sizes in the range -63..-1 will not run
> +	   any scasq instructions, and then the flags used by the jz
> +	   instruction will have whatever random value was in place
> +	   before.  Nobody should call us like that, but
> +	   find_next_bit() does when offset and size are at the same
> +	   word and it fails to find a one itself.  */

Comment style again.

But I'm happy that my kernel now boots.  I didn't realize that it was
a -Os issue until your email.  Thanks.

-- 
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/