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.mc...@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! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev