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 <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
