[ovs-dev] [PATCH] ofproto: Honour out_port of flow monitors

Simon Horman horms at verge.net.au
Fri Jun 13 23:33:13 UTC 2014


Previously the out_port of a flow monitor was
checked in ofmonitor_report() using ofoperation_has_out_port().

When ofoperation_has_out_port() was removed so was the call to
it in ofmonitor_report() thus flow monitor updates are longer
filtered on the out_port.

This restores filtering on the out_port by using
ofproto_rule_has_out_port to check the actions of the rule.
If the actions have been changed by a modify actions then
ofpacts_output_to_port() is also used to check the old actions.

This patch also adds a test to exercise out_ports for flow monitors.

This resolves what appears to be a regression introduced by
b20f4073eecd4761 ("ofproto: Do straightforward removal of asynchronous flow
operations.")

Signed-off-by: Simon Horman <horms at verge.net.au>
---
 ofproto/connmgr.c          |  8 ++++++-
 ofproto/connmgr.h          |  4 +++-
 ofproto/ofproto-provider.h |  2 ++
 ofproto/ofproto.c          | 12 ++++++-----
 tests/ofproto.at           | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 7d313c7..260b065 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -2068,7 +2068,8 @@ void
 ofmonitor_report(struct connmgr *mgr, struct rule *rule,
                  enum nx_flow_update_event event,
                  enum ofp_flow_removed_reason reason,
-                 const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid)
+                 const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid,
+                 const struct rule_actions *old_actions)
     OVS_REQUIRES(ofproto_mutex)
 {
     enum nx_flow_monitor_flags update;
@@ -2114,6 +2115,11 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
         HMAP_FOR_EACH (m, ofconn_node, &ofconn->monitors) {
             if (m->flags & update
                 && (m->table_id == 0xff || m->table_id == rule->table_id)
+                && (ofproto_rule_has_out_port(rule, m->out_port)
+                    || (old_actions
+                        && ofpacts_output_to_port(old_actions->ofpacts,
+                                                  old_actions->ofpacts_len,
+                                                  m->out_port)))
                 && cls_rule_is_loose_match(&rule->cr, &m->match)) {
                 flags |= m->flags;
             }
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 75a4c06..75a1ffe 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -24,6 +24,7 @@
 #include "ofp-errors.h"
 #include "ofp-util.h"
 #include "ofproto.h"
+#include "ofproto-provider.h"
 #include "openflow/nicira-ext.h"
 #include "openvswitch/types.h"
 
@@ -218,7 +219,8 @@ void ofmonitor_destroy(struct ofmonitor *)
 
 void ofmonitor_report(struct connmgr *, struct rule *,
                       enum nx_flow_update_event, enum ofp_flow_removed_reason,
-                      const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid)
+                      const struct ofconn *abbrev_ofconn, ovs_be32 abbrev_xid,
+                      const struct rule_actions *old_actions)
     OVS_REQUIRES(ofproto_mutex);
 void ofmonitor_flush(struct connmgr *) OVS_REQUIRES(ofproto_mutex);
 
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index c8d70e6..ab5cda8 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -421,6 +421,8 @@ BUILD_ASSERT_DECL(offsetof(struct rule_actions, ofpacts) % OFPACT_ALIGNTO == 0);
 
 const struct rule_actions *rule_actions_create(const struct ofpact *, size_t);
 void rule_actions_destroy(const struct rule_actions *);
+bool ofproto_rule_has_out_port(const struct rule *, ofp_port_t port)
+    OVS_REQUIRES(ofproto_mutex);
 
 /* A set of rules to which an OpenFlow operation applies. */
 struct rule_collection {
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 83a9c35..7b2de18 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2562,7 +2562,7 @@ rule_actions_destroy(const struct rule_actions *actions)
 
 /* Returns true if 'rule' has an OpenFlow OFPAT_OUTPUT or OFPAT_ENQUEUE action
  * that outputs to 'port' (output to OFPP_FLOOD and OFPP_ALL doesn't count). */
-static bool
+bool
 ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port)
     OVS_REQUIRES(ofproto_mutex)
 {
@@ -4077,7 +4077,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     }
 
     ofmonitor_report(ofproto->connmgr, rule, NXFME_ADDED, 0,
-                     req ? req->ofconn : NULL, req ? req->xid : 0);
+                     req ? req->ofconn : NULL, req ? req->xid : 0, NULL);
 
     return req ? send_buffered_packet(req->ofconn, fm->buffer_id, rule) : 0;
 }
