Wow. That's great. Thanks. I will look at what else we should add to the
sFlow export. (e.g. QinQ, NAT, ...). Should be straighforward now that we
have the actions for each sample.
Neil
On Tuesday, July 21, 2015, Ben Pfaff <b...@nicira.com> wrote:
> 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 <javascript:;>>
>
> 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!
>
--
------
Neil McKee
InMon Corp.
http://www.inmon.com
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
