[PATCH] net: use raw_cpu ops in snmp stats bh

[PATCH] net: use raw_cpu ops in snmp stats bh

[Sergey Senozhatsky]

Commit a25982c15ae52 ("percpu: add preemption checks to __this_cpu
ops") added preemption checks to __this_cpu ops, which are used in
SNMP_INC_STATS_BH() and SNMP_ADD_STATS_BH(), resulting in following
warnings:

BUG: using __this_cpu_add() in preemptible [00000000] code: ssh/4661
caller is __this_cpu_preempt_check+0x2b/0x2d
CPU: 3 PID: 4661 Comm: ssh Not tainted 3.14.0-rc5-next-20140306-dbg-dirty #162
Hardware name: Acer             Aspire 5741G    /Aspire 5741G    , BIOS V1.20 02/08/2011
 0000000000000000 ffff880094939be0 ffffffff813b8305 0000000000000003
 ffff880094939c00 ffffffff8121221f ffff88015130a8c0 0000000000000000
 ffff880094939c38 ffffffff8121226b 635f736968745f5f 29286464615f7570
Call Trace:
 [<ffffffff813b8305>] dump_stack+0x4e/0x7a
 [<ffffffff8121221f>] check_preemption_disabled+0xce/0xdd
 [<ffffffff8121226b>] __this_cpu_preempt_check+0x2b/0x2d
 [<ffffffff81370b5a>] ? tcp_rearm_rto+0x9f/0xa1
 [<ffffffff81373456>] tcp_event_new_data_sent+0x6d/0x91
 [<ffffffff81374dc6>] tcp_write_xmit+0x3f1/0x935
 [<ffffffff813754e4>] __tcp_push_pending_frames+0x28/0x82
 [<ffffffff81368109>] tcp_push+0xcc/0x102
 [<ffffffff8136a0c1>] tcp_sendmsg+0x8c5/0xb7a
 [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
 [<ffffffff8138a10a>] inet_sendmsg+0x75/0xa0
 [<ffffffff81320fb6>] sock_aio_write+0xe7/0xfe
 [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
 [<ffffffff810e08ab>] do_sync_write+0x54/0x73
 [<ffffffff810e0df7>] vfs_write+0xc6/0x17c
 [<ffffffff810e1603>] SyS_write+0x44/0x78
 [<ffffffff813be0a1>] tracesys+0xd4/0xd9

use raw_cpu_inc() and raw_cpu_add() instead of __this_cpu_inc()
and __this_cpu_add().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 include/net/snmp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/snmp.h b/include/net/snmp.h
index 36ab852..b549ad6 100644
--- a/include/net/snmp.h
+++ b/include/net/snmp.h
@@ -126,7 +126,7 @@ struct linux_xfrm_mib {
 	extern __typeof__(type) __percpu *name[SNMP_ARRAY_SZ]
 
 #define SNMP_INC_STATS_BH(mib, field)	\
-			__this_cpu_inc(mib[0]->mibs[field])
+			raw_cpu_inc(mib[0]->mibs[field])
 
 #define SNMP_INC_STATS_USER(mib, field)	\
 			this_cpu_inc(mib[0]->mibs[field])
@@ -141,7 +141,7 @@ struct linux_xfrm_mib {
 			this_cpu_dec(mib[0]->mibs[field])
 
 #define SNMP_ADD_STATS_BH(mib, field, addend)	\
-			__this_cpu_add(mib[0]->mibs[field], addend)
+			raw_cpu_add(mib[0]->mibs[field], addend)
 
 #define SNMP_ADD_STATS_USER(mib, field, addend)	\
 			this_cpu_add(mib[0]->mibs[field], addend)
-- 
1.9.0.382.g7f3562c

Re: [PATCH] net: use raw_cpu ops in snmp stats bh

[Eric Dumazet]

On Thu, 2014-03-06 at 15:55 +0300, Sergey Senozhatsky wrote:
> Commit a25982c15ae52 ("percpu: add preemption checks to __this_cpu
> ops") added preemption checks to __this_cpu ops, which are used in
> SNMP_INC_STATS_BH() and SNMP_ADD_STATS_BH(), resulting in following
> warnings:
> 
> BUG: using __this_cpu_add() in preemptible [00000000] code: ssh/4661
> caller is __this_cpu_preempt_check+0x2b/0x2d
> CPU: 3 PID: 4661 Comm: ssh Not tainted 3.14.0-rc5-next-20140306-dbg-dirty #162
> Hardware name: Acer             Aspire 5741G    /Aspire 5741G    , BIOS V1.20 02/08/2011
>  0000000000000000 ffff880094939be0 ffffffff813b8305 0000000000000003
>  ffff880094939c00 ffffffff8121221f ffff88015130a8c0 0000000000000000
>  ffff880094939c38 ffffffff8121226b 635f736968745f5f 29286464615f7570
> Call Trace:
>  [<ffffffff813b8305>] dump_stack+0x4e/0x7a
>  [<ffffffff8121221f>] check_preemption_disabled+0xce/0xdd
>  [<ffffffff8121226b>] __this_cpu_preempt_check+0x2b/0x2d
>  [<ffffffff81370b5a>] ? tcp_rearm_rto+0x9f/0xa1
>  [<ffffffff81373456>] tcp_event_new_data_sent+0x6d/0x91
>  [<ffffffff81374dc6>] tcp_write_xmit+0x3f1/0x935
>  [<ffffffff813754e4>] __tcp_push_pending_frames+0x28/0x82
>  [<ffffffff81368109>] tcp_push+0xcc/0x102
>  [<ffffffff8136a0c1>] tcp_sendmsg+0x8c5/0xb7a
>  [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
>  [<ffffffff8138a10a>] inet_sendmsg+0x75/0xa0
>  [<ffffffff81320fb6>] sock_aio_write+0xe7/0xfe
>  [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
>  [<ffffffff810e08ab>] do_sync_write+0x54/0x73
>  [<ffffffff810e0df7>] vfs_write+0xc6/0x17c
>  [<ffffffff810e1603>] SyS_write+0x44/0x78
>  [<ffffffff813be0a1>] tracesys+0xd4/0xd9
> 
> use raw_cpu_inc() and raw_cpu_add() instead of __this_cpu_inc()
> and __this_cpu_add().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  include/net/snmp.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I think you missed a lot of mails sent by Chrisoph Lameter recently...

Re: [PATCH] net: use raw_cpu ops in snmp stats bh

[Eric Dumazet]

On Thu, 2014-03-06 at 06:44 -0800, Eric Dumazet wrote:

> I think you missed a lot of mails sent by Chrisoph Lameter recently...

s/Chrisoph/Christoph/

Appropriate fix would be the following one, please submit this formally.


Fixes: f19c29e3e391 ("tcp: snmp stats for Fast Open, SYN rtx, and data pkts")

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5286228679bd..df063a32a177 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -87,7 +87,7 @@ static void tcp_event_new_data_sent(struct sock *sk, const struct sk_buff *skb)
 		tcp_rearm_rto(sk);
 	}
 
-	NET_ADD_STATS_BH(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
+	NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
 			 tcp_skb_pcount(skb));
 }
 

Re: [PATCH] net: use raw_cpu ops in snmp stats bh

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 06 Mar 2014 07:04:32 -0800

> On Thu, 2014-03-06 at 06:44 -0800, Eric Dumazet wrote:
> 
>> I think you missed a lot of mails sent by Chrisoph Lameter recently...
> 
> s/Chrisoph/Christoph/
> 
> Appropriate fix would be the following one, please submit this formally.
> 
> 
> Fixes: f19c29e3e391 ("tcp: snmp stats for Fast Open, SYN rtx, and data pkts")
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 5286228679bd..df063a32a177 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -87,7 +87,7 @@ static void tcp_event_new_data_sent(struct sock *sk, const struct sk_buff *skb)
>  		tcp_rearm_rto(sk);
>  	}
>  
> -	NET_ADD_STATS_BH(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
> +	NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
>  			 tcp_skb_pcount(skb));
>  }

I'll take care of this, as follows:

====================
>From 2997439191bc16340abcf51b18a7152b372a057e Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Thu, 6 Mar 2014 15:03:17 -0500
Subject: [PATCH] tcp: Use NET_ADD_STATS instead of NET_ADD_STATS_BH in
 tcp_event_new_data_sent()

Can be invoked from non-BH context.

Based upon a patch by Eric Dumazet.

Fixes: f19c29e3e391 ("tcp: snmp stats for Fast Open, SYN rtx, and data pkts")
Reported-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/net/ip.h      | 1 +
 net/ipv4/tcp_output.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index b885d75..25064c2 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -187,6 +187,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
 #define NET_INC_STATS(net, field)	SNMP_INC_STATS((net)->mib.net_statistics, field)
 #define NET_INC_STATS_BH(net, field)	SNMP_INC_STATS_BH((net)->mib.net_statistics, field)
 #define NET_INC_STATS_USER(net, field) 	SNMP_INC_STATS_USER((net)->mib.net_statistics, field)
+#define NET_ADD_STATS(net, field, adnd)	SNMP_ADD_STATS((net)->mib.net_statistics, field, adnd)
 #define NET_ADD_STATS_BH(net, field, adnd) SNMP_ADD_STATS_BH((net)->mib.net_statistics, field, adnd)
 #define NET_ADD_STATS_USER(net, field, adnd) SNMP_ADD_STATS_USER((net)->mib.net_statistics, field, adnd)
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5286228..a02c884 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -87,8 +87,8 @@ static void tcp_event_new_data_sent(struct sock *sk, const struct sk_buff *skb)
 		tcp_rearm_rto(sk);
 	}
 
-	NET_ADD_STATS_BH(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
-			 tcp_skb_pcount(skb));
+	NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT,
+		      tcp_skb_pcount(skb));
 }
 
 /* SND.NXT, if window was not shrunk.
-- 
1.7.11.7

Re: [PATCH] net: use raw_cpu ops in snmp stats bh

[David Miller]

From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Date: Thu,  6 Mar 2014 15:55:36 +0300

> Commit a25982c15ae52 ("percpu: add preemption checks to __this_cpu
> ops") added preemption checks to __this_cpu ops, which are used in
> SNMP_INC_STATS_BH() and SNMP_ADD_STATS_BH(), resulting in following
> warnings:
> 
> BUG: using __this_cpu_add() in preemptible [00000000] code: ssh/4661
> caller is __this_cpu_preempt_check+0x2b/0x2d
> CPU: 3 PID: 4661 Comm: ssh Not tainted 3.14.0-rc5-next-20140306-dbg-dirty #162
> Hardware name: Acer             Aspire 5741G    /Aspire 5741G    , BIOS V1.20 02/08/2011
>  0000000000000000 ffff880094939be0 ffffffff813b8305 0000000000000003
>  ffff880094939c00 ffffffff8121221f ffff88015130a8c0 0000000000000000
>  ffff880094939c38 ffffffff8121226b 635f736968745f5f 29286464615f7570
> Call Trace:
>  [<ffffffff813b8305>] dump_stack+0x4e/0x7a
>  [<ffffffff8121221f>] check_preemption_disabled+0xce/0xdd
>  [<ffffffff8121226b>] __this_cpu_preempt_check+0x2b/0x2d
>  [<ffffffff81370b5a>] ? tcp_rearm_rto+0x9f/0xa1
>  [<ffffffff81373456>] tcp_event_new_data_sent+0x6d/0x91
>  [<ffffffff81374dc6>] tcp_write_xmit+0x3f1/0x935
>  [<ffffffff813754e4>] __tcp_push_pending_frames+0x28/0x82
>  [<ffffffff81368109>] tcp_push+0xcc/0x102
>  [<ffffffff8136a0c1>] tcp_sendmsg+0x8c5/0xb7a
>  [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
>  [<ffffffff8138a10a>] inet_sendmsg+0x75/0xa0
>  [<ffffffff81320fb6>] sock_aio_write+0xe7/0xfe
>  [<ffffffff81207d59>] ? string.isra.6+0x3b/0x9f
>  [<ffffffff810e08ab>] do_sync_write+0x54/0x73
>  [<ffffffff810e0df7>] vfs_write+0xc6/0x17c
>  [<ffffffff810e1603>] SyS_write+0x44/0x78
>  [<ffffffff813be0a1>] tracesys+0xd4/0xd9
> 
> use raw_cpu_inc() and raw_cpu_add() instead of __this_cpu_inc()
> and __this_cpu_add().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

This is a bug in the TCP code, if this code path can happen outside of
software interrupts or BH protected sections, which it can as seen in
this trace, it must use the non-_BH-postfixed versions of the SNMP
counter bumps.