Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__
Thanks. -Anoob On 09/06/14 19:10, Ben Pfaff wrote: Sorry about the delay. I took a few minutes to properly understand the situation this morning. I applied the following form of your patch to master and branch-2.[123]: --8<--cut here-->8-- From: Anoob Soman Date: Tue, 20 May 2014 12:40:35 +0100 Subject: [PATCH] netflow: Fold netflow_expire() into netflow_flow_clear(). netflow_flow_clear() asserted that no packets or bytes were included in the statistics for the flow being cleared. Before threading Open vSwitch, this assertion was always true because netflow_expire() was always called before calling netflow_flow_clear(). Since Open vSwitch was threaded, however, it was possible that a packet arrived after netflow_expire() but before netflow_flow_clear(), since each of these function separately took the netflow mutex. This commit fixes the problem by merging netflow_expire() into netflow_flow_clear(), under a single acquisition of the netflow mutex. Signed-off-by: Anoob Soman [b...@nicira.com modified the patch to remove netflow_expire() and rewrote the commit message] Signed-off-by: Ben Pfaff --- ofproto/netflow.c | 16 +--- ofproto/netflow.h |3 +-- ofproto/ofproto-dpif-upcall.c |2 -- ofproto/ofproto-dpif-xlate.c |1 - 4 files changed, 2 insertions(+), 20 deletions(-) diff --git a/ofproto/netflow.c b/ofproto/netflow.c index e9382af..c7af010 100644 --- a/ofproto/netflow.c +++ b/ofproto/netflow.c @@ -276,19 +276,6 @@ netflow_expire__(struct netflow *nf, struct netflow_flow *nf_flow) } void -netflow_expire(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) -{ -struct netflow_flow *nf_flow; - -ovs_mutex_lock(&mutex); -nf_flow = netflow_flow_lookup(nf, flow); -if (nf_flow) { -netflow_expire__(nf, nf_flow); -} -ovs_mutex_unlock(&mutex); -} - -void netflow_flow_clear(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) { struct netflow_flow *nf_flow; @@ -296,8 +283,7 @@ netflow_flow_clear(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) ovs_mutex_lock(&mutex); nf_flow = netflow_flow_lookup(nf, flow); if (nf_flow) { -ovs_assert(!nf_flow->packet_count); -ovs_assert(!nf_flow->byte_count); +netflow_expire__(nf, nf_flow); hmap_remove(&nf->flows, &nf_flow->hmap_node); free(nf_flow); } diff --git a/ofproto/netflow.h b/ofproto/netflow.h index e89b75e..94dd3ff 100644 --- a/ofproto/netflow.h +++ b/ofproto/netflow.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2014 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -46,7 +46,6 @@ void netflow_unref(struct netflow *); bool netflow_exists(void); int netflow_set_options(struct netflow *, const struct netflow_options *); -void netflow_expire(struct netflow *, struct flow *); void netflow_run(struct netflow *); void netflow_wait(struct netflow *); diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 67a0ed8..d38b843 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1219,7 +1219,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, exit: if (netflow) { if (!ok) { -netflow_expire(netflow, &flow); netflow_flow_clear(netflow, &flow); } netflow_unref(netflow); @@ -1304,7 +1303,6 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops) xlate_actions_for_side_effects(&xin); if (netflow) { -netflow_expire(netflow, &flow); netflow_flow_clear(netflow, &flow); netflow_unref(netflow); } diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index eded9d8..71eaad1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3956,7 +3956,6 @@ xlate_dev_unref(struct xc_entry *entry) static void xlate_cache_clear_netflow(struct netflow *netflow, struct flow *flow) { -netflow_expire(netflow, flow); netflow_flow_clear(netflow, flow); netflow_unref(netflow); free(flow); ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__
Sorry about the delay. I took a few minutes to properly understand the situation this morning. I applied the following form of your patch to master and branch-2.[123]: --8<--cut here-->8-- From: Anoob Soman Date: Tue, 20 May 2014 12:40:35 +0100 Subject: [PATCH] netflow: Fold netflow_expire() into netflow_flow_clear(). netflow_flow_clear() asserted that no packets or bytes were included in the statistics for the flow being cleared. Before threading Open vSwitch, this assertion was always true because netflow_expire() was always called before calling netflow_flow_clear(). Since Open vSwitch was threaded, however, it was possible that a packet arrived after netflow_expire() but before netflow_flow_clear(), since each of these function separately took the netflow mutex. This commit fixes the problem by merging netflow_expire() into netflow_flow_clear(), under a single acquisition of the netflow mutex. Signed-off-by: Anoob Soman [b...@nicira.com modified the patch to remove netflow_expire() and rewrote the commit message] Signed-off-by: Ben Pfaff --- ofproto/netflow.c | 16 +--- ofproto/netflow.h |3 +-- ofproto/ofproto-dpif-upcall.c |2 -- ofproto/ofproto-dpif-xlate.c |1 - 4 files changed, 2 insertions(+), 20 deletions(-) diff --git a/ofproto/netflow.c b/ofproto/netflow.c index e9382af..c7af010 100644 --- a/ofproto/netflow.c +++ b/ofproto/netflow.c @@ -276,19 +276,6 @@ netflow_expire__(struct netflow *nf, struct netflow_flow *nf_flow) } void -netflow_expire(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) -{ -struct netflow_flow *nf_flow; - -ovs_mutex_lock(&mutex); -nf_flow = netflow_flow_lookup(nf, flow); -if (nf_flow) { -netflow_expire__(nf, nf_flow); -} -ovs_mutex_unlock(&mutex); -} - -void netflow_flow_clear(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) { struct netflow_flow *nf_flow; @@ -296,8 +283,7 @@ netflow_flow_clear(struct netflow *nf, struct flow *flow) OVS_EXCLUDED(mutex) ovs_mutex_lock(&mutex); nf_flow = netflow_flow_lookup(nf, flow); if (nf_flow) { -ovs_assert(!nf_flow->packet_count); -ovs_assert(!nf_flow->byte_count); +netflow_expire__(nf, nf_flow); hmap_remove(&nf->flows, &nf_flow->hmap_node); free(nf_flow); } diff --git a/ofproto/netflow.h b/ofproto/netflow.h index e89b75e..94dd3ff 100644 --- a/ofproto/netflow.h +++ b/ofproto/netflow.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2014 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -46,7 +46,6 @@ void netflow_unref(struct netflow *); bool netflow_exists(void); int netflow_set_options(struct netflow *, const struct netflow_options *); -void netflow_expire(struct netflow *, struct flow *); void netflow_run(struct netflow *); void netflow_wait(struct netflow *); diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 67a0ed8..d38b843 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1219,7 +1219,6 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, exit: if (netflow) { if (!ok) { -netflow_expire(netflow, &flow); netflow_flow_clear(netflow, &flow); } netflow_unref(netflow); @@ -1304,7 +1303,6 @@ push_dump_ops__(struct udpif *udpif, struct dump_op *ops, size_t n_ops) xlate_actions_for_side_effects(&xin); if (netflow) { -netflow_expire(netflow, &flow); netflow_flow_clear(netflow, &flow); netflow_unref(netflow); } diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index eded9d8..71eaad1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3956,7 +3956,6 @@ xlate_dev_unref(struct xc_entry *entry) static void xlate_cache_clear_netflow(struct netflow *netflow, struct flow *flow) { -netflow_expire(netflow, flow); netflow_flow_clear(netflow, flow); netflow_unref(netflow); free(flow); -- 1.7.10.4 On Tue, May 27, 2014 at 04:27:44PM +, Anoob Soman wrote: > Hi Ben, > > Do you have comments on the scenario in which the assert might happen ? > > Thanks, > Anoob. > > From: Anoob Soman > Sent: 20 May 2014 20:06 > To: Ben Pfaff > Cc: dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do > netflow_expire__ > > Sorry, I should have made it clear in the commit message. In our testing, > occasionally
Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__
Hi Ben, Do you have comments on the scenario in which the assert might happen ? Thanks, Anoob. From: Anoob Soman Sent: 20 May 2014 20:06 To: Ben Pfaff Cc: dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__ Sorry, I should have made it clear in the commit message. In our testing, occasionally ovs_assert(!nf_flow->packet_count) would fail, with the following backtrace #0 0x7f7d263ad265 in raise () from /lib64/libc.so.6 #1 0x7f7d263aed10 in abort () from /lib64/libc.so.6 #2 0x004a3b1e in ovs_abort_valist (err_no=, format=, args=) at lib/util.c:245 #3 0x004a8e78 in vlog_abort_valist (module_=, message=0x4f9410 "%s: assertion %s failed in %s()", args=0x42e3cfe0) at lib/vlog.c:992 #4 0x004a8f06 in vlog_abort (module=0x10a7, message=0x1192 ) at lib/vlog.c:1006 #5 0x004a3dab in ovs_assert_failure (where=0x6 , function=0x80 , condition=0x ) at lib/util.c:68 #6 0x00437db6 in netflow_flow_clear (nf=0x8722a0, flow=0x42e3ee20) at ofproto/netflow.c:299 #7 0x0042c6fb in revalidate_udumps (arg=0x7a4fe0) at ofproto/ofproto-dpif-upcall.c:1391 #8 udpif_revalidator (arg=0x7a4fe0) at ofproto/ofproto-dpif-upcall.c:724 #9 0x7f7d2616873d in __free_tcb () from /lib64/libpthread.so.0 Though netflow_flow_clear() and netflow_expire() are always called together, there is a slight chance that the netflow stats are updated between the two calls. With my patch, netflow_expire() is dead code (no one calls it). But I have retained it for future use, if someone wants to expire netflow, without explicitly taking locks. Thanks, Anoob. From: Ben Pfaff [b...@nicira.com] Sent: 20 May 2014 19:25 To: Anoob Soman Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__ On Tue, May 20, 2014 at 12:40:35PM +0100, Anoob Soman wrote: > This avoids causing failed assert(), when netflow_flow_clear() and > netflow_expire() are called together. > > Signed-off-by: Anoob Soman I broke out the fix to the spelling of your name in AUTHORS (my fault, sorry!) and pushed that. Can you explain a little more about the remainder of the patch? Does the assertion trigger in practice (we have not received reports of problems)? Can you explain the code flow that triggers it? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__
Sorry, I should have made it clear in the commit message. In our testing, occasionally ovs_assert(!nf_flow->packet_count) would fail, with the following backtrace #0 0x7f7d263ad265 in raise () from /lib64/libc.so.6 #1 0x7f7d263aed10 in abort () from /lib64/libc.so.6 #2 0x004a3b1e in ovs_abort_valist (err_no=, format=, args=) at lib/util.c:245 #3 0x004a8e78 in vlog_abort_valist (module_=, message=0x4f9410 "%s: assertion %s failed in %s()", args=0x42e3cfe0) at lib/vlog.c:992 #4 0x004a8f06 in vlog_abort (module=0x10a7, message=0x1192 ) at lib/vlog.c:1006 #5 0x004a3dab in ovs_assert_failure (where=0x6 , function=0x80 , condition=0x ) at lib/util.c:68 #6 0x00437db6 in netflow_flow_clear (nf=0x8722a0, flow=0x42e3ee20) at ofproto/netflow.c:299 #7 0x0042c6fb in revalidate_udumps (arg=0x7a4fe0) at ofproto/ofproto-dpif-upcall.c:1391 #8 udpif_revalidator (arg=0x7a4fe0) at ofproto/ofproto-dpif-upcall.c:724 #9 0x7f7d2616873d in __free_tcb () from /lib64/libpthread.so.0 Though netflow_flow_clear() and netflow_expire() are always called together, there is a slight chance that the netflow stats are updated between the two calls. With my patch, netflow_expire() is dead code (no one calls it). But I have retained it for future use, if someone wants to expire netflow, without explicitly taking locks. Thanks, Anoob. From: Ben Pfaff [b...@nicira.com] Sent: 20 May 2014 19:25 To: Anoob Soman Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__ On Tue, May 20, 2014 at 12:40:35PM +0100, Anoob Soman wrote: > This avoids causing failed assert(), when netflow_flow_clear() and > netflow_expire() are called together. > > Signed-off-by: Anoob Soman I broke out the fix to the spelling of your name in AUTHORS (my fault, sorry!) and pushed that. Can you explain a little more about the remainder of the patch? Does the assertion trigger in practice (we have not received reports of problems)? Can you explain the code flow that triggers it? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__
On Tue, May 20, 2014 at 12:40:35PM +0100, Anoob Soman wrote: > This avoids causing failed assert(), when netflow_flow_clear() and > netflow_expire() are called together. > > Signed-off-by: Anoob Soman I broke out the fix to the spelling of your name in AUTHORS (my fault, sorry!) and pushed that. Can you explain a little more about the remainder of the patch? Does the assertion trigger in practice (we have not received reports of problems)? Can you explain the code flow that triggers it? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev