Re: [ovs-dev] [PATCH 1/1] netflow: Modify netflow_flow_clear() to do netflow_expire__

2014-06-23 Thread Anoob Soman

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__

2014-06-09 Thread Ben Pfaff
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__

2014-05-27 Thread Anoob Soman
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__

2014-05-20 Thread Anoob Soman
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__

2014-05-20 Thread Ben Pfaff
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