On 5 Jun 2024, at 22:23, Adrian Moreno wrote: > Add support for parsing and formatting the new action. > > Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it > contains a nested OVS_ACTION_ATTR_EMIT_SAMPLE. The reason is that the > sampling rate form the parent "sample" is made available to the nested > "emit_sample" by the kernel.
Hi Adrian, Thanks for this series! This email kicks off my review of the series, see comments below and in the other patches. Cheers, Eelco > Signed-off-by: Adrian Moreno <[email protected]> > --- > include/linux/openvswitch.h | 25 +++++++++ > lib/dpif-netdev.c | 1 + > lib/dpif.c | 1 + > lib/odp-execute.c | 25 ++++++++- > lib/odp-util.c | 103 +++++++++++++++++++++++++++++++++++ > lib/odp-util.h | 3 + > ofproto/ofproto-dpif-ipfix.c | 1 + > ofproto/ofproto-dpif-sflow.c | 1 + > python/ovs/flow/odp.py | 8 +++ > tests/odp.at | 16 ++++++ > 10 files changed, 183 insertions(+), 1 deletion(-) > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > index d9fb991ef..b4e0647bd 100644 > --- a/include/linux/openvswitch.h > +++ b/include/linux/openvswitch.h > @@ -992,6 +992,30 @@ struct check_pkt_len_arg { > }; > #endif > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > +/** > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > + * action. > + * > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > + * sample. > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that > contains > + * user-defined metadata. The maximum length is 16 bytes. > + * > + * Sends the packet to the psample multicast group with the specified group > and > + * cookie. It is possible to combine this action with the > + * %OVS_ACTION_ATTR_TRUNC to limit the size of the packet being emitted. I'll start the discussion again on the naming. The name "emit_sample()" does not seem appropriate. This function's primary role is to copy the packet and send it to a local collector, which varies depending on the datapath. For the kernel datapath, this collector is psample, while for userspace, it will likely be some kind of probe. This action is distinct from the sample() action by design; it is a standalone action that can be combined with others. Furthermore, the action itself does not involve taking a sample; it consistently pushes the packet to the local collector. Therefore, I suggest renaming "emit_sample()" to "emit_local()". This same goes for all the derivative ATTR naming. > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ Fix comment alignment? Maybe also change the order to be alphabetical? > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > + > /** > * enum ovs_action_attr - Action types. > * > @@ -1087,6 +1111,7 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > #ifndef __KERNEL__ > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index c7f9e1490..c1171890c 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch > *packets_, > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > case OVS_ACTION_ATTR_DEC_TTL: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/dpif.c b/lib/dpif.c > index 23eb18495..489d6a095 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct > dp_packet_batch *packets_, > case OVS_ACTION_ATTR_TUNNEL_PUSH: > case OVS_ACTION_ATTR_TUNNEL_POP: > case OVS_ACTION_ATTR_USERSPACE: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > case OVS_ACTION_ATTR_RECIRC: { > struct dpif_execute execute; > struct ofpbuf execute_actions; > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 081e4d432..967abfd0a 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a) > case OVS_ACTION_ATTR_RECIRC: > case OVS_ACTION_ATTR_CT: > case OVS_ACTION_ATTR_METER: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > return true; > > case OVS_ACTION_ATTR_SET: > case OVS_ACTION_ATTR_SET_MASKED: > case OVS_ACTION_ATTR_PUSH_VLAN: > case OVS_ACTION_ATTR_POP_VLAN: > - case OVS_ACTION_ATTR_SAMPLE: > case OVS_ACTION_ATTR_HASH: > case OVS_ACTION_ATTR_PUSH_MPLS: > case OVS_ACTION_ATTR_POP_MPLS: > @@ -841,6 +841,28 @@ requires_datapath_assistance(const struct nlattr *a) > case OVS_ACTION_ATTR_DROP: > return false; > > + case OVS_ACTION_ATTR_SAMPLE: { > + /* Nested "emit_sample" actions rely on the datapath executing the > + * parent "sample", storing the probability and making it available > + * when the nested "emit_sample" is run. > + */ Comments in this file have the closing mark on the last line, so please do this here also. > + const struct nlattr *attr; > + unsigned int left; Please add a new line after variable declarations. > + NL_NESTED_FOR_EACH (attr, left, a) { Also can we get a test case for this code path? The patch 9/9 is not hitting this. > + if (nl_attr_type(attr) == OVS_SAMPLE_ATTR_ACTIONS) { > + const struct nlattr *act; > + unsigned int act_left; > + > + NL_NESTED_FOR_EACH (act, act_left, attr) { > + if (nl_attr_type(act) == OVS_ACTION_ATTR_EMIT_SAMPLE) { > + return true; > + } > + } > + } > + } > + return false; > + } > + > case OVS_ACTION_ATTR_UNSPEC: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > @@ -1229,6 +1251,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch > *batch, bool steal, > case OVS_ACTION_ATTR_CT: > case OVS_ACTION_ATTR_UNSPEC: > case OVS_ACTION_ATTR_DEC_TTL: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > case __OVS_ACTION_ATTR_MAX: > /* The following actions are handled by the scalar implementation. */ > case OVS_ACTION_ATTR_POP_VLAN: > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 724e6f2bc..02452083f 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -145,6 +145,7 @@ odp_action_len(uint16_t type) > case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct ovs_action_add_mpls); > case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; > case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); > + case OVS_ACTION_ATTR_EMIT_SAMPLE: return ATTR_LEN_VARIABLE; > > case OVS_ACTION_ATTR_UNSPEC: > case __OVS_ACTION_ATTR_MAX: > @@ -1150,6 +1151,36 @@ format_dec_ttl_action(struct ds *ds, const struct > nlattr *attr, > ds_put_format(ds, "))"); > } > > +static void > +format_odp_emit_sample_action(struct ds *ds, const struct nlattr *attr) > +{ > + const struct nlattr *a; > + unsigned int left; > + > + ds_put_cstr(ds, "emit_sample("); > + > + NL_ATTR_FOR_EACH (a, left, > + nl_attr_get(attr), nl_attr_get_size(attr)) { > + switch (a->nla_type) { > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > + ds_put_format(ds, "group=%"PRIu32",", nl_attr_get_u32(a)); > + break; > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: { > + const uint8_t *cookie = nl_attr_get(a); > + int i; > + > + ds_put_cstr(ds, "cookie="); > + for (i = 0; i < nl_attr_get_size(a); i++) { > + ds_put_format(ds, "%02x", cookie[i]); > + } There is a ds_put_hex() function, it adds '0x' which I guess is not desired here (or maybe we should as it's more clear its a hex string). > + break; > + } > + } nit: Maybe swap the two case statements to avoid the odd looking double }. > + } > + ds_chomp(ds, ','); > + ds_put_char(ds, ')'); > +} > + > static void > format_odp_action(struct ds *ds, const struct nlattr *a, > const struct hmap *portno_names) > @@ -1309,6 +1340,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a, > case OVS_ACTION_ATTR_DROP: > ds_put_cstr(ds, "drop"); > break; > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > + format_odp_emit_sample_action(ds, a); > + break; > case OVS_ACTION_ATTR_UNSPEC: > case __OVS_ACTION_ATTR_MAX: > default: > @@ -2358,6 +2392,50 @@ out: > return ret; > } > > +static int > +parse_odp_emit_sample_action(const char *s, struct ofpbuf *actions) > +{ > + uint8_t cookie[OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE]; > + char buf[2 * OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE + 1]; Reverse Christmas tree order. > + bool has_group = false; > + size_t cookie_len = 0; > + uint32_t group; > + int n = 0; > + > + if (!ovs_scan_len(s, &n, "emit_sample(")) { > + return -EINVAL; > + } > + > + while (s[n] != ')') { > + n += strspn(s + n, delimiters); > + > + if (!has_group && ovs_scan_len(s, &n, "group=%"SCNi32, &group)) { > + has_group = true; > + continue; > + } > + > + if (!cookie_len && > + ovs_scan_len(s, &n, "cookie=%32[0-9a-fA-F]", buf) && n > 7) { > + struct ofpbuf b; > + > + ofpbuf_use_stub(&b, cookie, OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE); > + ofpbuf_put_hex(&b, buf, &cookie_len); > + ofpbuf_uninit(&b); > + continue; > + } > + return -EINVAL; > + } > + n++; > + > + if (!has_group) { > + return -EINVAL; > + } > + > + odp_put_emit_sample_action(actions, group, cookie_len ? cookie : NULL, > + cookie_len); > + return n; > +} > + > static int > parse_action_list(struct parse_odp_context *context, const char *s, > struct ofpbuf *actions) > @@ -2719,6 +2797,12 @@ parse_odp_action__(struct parse_odp_context *context, > const char *s, > } > } > > + { You don' t need the extra {} set here, it's only needed if you have local variables defined. See pop_vlan for example. > + if (!strncmp(s, "emit_sample(", 12)) { > + return parse_odp_emit_sample_action(s, actions); > + } > + } > + > { > struct ovs_action_push_tnl data; > int n; > @@ -7828,6 +7912,25 @@ odp_put_tnl_push_action(struct ofpbuf *odp_actions, > nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_TUNNEL_PUSH, data, size); > } > > +void > +odp_put_emit_sample_action(struct ofpbuf *odp_actions, > + uint32_t group_id, > + uint8_t *cookie, > + size_t cookie_len) > +{ > + size_t offset = nl_msg_start_nested_with_flag(odp_actions, > + > OVS_ACTION_ATTR_EMIT_SAMPLE); > + > + nl_msg_put_u32(odp_actions, OVS_EMIT_SAMPLE_ATTR_GROUP, group_id); > + if (cookie && cookie_len) { > + ovs_assert(cookie_len <= OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE); > + nl_msg_put_unspec(odp_actions, OVS_EMIT_SAMPLE_ATTR_COOKIE, cookie, > + cookie_len); > + } > + > + nl_msg_end_nested(odp_actions, offset); > +} > + > > /* The commit_odp_actions() function and its helpers. */ > > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 8c7baa680..2159f0897 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -376,6 +376,9 @@ void odp_put_pop_eth_action(struct ofpbuf *odp_actions); > void odp_put_push_eth_action(struct ofpbuf *odp_actions, > const struct eth_addr *eth_src, > const struct eth_addr *eth_dst); > +void odp_put_emit_sample_action(struct ofpbuf *odp_actions, > + uint32_t group_id, uint8_t *cookie, > + size_t cookie_len); > > static inline void odp_decode_gbp_raw(uint32_t gbp_raw, > ovs_be16 *id, > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index cd65dae7e..15e92ba74 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -3136,6 +3136,7 @@ dpif_ipfix_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > case OVS_ACTION_ATTR_DEC_TTL: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > case __OVS_ACTION_ATTR_MAX: > default: > break; > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index 4a68e9b94..9db499fd4 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -1237,6 +1237,7 @@ dpif_sflow_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_DROP: > case OVS_ACTION_ATTR_ADD_MPLS: > case OVS_ACTION_ATTR_DEC_TTL: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > case __OVS_ACTION_ATTR_MAX: > default: > break; > diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py > index 7d9b165d4..896ae34bd 100644 > --- a/python/ovs/flow/odp.py > +++ b/python/ovs/flow/odp.py > @@ -343,6 +343,14 @@ class ODPFlow(Flow): > } > ) > ), > + "emit_sample": nested_kv_decoder( > + KVDecoders( > + { > + "group": decode_int, > + "cookie": decode_default, > + } > + ) > + ) > } > > _decoders["sample"] = nested_kv_decoder( > diff --git a/tests/odp.at b/tests/odp.at > index ba20604e4..e5c2ee07c 100644 > --- a/tests/odp.at > +++ b/tests/odp.at > @@ -393,6 +393,10 @@ check_pkt_len(size=200,gt(ct(nat)),le(drop)) > > check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) > lb_output(1) > add_mpls(label=200,tc=7,ttl=64,bos=1,eth_type=0x8847) > +emit_sample(group=12,cookie=01020304050607080910111213141516) > +emit_sample(group=12) > +sample(sample=50.0%,actions(emit_sample(group=12,cookie=01020304))) > +sample(sample=50.0%,actions(userspace(pid=42,userdata(0102030400000000)),emit_sample(group=12))) > ]) > AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], > [`cat actions.txt` > @@ -406,11 +410,23 @@ AT_DATA([actions.txt], [dnl > encap_nsh@:{@ > > tnl_push(tnl_port(6),header(size=94,type=112,eth(dst=f8:bc:12:44:34:b6,src=f8:bc:12:46:58:e0,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=43,tclass=0x0,hlimit=64),srv6(segments_left=2,segs(2001:cafe::90,2001:cafe::91))),out_port(1)) > > tnl_push(tnl_port(6),header(size=126,type=112,eth(dst=f8:bc:12:44:34:b6,src=f8:bc:12:46:58:e0,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=43,tclass=0x0,hlimit=64),srv6(segments_left=2,segs(2001:cafe::90,2001:cafe::91,2001:cafe::92,2001:cafe::93))),out_port(1)) > +emit_sample(group_id=12,cookie=0102030405060708090a0b0c0d0e0f0f0f) > +emit_sample(cookie=010203) > +emit_sample(group=12,cookie=010203,group=12) > +emit_sample(group=abc) > +emit_sample(group=12,cookie=wrong) > +emit_sample() > ]) > AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [dnl > odp_actions_from_string: error > odp_actions_from_string: error > odp_actions_from_string: error > +odp_actions_from_string: error > +odp_actions_from_string: error > +odp_actions_from_string: error > +odp_actions_from_string: error > +odp_actions_from_string: error > +odp_actions_from_string: error > ]) > AT_CLEANUP > > -- > 2.45.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