@@ -4167,7 +4167,6 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         if (change_actions) {
             ovsrcu_set(&rule->actions, rule_actions_create(fm->ofpacts,
                                                            fm->ofpacts_len));
-            rule_actions_destroy(actions);
         }
 
         if (change_actions || reset_counters) {
@@ -4176,10 +4175,12 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 
         if (event != NXFME_MODIFIED || change_actions || change_cookie) {
             ofmonitor_report(ofproto->connmgr, rule, event, 0,
-                             req ? req->ofconn : NULL, req ? req->xid : 0);
+                             req ? req->ofconn : NULL, req ? req->xid : 0,
+                             change_actions ? actions : NULL);
         }
 
         if (change_actions) {
+            rule_actions_destroy(actions);
             learned_cookies_inc(ofproto, rule_get_actions(rule));
             learned_cookies_dec(ofproto, actions, &dead_cookies);
         }
@@ -4289,7 +4290,8 @@ delete_flows__(const struct rule_collection *rules,
             ofproto_rule_send_removed(rule, reason);
 
             ofmonitor_report(ofproto->connmgr, rule, NXFME_DELETED, reason,
-                             req ? req->ofconn : NULL, req ? req->xid : 0);
+                             req ? req->ofconn : NULL, req ? req->xid : 0,
+                             NULL);
             oftable_remove_rule(rule);
             ofproto->ofproto_class->rule_delete(rule);
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index bd9c3af..8ed1571 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2250,6 +2250,58 @@ ovs-appctl -t ovs-ofctl exit
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto - flow monitoring with out_port])
+AT_KEYWORDS([monitor])
+OVS_VSWITCHD_START
+
+ovs-ofctl add-flow br0 in_port=0,dl_vlan=121,actions=output:1
+ovs-ofctl add-flow br0 in_port=0,dl_vlan=122,actions=output:1
+ovs-ofctl add-flow br0 in_port=0,dl_vlan=123,actions=output:2
+
+# Start a monitor watching the flow table and check the initial reply.
+ovs-ofctl monitor br0 watch:out_port=2 --detach --no-chdir --pidfile >monitor.log 2>&1
+AT_CAPTURE_FILE([monitor.log])
+ovs-appctl -t ovs-ofctl ofctl/barrier
+AT_CHECK([sed 's/ (xid=0x[[1-9a-fA-F]][[0-9a-fA-F]]*)//' monitor.log], [0],
+  [NXST_FLOW_MONITOR reply:
+ event=ADDED table=0 cookie=0 in_port=0,dl_vlan=123 actions=output:2
+OFPT_BARRIER_REPLY:
+])
+
+ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
+
+# Add, modify flows and check the updates.
+ovs-ofctl mod-flows br0 dl_vlan=121,actions=drop
+ovs-ofctl mod-flows br0 dl_vlan=122,actions=output:1,output:2
+ovs-appctl -t ovs-ofctl ofctl/barrier
+
+ovs-ofctl mod-flows br0 dl_vlan=123,actions=output:1,output:2
+ovs-appctl -t ovs-ofctl ofctl/barrier
+
+ovs-ofctl mod-flows br0 dl_vlan=122,actions=output:1
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-ofctl mod-flows br0 dl_vlan=123,actions=output:2
+ovs-appctl -t ovs-ofctl ofctl/barrier
+
+AT_CHECK([sed 's/ (xid=0x[[1-9a-fA-F]][[0-9a-fA-F]]*)//' monitor.log], [0],
+[NXST_FLOW_MONITOR reply (xid=0x0):
+ event=MODIFIED table=0 cookie=0 in_port=0,dl_vlan=122 actions=output:1,output:2
+OFPT_BARRIER_REPLY:
+NXST_FLOW_MONITOR reply (xid=0x0):
+ event=MODIFIED table=0 cookie=0 in_port=0,dl_vlan=123 actions=output:1,output:2
+OFPT_BARRIER_REPLY:
+NXST_FLOW_MONITOR reply (xid=0x0):
+ event=MODIFIED table=0 cookie=0 in_port=0,dl_vlan=122 actions=output:1
+OFPT_BARRIER_REPLY:
+NXST_FLOW_MONITOR reply (xid=0x0):
+ event=MODIFIED table=0 cookie=0 in_port=0,dl_vlan=123 actions=output:2
+OFPT_BARRIER_REPLY:
+])
+
+ovs-appctl -t ovs-ofctl exit
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto - flow monitoring pause and resume])
 AT_KEYWORDS([monitor])
 
-- 
2.0.0.rc2




More information about the dev mailing list