* [PATCH] drop_monitor: allow more events per second
@ 2012-04-19 17:16 Eric Dumazet
2012-04-19 17:28 ` Neil Horman
2012-04-21 20:29 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2012-04-19 17:16 UTC (permalink / raw)
To: Neil Horman, David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
It seems there is a logic error in trace_drop_common(), since we store
only 64 drops, even if they are from same location.
This fix is a one liner, but we probably need more work to avoid useless
atomic dec/inc
Now I can watch 1 Mpps drops through dropwatch...
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
Neil, it seems this code is not SMP/preempt safe. Worker can free our
data under us, and genlmsg_new() can return NULL under stress.
net/core/drop_monitor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 7f36b38..5c3c81a 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -150,6 +150,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
for (i = 0; i < msg->entries; i++) {
if (!memcmp(&location, msg->points[i].pc, sizeof(void *))) {
msg->points[i].count++;
+ atomic_inc(&data->dm_hit_count);
goto out;
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drop_monitor: allow more events per second
2012-04-19 17:16 [PATCH] drop_monitor: allow more events per second Eric Dumazet
@ 2012-04-19 17:28 ` Neil Horman
2012-04-19 21:25 ` Eric Dumazet
2012-04-21 20:29 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Neil Horman @ 2012-04-19 17:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Thu, Apr 19, 2012 at 07:16:21PM +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> It seems there is a logic error in trace_drop_common(), since we store
> only 64 drops, even if they are from same location.
>
> This fix is a one liner, but we probably need more work to avoid useless
> atomic dec/inc
>
> Now I can watch 1 Mpps drops through dropwatch...
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> ---
> Neil, it seems this code is not SMP/preempt safe. Worker can free our
> data under us, and genlmsg_new() can return NULL under stress.
>
> net/core/drop_monitor.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 7f36b38..5c3c81a 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -150,6 +150,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> for (i = 0; i < msg->entries; i++) {
> if (!memcmp(&location, msg->points[i].pc, sizeof(void *))) {
> msg->points[i].count++;
> + atomic_inc(&data->dm_hit_count);
> goto out;
> }
> }
>
>
>
I spent a good deal of time going through it to make sure it was preempt safe,
but its certainly possible that I missed something. I'll look at it shortly.
Thanks for this update though
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drop_monitor: allow more events per second
2012-04-19 17:28 ` Neil Horman
@ 2012-04-19 21:25 ` Eric Dumazet
2012-04-20 13:06 ` Neil Horman
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2012-04-19 21:25 UTC (permalink / raw)
To: Neil Horman; +Cc: David Miller, netdev
Le jeudi 19 avril 2012 à 13:28 -0400, Neil Horman a écrit :
> I spent a good deal of time going through it to make sure it was preempt safe,
> but its certainly possible that I missed something. I'll look at it shortly.
> Thanks for this update though
>
You might want to check this problem as well :
[ 38.352571] BUG: sleeping function called from invalid context at kernel/mutex.c:85
[ 38.352576] in_atomic(): 1, irqs_disabled(): 0, pid: 4415, name: dropwatch
[ 38.352580] Pid: 4415, comm: dropwatch Not tainted 3.4.0-rc2+ #71
[ 38.352582] Call Trace:
[ 38.352592] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
[ 38.352599] [<ffffffff81063f2a>] __might_sleep+0xca/0xf0
[ 38.352606] [<ffffffff81655b16>] mutex_lock+0x26/0x50
[ 38.352610] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
[ 38.352616] [<ffffffff810b72d9>] tracepoint_probe_register+0x29/0x90
[ 38.352621] [<ffffffff8153a585>] set_all_monitor_traces+0x105/0x170
[ 38.352625] [<ffffffff8153a8ca>] net_dm_cmd_trace+0x2a/0x40
[ 38.352630] [<ffffffff8154a81a>] genl_rcv_msg+0x21a/0x2b0
[ 38.352636] [<ffffffff810f8029>] ? zone_statistics+0x99/0xc0
[ 38.352640] [<ffffffff8154a600>] ? genl_rcv+0x30/0x30
[ 38.352645] [<ffffffff8154a059>] netlink_rcv_skb+0xa9/0xd0
[ 38.352649] [<ffffffff8154a5f0>] genl_rcv+0x20/0x30
[ 38.352653] [<ffffffff81549a7e>] netlink_unicast+0x1ae/0x1f0
[ 38.352658] [<ffffffff81549d76>] netlink_sendmsg+0x2b6/0x310
[ 38.352663] [<ffffffff8150824f>] sock_sendmsg+0x10f/0x130
[ 38.352668] [<ffffffff8150abe0>] ? move_addr_to_kernel+0x60/0xb0
[ 38.352673] [<ffffffff81515f04>] ? verify_iovec+0x64/0xe0
[ 38.352677] [<ffffffff81509c46>] __sys_sendmsg+0x386/0x390
[ 38.352682] [<ffffffff810ffaf9>] ? handle_mm_fault+0x139/0x210
[ 38.352687] [<ffffffff8165b5bc>] ? do_page_fault+0x1ec/0x4f0
[ 38.352693] [<ffffffff8106ba4d>] ? set_next_entity+0x9d/0xb0
[ 38.352699] [<ffffffff81310b49>] ? tty_ldisc_deref+0x9/0x10
[ 38.352703] [<ffffffff8106d363>] ? pick_next_task_fair+0x63/0x140
[ 38.352708] [<ffffffff8150b8d4>] sys_sendmsg+0x44/0x80
[ 38.352713] [<ffffffff8165f8e2>] system_call_fastpath+0x16/0x1b
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drop_monitor: allow more events per second
2012-04-19 21:25 ` Eric Dumazet
@ 2012-04-20 13:06 ` Neil Horman
0 siblings, 0 replies; 5+ messages in thread
From: Neil Horman @ 2012-04-20 13:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Thu, Apr 19, 2012 at 11:25:33PM +0200, Eric Dumazet wrote:
> Le jeudi 19 avril 2012 à 13:28 -0400, Neil Horman a écrit :
>
> > I spent a good deal of time going through it to make sure it was preempt safe,
> > but its certainly possible that I missed something. I'll look at it shortly.
> > Thanks for this update though
> >
>
> You might want to check this problem as well :
>
> [ 38.352571] BUG: sleeping function called from invalid context at kernel/mutex.c:85
> [ 38.352576] in_atomic(): 1, irqs_disabled(): 0, pid: 4415, name: dropwatch
> [ 38.352580] Pid: 4415, comm: dropwatch Not tainted 3.4.0-rc2+ #71
> [ 38.352582] Call Trace:
> [ 38.352592] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
> [ 38.352599] [<ffffffff81063f2a>] __might_sleep+0xca/0xf0
> [ 38.352606] [<ffffffff81655b16>] mutex_lock+0x26/0x50
> [ 38.352610] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
> [ 38.352616] [<ffffffff810b72d9>] tracepoint_probe_register+0x29/0x90
> [ 38.352621] [<ffffffff8153a585>] set_all_monitor_traces+0x105/0x170
> [ 38.352625] [<ffffffff8153a8ca>] net_dm_cmd_trace+0x2a/0x40
> [ 38.352630] [<ffffffff8154a81a>] genl_rcv_msg+0x21a/0x2b0
> [ 38.352636] [<ffffffff810f8029>] ? zone_statistics+0x99/0xc0
> [ 38.352640] [<ffffffff8154a600>] ? genl_rcv+0x30/0x30
> [ 38.352645] [<ffffffff8154a059>] netlink_rcv_skb+0xa9/0xd0
> [ 38.352649] [<ffffffff8154a5f0>] genl_rcv+0x20/0x30
> [ 38.352653] [<ffffffff81549a7e>] netlink_unicast+0x1ae/0x1f0
> [ 38.352658] [<ffffffff81549d76>] netlink_sendmsg+0x2b6/0x310
> [ 38.352663] [<ffffffff8150824f>] sock_sendmsg+0x10f/0x130
> [ 38.352668] [<ffffffff8150abe0>] ? move_addr_to_kernel+0x60/0xb0
> [ 38.352673] [<ffffffff81515f04>] ? verify_iovec+0x64/0xe0
> [ 38.352677] [<ffffffff81509c46>] __sys_sendmsg+0x386/0x390
> [ 38.352682] [<ffffffff810ffaf9>] ? handle_mm_fault+0x139/0x210
> [ 38.352687] [<ffffffff8165b5bc>] ? do_page_fault+0x1ec/0x4f0
> [ 38.352693] [<ffffffff8106ba4d>] ? set_next_entity+0x9d/0xb0
> [ 38.352699] [<ffffffff81310b49>] ? tty_ldisc_deref+0x9/0x10
> [ 38.352703] [<ffffffff8106d363>] ? pick_next_task_fair+0x63/0x140
> [ 38.352708] [<ffffffff8150b8d4>] sys_sendmsg+0x44/0x80
> [ 38.352713] [<ffffffff8165f8e2>] system_call_fastpath+0x16/0x1b
>
>
>
Will do, thanks!
Neil
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drop_monitor: allow more events per second
2012-04-19 17:16 [PATCH] drop_monitor: allow more events per second Eric Dumazet
2012-04-19 17:28 ` Neil Horman
@ 2012-04-21 20:29 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2012-04-21 20:29 UTC (permalink / raw)
To: eric.dumazet; +Cc: nhorman, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Apr 2012 19:16:21 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> It seems there is a logic error in trace_drop_common(), since we store
> only 64 drops, even if they are from same location.
>
> This fix is a one liner, but we probably need more work to avoid useless
> atomic dec/inc
>
> Now I can watch 1 Mpps drops through dropwatch...
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-21 20:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-19 17:16 [PATCH] drop_monitor: allow more events per second Eric Dumazet
2012-04-19 17:28 ` Neil Horman
2012-04-19 21:25 ` Eric Dumazet
2012-04-20 13:06 ` Neil Horman
2012-04-21 20:29 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox