netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
@ 2008-07-13  0:28 Krzysztof Halasa
  2008-07-13  8:31 ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Halasa @ 2008-07-13  0:28 UTC (permalink / raw)
  To: linux-kernel, netdev

Hi,

Bisected the panic. It seems it occurs on my ARM IXP4xx big-endian
system with USB ADSL PPP over ATM connection (Thomson/Alcatel
Speedtouch). It doesn't seem to occur on the same IXP4xx with Ethernet
or V.35 WAN, and it doesn't occur on i386 + the same Speedtouch ADSL.
The kernel is basically unpatched, the only extra patch applied is the
platform support (nothing magic).

Generally to trigger the panic one has to request a TCP data stream
over that PPPoATM connection.

Reverting the commit on top of 2.6.25.10 fixes the problem.

Now what's wrong with that?

7e58886b45bc4a309aeaa8178ef89ff767daaf7f:
author Stephen Hemminger <shemminger@linux-foundation.org>
[TCP]: cubic optimization

Use willy's work in optimizing cube root by having table for small values.

--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -91,23 +91,51 @@ static void bictcp_init(struct sock *sk)
 		tcp_sk(sk)->snd_ssthresh = initial_ssthresh;
 }
 
-/*
- * calculate the cubic root of x using Newton-Raphson
+/* calculate the cubic root of x using a table lookup followed by one
+ * Newton-Raphson iteration.
+ * Avg err ~= 0.195%
  */
 static u32 cubic_root(u64 a)
 {
-	u32 x;
-
-	/* Initial estimate is based on:
-	 * cbrt(x) = exp(log(x) / 3)
+	u32 x, b, shift;
+	/*
+	 * cbrt(x) MSB values for x MSB values in [0..63].
+	 * Precomputed then refined by hand - Willy Tarreau
+	 *
+	 * For x in [0..63],
+	 *   v = cbrt(x << 18) - 1
+	 *   cbrt(x) = (v[x] + 10) >> 6
 	 */
-	x = 1u << (fls64(a)/3);
+	static const u8 v[] = {
+		/* 0x00 */    0,   54,   54,   54,  118,  118,  118,  118,
+		/* 0x08 */  123,  129,  134,  138,  143,  147,  151,  156,
+		/* 0x10 */  157,  161,  164,  168,  170,  173,  176,  179,
+		/* 0x18 */  181,  185,  187,  190,  192,  194,  197,  199,
+		/* 0x20 */  200,  202,  204,  206,  209,  211,  213,  215,
+		/* 0x28 */  217,  219,  221,  222,  224,  225,  227,  229,
+		/* 0x30 */  231,  232,  234,  236,  237,  239,  240,  242,
+		/* 0x38 */  244,  245,  246,  248,  250,  251,  252,  254,
+	};
+
+	b = fls64(a);
+	if (b < 7) {
+		/* a in [0..63] */
+		return ((u32)v[(u32)a] + 35) >> 6;
+	}
+
+	b = ((b * 84) >> 8) - 1;
+	shift = (a >> (b * 3));
 
-	/* converges to 32 bits in 3 iterations */
-	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
-	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
-	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
+	x = ((u32)(((u32)v[shift] + 10) << b)) >> 6;
 
+	/*
+	 * Newton-Raphson iteration
+	 *                         2
+	 * x    = ( 2 * x  +  a / x  ) / 3
+	 *  k+1          k         k
+	 */
+	x = (2 * x + (u32)div64_64(a, (u64)x * (u64)(x - 1)));
+	x = ((x * 341) >> 10);
 	return x;
 }
 
PC is at bictcp_cong_avoid+0x33c/0x454

Backtrace:

bictcp_cong_avoid+0x0/0x454		from tcp_cong_avoid+0x1c/0x30
tcp_cong_avoid+0x0/0x30			from tcp_ack+0x156c/0x1c6c
	r4:00000000
tcp_ack+0x0/0x1c6c			from tcp_rcv_established+0x4a8/0x76c
tcp_rcv_established+0x0/0x76c		from tcp_v4_do_rcv+0x25c/0x458
tcp_v4_do_rcv+0x0/0x458			from tcp_v4_rcv+0x6c4/0x7ec
tcp_v4_rcv+0x0/0x7ec			from ip_local_deliver_finish+0xe4/0x24c
ip_local_deliver_finish+0x0/0x24c	from ip_local_deliver+0x4c/0xac
	r7:c043817c r6:c6818612 r5:c6a6a3c0 r4:c6a6a3c0
