[ovs-dev] [PATCH] Extend sFlow agent to report tunnel and MPLS structures

Ben Pfaff blp at nicira.com
Tue Jul 21 21:19:32 UTC 2015


On Fri, Jul 17, 2015 at 09:37:02PM -0700, Neil McKee wrote:
> Packets are still sampled at ingress only, so the egress
> tunnel and/or MPLS structures are only included when there is just 1 output
> port.  The actions are either provided by the datapath in the sample upcall
> or looked up in the userspace cache.  The former is preferred because it is
> more reliable and does not present any new demands or constraints on the
> userspace cache, however the code falls back on the userspace lookup so that
> this solution can work with existing kernel datapath modules. If the lookup
> fails it is not critical: the compiled user-action-cookie is still available
> and provides the essential output port and output VLAN forwarding information
> just as before.
> 
> The openvswitch actions can express almost any tunneling/mangling so the only
> totally faithful representation would be to somehow encode the whole list of
> flow actions in the sFlow output.  However the standard sFlow tunnel structures
> can express most common real-world scenarios, so in parsing the actions we
> look for those and skip the encoding if we see anything unusual. For example,
> a single set(tunnel()) or tnl_push() is interpreted,  but if a second such
> action is encountered then the egress tunnel reporting is suppressed.
> 
> The sFlow standard allows "best effort" encoding so that if a field is not
> knowable or too onerous to look up then it can be left out. This is often
> the case for the layer-4 source port or even the src ip address of a tunnel.
> The assumption is that monitoring is enabled everywhere so a missing field
> can typically be seen at ingress to the next switch in the path.
> 
> This patch also adds unit tests to check the sFlow encoding of set(tunnel()),
> tnl_push() and push_mpls() actions.
> 
> The netlink attribute to request that actions be included in the upcall
> from the datapath is inserted for sFlow sampling only.  To make that option
> be explicit would require further changes to the printing and parsing of
> actions in lib/odp-util.c, and to scripts in the test suite.
> 
> Further enhancements to report on 802.1AD QinQ, 64-bit tunnel IDs, and NAT
> transformations can follow in future patches that make only incremental
> changes.
> 
> Signed-off-by: Neil McKee <neil.mckee at inmon.com>

Thanks Neil!

A lot of the new code here used tabs instead of spaces for indentation.
I fixed it up where I noticed it.

The parsing and formatting code in odp-util.c is supposed to use a very
"raw" representation of the Netlink form.  I wasn't really comfortable
with having "actions" be implicit, so I folded in the following to make
it explicit:

--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -257,7 +257,6 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
     struct nlattr *a[ARRAY_SIZE(ovs_userspace_policy)];
     const struct nlattr *userdata_attr;
     const struct nlattr *tunnel_out_port_attr;
-    const struct nlattr *actions_attr;
 
     if (!nl_parse_nested(attr, ovs_userspace_policy, a, ARRAY_SIZE(a))) {
         ds_put_cstr(ds, "userspace(error)");
@@ -325,22 +324,16 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
         }
     }
 
+    if (a[OVS_USERSPACE_ATTR_ACTIONS]) {
+        ds_put_cstr(ds, ",actions");
+    }
+
     tunnel_out_port_attr = a[OVS_USERSPACE_ATTR_EGRESS_TUN_PORT];
     if (tunnel_out_port_attr) {
         ds_put_format(ds, ",tunnel_out_port=%"PRIu32,
                       nl_attr_get_u32(tunnel_out_port_attr));
     }
 
-    actions_attr = a[OVS_USERSPACE_ATTR_ACTIONS];
-    if (actions_attr) {
-	/* XXX This attribute is only ever used by sFlow, so treat
-	 * it as being implicit and do not print it here - at least
-	 * not until we parse it back again in parse_odp_userspace_action().
-	 * This affects unit test 367 in tests/odp.at.
-	 */
-        /* ds_put_cstr(ds, ",actions"); */
-    }
-
     ds_put_char(ds, ')');
 }
 
@@ -709,7 +702,6 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
             cookie.sflow.output = output;
             user_data = &cookie;
             user_data_size = sizeof cookie.sflow;
-            include_actions = true;
         } else if (ovs_scan(&s[n], ",slow_path(%n",
                             &n1)) {
             int res;
@@ -769,6 +761,14 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
 
     {
         int n1 = -1;
+        if (ovs_scan(&s[n], ",actions%n", &n1)) {
+            n += n1;
+            include_actions = true;
+        }
+    }
+
+    {
+        int n1 = -1;
         if (ovs_scan(&s[n], ",tunnel_out_port=%"SCNi32")%n",
                      &tunnel_out_port, &n1)) {
             odp_put_userspace_action(pid, user_data, user_data_size,
diff --git a/tests/odp.at b/tests/odp.at
index a50a9cd..61edde3 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -234,13 +234,13 @@ AT_DATA([actions.txt], [dnl
 1,2,3
 userspace(pid=555666777)
 userspace(pid=555666777,tunnel_out_port=10)
-userspace(pid=6633,sFlow(vid=9,pcp=7,output=10))
-userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),tunnel_out_port=10)
+userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions)
+userspace(pid=6633,sFlow(vid=9,pcp=7,output=10),actions,tunnel_out_port=10)
 userspace(pid=9765,slow_path(0))
 userspace(pid=9765,slow_path(0),tunnel_out_port=10)
 userspace(pid=9765,slow_path(cfm))
 userspace(pid=9765,slow_path(cfm),tunnel_out_port=10)
-userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f))
+userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f),actions)
 userspace(pid=1234567,userdata(0102030405060708090a0b0c0d0e0f),tunnel_out_port=10)
 userspace(pid=6633,flow_sample(probability=123,collector_set_id=1234,obs_domain_id=2345,obs_point_id=3456))
 userspace(pid=6633,flow_sample(probability=123,collector_set_id=1234,obs_domain_id=2345,obs_point_id=3456),tunnel_out_port=10)


This caused some "sparse" warnings.  I fixed them.

I made coding style changes in sflow_read_tnl_push_action().

I removed the code that took a mutex in process_upcall().  The comment
on struct udpif_key, the structure being accessed, says that no mutex is
needed:

    /* These elements are read only once created, and therefore aren't
     * protected by a mutex. */

I added a NEWS item.

And I pushed the whole thing to master.  Thank you for your contributions!



More information about the dev mailing list