ip_local_deliver+0x0/0xac		from ip_rcv_finish+0x114/0x378
	r4:80000000
ip_rcv_finish+0x0/0x378			from ip_rcv+0x1f8/0x2a8
ip_rcv+0x0/0x2a8			from netif_receive_skb+0x36c/0x4a4
	r7:00000800 r6:c0405e80 r5:c6a6a3c0 r4:c0436d54
netif_receive_skb+0x0/0x4a4		from process_backlog+0x88/0x14c
	r8:00000040 r7:c0436d24 r6:00000000 r5:c0436d0c r4:c6912c00
process_backlog+0x0/0x14c		from net_rx_action+0xb4/0x1c4
net_rx_action+0x0/0x1c4			from __do_softirq+0x68/0xe0
_do_softirq+0x0/0xe0			from irq_exit+0x48/0x50
	r7:00000000 r6:c04344d0 r5:c040ae24 r4:00000014
irq_exit+0x0/0x50			from asm_do_IRQ+0x48/0x5c
asm_do_IRQ+0x0/0x5c			from __irq_svc+0x24/0x60
...
-- 
Krzysztof Halasa

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13  0:28 [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+ Krzysztof Halasa
@ 2008-07-13  8:31 ` Andrew Morton
  2008-07-13 10:48   ` Krzysztof Halasa
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2008-07-13  8:31 UTC (permalink / raw)
  To: Krzysztof Halasa; +Cc: linux-kernel, netdev, Stephen Hemminger, David S. Miller


(cc's added)

On Sun, 13 Jul 2008 02:28:13 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Hi,
> 
> Bisected the panic. It seems it occurs on my ARM IXP4xx big-endian
> system with USB ADS

I guess you're referring to this:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-07/msg04754.html

> PPP over ATM connection (Thomson/Alcatel
> Speedtouch). It doesn't seem to occur on the same IXP4xx with Ethernet
> or V.35 WAN, and it doesn't occur on i386 + the same Speedtouch ADSL.
> The kernel is basically unpatched, the only extra patch applied is the
> platform support (nothing magic).
> 
> Generally to trigger the panic one has to request a TCP data stream
> over that PPPoATM connection.
> 
> Reverting the commit on top of 2.6.25.10 fixes the problem.
> 
> Now what's wrong with that?
> 
> 7e58886b45bc4a309aeaa8178ef89ff767daaf7f:
> author Stephen Hemminger <shemminger@linux-foundation.org>
> [TCP]: cubic optimization
> 
> Use willy's work in optimizing cube root by having table for small values.
> 
> --- a/net/ipv4/tcp_cubic.c
> +++ b/net/ipv4/tcp_cubic.c
> @@ -91,23 +91,51 @@ static void bictcp_init(struct sock *sk)
>  		tcp_sk(sk)->snd_ssthresh = initial_ssthresh;
>  }
>  
> -/*
> - * calculate the cubic root of x using Newton-Raphson
> +/* calculate the cubic root of x using a table lookup followed by one
> + * Newton-Raphson iteration.
> + * Avg err ~= 0.195%
>   */
>  static u32 cubic_root(u64 a)
>  {
> -	u32 x;
> -
> -	/* Initial estimate is based on:
> -	 * cbrt(x) = exp(log(x) / 3)
> +	u32 x, b, shift;
> +	/*
> +	 * cbrt(x) MSB values for x MSB values in [0..63].
> +	 * Precomputed then refined by hand - Willy Tarreau
> +	 *
> +	 * For x in [0..63],
> +	 *   v = cbrt(x << 18) - 1
> +	 *   cbrt(x) = (v[x] + 10) >> 6
>  	 */
> -	x = 1u << (fls64(a)/3);
> +	static const u8 v[] = {
> +		/* 0x00 */    0,   54,   54,   54,  118,  118,  118,  118,
> +		/* 0x08 */  123,  129,  134,  138,  143,  147,  151,  156,
> +		/* 0x10 */  157,  161,  164,  168,  170,  173,  176,  179,
> +		/* 0x18 */  181,  185,  187,  190,  192,  194,  197,  199,
> +		/* 0x20 */  200,  202,  204,  206,  209,  211,  213,  215,
> +		/* 0x28 */  217,  219,  221,  222,  224,  225,  227,  229,
> +		/* 0x30 */  231,  232,  234,  236,  237,  239,  240,  242,
> +		/* 0x38 */  244,  245,  246,  248,  250,  251,  252,  254,
> +	};
> +
> +	b = fls64(a);
> +	if (b < 7) {
> +		/* a in [0..63] */
> +		return ((u32)v[(u32)a] + 35) >> 6;
> +	}
> +
> +	b = ((b * 84) >> 8) - 1;
> +	shift = (a >> (b * 3));
>  
> -	/* converges to 32 bits in 3 iterations */
> -	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
> -	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
> -	x = (2 * x + (u32)div64_64(a, (u64)x*(u64)x)) / 3;
> +	x = ((u32)(((u32)v[shift] + 10) << b)) >> 6;
>  
> +	/*
> +	 * Newton-Raphson iteration
> +	 *                         2
> +	 * x    = ( 2 * x  +  a / x  ) / 3
> +	 *  k+1          k         k
> +	 */
> +	x = (2 * x + (u32)div64_64(a, (u64)x * (u64)(x - 1)));
> +	x = ((x * 341) >> 10);
>  	return x;
>  }
>  
> PC is at bictcp_cong_avoid+0x33c/0x454
> 
> Backtrace:
> 
> bictcp_cong_avoid+0x0/0x454		from tcp_cong_avoid+0x1c/0x30
> tcp_cong_avoid+0x0/0x30			from tcp_ack+0x156c/0x1c6c
> 	r4:00000000
> tcp_ack+0x0/0x1c6c			from tcp_rcv_established+0x4a8/0x76c
> tcp_rcv_established+0x0/0x76c		from tcp_v4_do_rcv+0x25c/0x458
> tcp_v4_do_rcv+0x0/0x458			from tcp_v4_rcv+0x6c4/0x7ec
> tcp_v4_rcv+0x0/0x7ec			from ip_local_deliver_finish+0xe4/0x24c
> ip_local_deliver_finish+0x0/0x24c	from ip_local_deliver+0x4c/0xac
> 	r7:c043817c r6:c6818612 r5:c6a6a3c0 r4:c6a6a3c0
> ip_local_deliver+0x0/0xac		from ip_rcv_finish+0x114/0x378
> 	r4:80000000
> ip_rcv_finish+0x0/0x378			from ip_rcv+0x1f8/0x2a8
> ip_rcv+0x0/0x2a8			from netif_receive_skb+0x36c/0x4a4
> 	r7:00000800 r6:c0405e80 r5:c6a6a3c0 r4:c0436d54
> netif_receive_skb+0x0/0x4a4		from process_backlog+0x88/0x14c
> 	r8:00000040 r7:c0436d24 r6:00000000 r5:c0436d0c r4:c6912c00
> process_backlog+0x0/0x14c		from net_rx_action+0xb4/0x1c4
> net_rx_action+0x0/0x1c4			from __do_softirq+0x68/0xe0
> _do_softirq+0x0/0xe0			from irq_exit+0x48/0x50
> 	r7:00000000 r6:c04344d0 r5:c040ae24 r4:00000014
> irq_exit+0x0/0x50			from asm_do_IRQ+0x48/0x5c
> asm_do_IRQ+0x0/0x5c			from __irq_svc+0x24/0x60
> ...
> -- 
> Krzysztof Halasa
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13  8:31 ` Andrew Morton
@ 2008-07-13 10:48   ` Krzysztof Halasa
  2008-07-13 18:22     ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Halasa @ 2008-07-13 10:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, netdev, Stephen Hemminger, David S. Miller,
	Russell King

Andrew Morton <akpm@linux-foundation.org> writes:

> (cc's added)

(cc added) :-)

> I guess you're referring to this:
> http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-07/msg04754.html

Right.

>> PPP over ATM connection (Thomson/Alcatel
>> Speedtouch). It doesn't seem to occur on the same IXP4xx with Ethernet
>> or V.35 WAN, and it doesn't occur on i386 + the same Speedtouch ADSL.
>> The kernel is basically unpatched, the only extra patch applied is the
>> platform support (nothing magic).
>> 
>> Generally to trigger the panic one has to request a TCP data stream
>> over that PPPoATM connection.

I see what's wrong now: that's the ARM fls() problem, the call in
fls64() to be precise. It seems it was already discussed: the patch in
http://lkml.org/lkml/2008/5/4/233 makes the problem disappear.

Perhaps it's time to fix it definitely?
-- 
Krzysztof Halasa

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13 10:48   ` Krzysztof Halasa
@ 2008-07-13 18:22     ` Andrew Morton
  2008-07-13 21:51       ` Krzysztof Halasa
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2008-07-13 18:22 UTC (permalink / raw)
  To: Krzysztof Halasa
  Cc: linux-kernel, netdev, Stephen Hemminger, David S. Miller,
	Russell King

On Sun, 13 Jul 2008 12:48:15 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > (cc's added)
> 
> (cc added) :-)
> 
> > I guess you're referring to this:
> > http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-07/msg04754.html
> 
> Right.
> 
> >> PPP over ATM connection (Thomson/Alcatel
> >> Speedtouch). It doesn't seem to occur on the same IXP4xx with Ethernet
> >> or V.35 WAN, and it doesn't occur on i386 + the same Speedtouch ADSL.
> >> The kernel is basically unpatched, the only extra patch applied is the
> >> platform support (nothing magic).
> >> 
> >> Generally to trigger the panic one has to request a TCP data stream
> >> over that PPPoATM connection.
> 
> I see what's wrong now: that's the ARM fls() problem, the call in
> fls64() to be precise. It seems it was already discussed: the patch in
> http://lkml.org/lkml/2008/5/4/233 makes the problem disappear.

Ah.

> Perhaps it's time to fix it definitely?

I'd sugget something like this.  Can you test, please?

--- a/include/asm-arm/bitops.h~a
+++ a/include/asm-arm/bitops.h
@@ -277,9 +277,16 @@ static inline int constant_fls(int x)
  * the clz instruction for much better code efficiency.
  */
 
-#define fls(x) \
+#define __fls(x) \
 	( __builtin_constant_p(x) ? constant_fls(x) : \
 	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
+
+/* Implement fls() in C so that 64-bit args are suitably truncated */
+static inline int fls(int x)
+{
+	return __fls(x);
+}
+
 #define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
 #define __ffs(x) (ffs(x) - 1)
 #define ffz(x) __ffs( ~(x) )
_


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13 18:22     ` Andrew Morton
@ 2008-07-13 21:51       ` Krzysztof Halasa
  2008-07-13 21:55         ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Halasa @ 2008-07-13 21:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, netdev, Stephen Hemminger, David S. Miller,
	Russell King

Andrew Morton <akpm@linux-foundation.org> writes:

> --- a/include/asm-arm/bitops.h~a
> +++ a/include/asm-arm/bitops.h
> @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
>   * the clz instruction for much better code efficiency.
>   */
>  
> -#define fls(x) \
> +#define __fls(x) \
>  	( __builtin_constant_p(x) ? constant_fls(x) : \
>  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> +
> +/* Implement fls() in C so that 64-bit args are suitably truncated */
> +static inline int fls(int x)
> +{
> +	return __fls(x);
> +}
> +

Well, I like it more as it fixes all possible places instead of only
fls64().

But... can't we just move the #define body into the inline fls(x)?
Will there be other users of __fls(x)? It seems the
__builtin_constant_p(x) works for inline functions.

The above patch fixes the kernel panic, too.
-- 
Krzysztof Halasa

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13 21:51       ` Krzysztof Halasa
@ 2008-07-13 21:55         ` Andrew Morton
  2008-07-23  4:52           ` Willy Tarreau
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2008-07-13 21:55 UTC (permalink / raw)
  To: Krzysztof Halasa
  Cc: linux-kernel, netdev, Stephen Hemminger, David S. Miller,
	Russell King

On Sun, 13 Jul 2008 23:51:06 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > --- a/include/asm-arm/bitops.h~a
> > +++ a/include/asm-arm/bitops.h
> > @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
> >   * the clz instruction for much better code efficiency.
> >   */
> >  
> > -#define fls(x) \
> > +#define __fls(x) \
> >  	( __builtin_constant_p(x) ? constant_fls(x) : \
> >  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> > +
> > +/* Implement fls() in C so that 64-bit args are suitably truncated */
> > +static inline int fls(int x)
> > +{
> > +	return __fls(x);
> > +}
> > +
> 
> Well, I like it more as it fixes all possible places instead of only
> fls64().
> 
> But... can't we just move the #define body into the inline fls(x)?
> Will there be other users of __fls(x)? It seems the
> __builtin_constant_p(x) works for inline functions.

Could.  That was a minimal&safe thing.

> The above patch fixes the kernel panic, too.

OK, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-13 21:55         ` Andrew Morton
@ 2008-07-23  4:52           ` Willy Tarreau
  2008-07-23  5:00             ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Willy Tarreau @ 2008-07-23  4:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Krzysztof Halasa, linux-kernel, netdev, Stephen Hemminger,
	David S. Miller, Russell King

On Sun, Jul 13, 2008 at 02:55:25PM -0700, Andrew Morton wrote:
> On Sun, 13 Jul 2008 23:51:06 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:
> 
> > Andrew Morton <akpm@linux-foundation.org> writes:
> > 
> > > --- a/include/asm-arm/bitops.h~a
> > > +++ a/include/asm-arm/bitops.h
> > > @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
> > >   * the clz instruction for much better code efficiency.
> > >   */
> > >  
> > > -#define fls(x) \
> > > +#define __fls(x) \
> > >  	( __builtin_constant_p(x) ? constant_fls(x) : \
> > >  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> > > +
> > > +/* Implement fls() in C so that 64-bit args are suitably truncated */
> > > +static inline int fls(int x)
> > > +{
> > > +	return __fls(x);
> > > +}
> > > +
> > 
> > Well, I like it more as it fixes all possible places instead of only
> > fls64().
> > 
> > But... can't we just move the #define body into the inline fls(x)?
> > Will there be other users of __fls(x)? It seems the
> > __builtin_constant_p(x) works for inline functions.
> 
> Could.  That was a minimal&safe thing.
> 
> > The above patch fixes the kernel panic, too.
> 
> OK, thanks.

Andrew,

have you sent your patch to Linus ? I haven't seen it merged yet, and
I'd like to get it into -stable too.

Thanks,
Willy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-23  4:52           ` Willy Tarreau
@ 2008-07-23  5:00             ` Andrew Morton
  2008-07-23  5:06               ` Willy Tarreau
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2008-07-23  5:00 UTC (permalink / raw)
  To: Willy Tarreau
  Cc: Krzysztof Halasa, linux-kernel, netdev, Stephen Hemminger,
	David S. Miller, Russell King

On Wed, 23 Jul 2008 06:52:31 +0200 Willy Tarreau <w@1wt.eu> wrote:

> On Sun, Jul 13, 2008 at 02:55:25PM -0700, Andrew Morton wrote:
> > On Sun, 13 Jul 2008 23:51:06 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > 
> > > Andrew Morton <akpm@linux-foundation.org> writes:
> > > 
> > > > --- a/include/asm-arm/bitops.h~a
> > > > +++ a/include/asm-arm/bitops.h
> > > > @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
> > > >   * the clz instruction for much better code efficiency.
> > > >   */
> > > >  
> > > > -#define fls(x) \
> > > > +#define __fls(x) \
> > > >  	( __builtin_constant_p(x) ? constant_fls(x) : \
> > > >  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> > > > +
> > > > +/* Implement fls() in C so that 64-bit args are suitably truncated */
> > > > +static inline int fls(int x)
> > > > +{
> > > > +	return __fls(x);
> > > > +}
> > > > +
> > > 
> > > Well, I like it more as it fixes all possible places instead of only
> > > fls64().
> > > 
> > > But... can't we just move the #define body into the inline fls(x)?
> > > Will there be other users of __fls(x)? It seems the
> > > __builtin_constant_p(x) works for inline functions.
> > 
> > Could.  That was a minimal&safe thing.
> > 
> > > The above patch fixes the kernel panic, too.
> > 
> > OK, thanks.
> 
> Andrew,
> 
> have you sent your patch to Linus ? I haven't seen it merged yet, and
> I'd like to get it into -stable too.

This one?

From: Andrew Morton <akpm@linux-foundation.org>

arm's fls() is implemented as a macro, causing it to misbehave when passed
64-bit arguments.  Fix.

Cc: Nickolay Vinogradov <nickolay@protei.ru>
Tested-by: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/asm-arm/bitops.h |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff -puN include/asm-arm/bitops.h~arm-fix-fls-for-64-bit-arguments include/asm-arm/bitops.h
--- a/include/asm-arm/bitops.h~arm-fix-fls-for-64-bit-arguments
+++ a/include/asm-arm/bitops.h
@@ -277,9 +277,16 @@ static inline int constant_fls(int x)
  * the clz instruction for much better code efficiency.
  */
 
-#define fls(x) \
+#define __fls(x) \
 	( __builtin_constant_p(x) ? constant_fls(x) : \
 	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
+
+/* Implement fls() in C so that 64-bit args are suitably truncated */
+static inline int fls(int x)
+{
+	return __fls(x);
+}
+
 #define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
 #define __ffs(x) (ffs(x) - 1)
 #define ffz(x) __ffs( ~(x) )
_


Nope, I'm just kind of sitting on it.  I'll put a stable@kernel.org tag
on it and send it to Russell I guess.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+
  2008-07-23  5:00             ` Andrew Morton
@ 2008-07-23  5:06               ` Willy Tarreau
  0 siblings, 0 replies; 9+ messages in thread
From: Willy Tarreau @ 2008-07-23  5:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Krzysztof Halasa, linux-kernel, netdev, Stephen Hemminger,
	David S. Miller, Russell King

On Tue, Jul 22, 2008 at 10:00:56PM -0700, Andrew Morton wrote:
> On Wed, 23 Jul 2008 06:52:31 +0200 Willy Tarreau <w@1wt.eu> wrote:
> 
> > On Sun, Jul 13, 2008 at 02:55:25PM -0700, Andrew Morton wrote:
> > > On Sun, 13 Jul 2008 23:51:06 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > > 
> > > > Andrew Morton <akpm@linux-foundation.org> writes:
> > > > 
> > > > > --- a/include/asm-arm/bitops.h~a
> > > > > +++ a/include/asm-arm/bitops.h
> > > > > @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
> > > > >   * the clz instruction for much better code efficiency.
> > > > >   */
> > > > >  
> > > > > -#define fls(x) \
> > > > > +#define __fls(x) \
> > > > >  	( __builtin_constant_p(x) ? constant_fls(x) : \
> > > > >  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> > > > > +
> > > > > +/* Implement fls() in C so that 64-bit args are suitably truncated */
> > > > > +static inline int fls(int x)
> > > > > +{
> > > > > +	return __fls(x);
> > > > > +}
> > > > > +
> > > > 
> > > > Well, I like it more as it fixes all possible places instead of only
> > > > fls64().
> > > > 
> > > > But... can't we just move the #define body into the inline fls(x)?
> > > > Will there be other users of __fls(x)? It seems the
> > > > __builtin_constant_p(x) works for inline functions.
> > > 
> > > Could.  That was a minimal&safe thing.
> > > 
> > > > The above patch fixes the kernel panic, too.
> > > 
> > > OK, thanks.
> > 
> > Andrew,
> > 
> > have you sent your patch to Linus ? I haven't seen it merged yet, and
> > I'd like to get it into -stable too.
> 
> This one?
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> arm's fls() is implemented as a macro, causing it to misbehave when passed
> 64-bit arguments.  Fix.
> 
> Cc: Nickolay Vinogradov <nickolay@protei.ru>
> Tested-by: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  include/asm-arm/bitops.h |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff -puN include/asm-arm/bitops.h~arm-fix-fls-for-64-bit-arguments include/asm-arm/bitops.h
> --- a/include/asm-arm/bitops.h~arm-fix-fls-for-64-bit-arguments
> +++ a/include/asm-arm/bitops.h
> @@ -277,9 +277,16 @@ static inline int constant_fls(int x)
>   * the clz instruction for much better code efficiency.
>   */
>  
> -#define fls(x) \
> +#define __fls(x) \
>  	( __builtin_constant_p(x) ? constant_fls(x) : \
>  	  ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc"); 32-__r; }) )
> +
> +/* Implement fls() in C so that 64-bit args are suitably truncated */
> +static inline int fls(int x)
> +{
> +	return __fls(x);
> +}
> +
>  #define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
>  #define __ffs(x) (ffs(x) - 1)
>  #define ffz(x) __ffs( ~(x) )
> _
> 

yes, precisely this one.

> Nope, I'm just kind of sitting on it.  I'll put a stable@kernel.org tag
> on it and send it to Russell I guess.

Perfect, thanks!
Willy


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-07-23  5:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-13  0:28 [bisected] kernel panic 2.6.22 -> 2.6.26-rc9+ Krzysztof Halasa
2008-07-13  8:31 ` Andrew Morton
2008-07-13 10:48   ` Krzysztof Halasa
2008-07-13 18:22     ` Andrew Morton
2008-07-13 21:51       ` Krzysztof Halasa
2008-07-13 21:55         ` Andrew Morton
2008-07-23  4:52           ` Willy Tarreau
2008-07-23  5:00             ` Andrew Morton
2008-07-23  5:06               ` Willy Tarreau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).