On 9 Jul 2024, at 15:30, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote: >> On 4 Jul 2024, at 9:52, 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_PSAMPLE. The reason is that the >>> sampling rate form the parent "sample" is made available to the nested >> >> form -> from >> >>> "psample" by the kernel. >> >> Two small comments below, the rest looks good. >> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >>> --- >>> include/linux/openvswitch.h | 28 +++++++++++ >>> lib/dpif-netdev.c | 1 + >>> lib/dpif.c | 1 + >>> lib/odp-execute.c | 25 +++++++++- >>> lib/odp-util.c | 93 ++++++++++++++++++++++++++++++++++++ >>> 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, 176 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h >>> index d9fb991ef..0023b65fb 100644 >>> --- a/include/linux/openvswitch.h >>> +++ b/include/linux/openvswitch.h >>> @@ -992,6 +992,31 @@ struct check_pkt_len_arg { >>> }; >>> #endif >>> >>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16 >>> +/** >>> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE >>> + * action. >>> + * >>> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the >>> + * sample. >>> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that >>> + * contains user-defined metadata. The maximum length is >>> + * OVS_PSAMPLE_COOKIE_MAX_SIZE 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 action to limit the size of the sample. >>> + */ >>> +enum ovs_psample_attr { >>> + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>> + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. >>> */ >>> + >>> + /* private: */ >> >> None of the other definitions have this private marking, so I see no need to >> start adding it here. > > OK. The uAPI file has it (requested by netdev maintainers) but I guess > it's kernel-specific and this file is not a blind copy of the uAPI > anyways so I think we can remove it. I'll do on the next version. > >> >>> + __OVS_PSAMPLE_ATTR_MAX >>> +}; >>> + >>> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1) >>> + >>> /** >>> * enum ovs_action_attr - Action types. >>> * >>> @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg { >>> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS >>> * argument. >>> * @OVS_ACTION_ATTR_DROP: Explicit drop action. >>> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external >>> observers >>> + * via psample. >>> */ >>> >>> enum ovs_action_attr { >>> @@ -1087,6 +1114,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_PSAMPLE, /* Nested OVS_PSAMPLE_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..f0594e5f5 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_PSAMPLE: >>> case __OVS_ACTION_ATTR_MAX: >>> OVS_NOT_REACHED(); >>> } >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index 23eb18495..71728badc 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_PSAMPLE: >>> 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..15577d539 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_PSAMPLE: >>> 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 "psample" actions rely on the datapath executing the >>> + * parent "sample", storing the probability and making it available >>> + * when the nested "psample" is run. */ >>> + const struct nlattr *attr; >>> + unsigned int left; >>> + >>> + NL_NESTED_FOR_EACH (attr, left, a) { >> >> Still not hitting this code with a test case. Is this supposed to be the >> psample - local case? > > No. We had been discussing and I hadn't come up with a valid yet > reliable test when I reposted the series. > > Do you think it's OK to build a test with an inherent probability of it > failing? I assumed you would add it ;) Anyway, if 0.0015% is not sampled, just sending 100 packets would make it very unlikely that all would be dropped. Maybe just add a note to the test at the location where it would potentially fail. Haven’t thought this through, but could we do something to find out the drop reason, and retry? >>> + 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_PSAMPLE) { >>> + 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_PSAMPLE: >>> 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..be9c8b449 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_PSAMPLE: return ATTR_LEN_VARIABLE; >>> >>> case OVS_ACTION_ATTR_UNSPEC: >>> case __OVS_ACTION_ATTR_MAX: >>> @@ -1150,6 +1151,30 @@ format_dec_ttl_action(struct ds *ds, const struct >>> nlattr *attr, >>> ds_put_format(ds, "))"); >>> } >>> >>> +static void >>> +format_odp_psample_action(struct ds *ds, const struct nlattr *attr) >>> +{ >>> + const struct nlattr *a; >>> + unsigned int left; >>> + >>> + ds_put_cstr(ds, "psample("); >>> + >>> + NL_ATTR_FOR_EACH (a, left, >>> + nl_attr_get(attr), nl_attr_get_size(attr)) { >>> + switch (a->nla_type) { >>> + case OVS_PSAMPLE_ATTR_GROUP: >>> + ds_put_format(ds, "group=%"PRIu32",", nl_attr_get_u32(a)); >>> + break; >>> + case OVS_PSAMPLE_ATTR_COOKIE: >>> + ds_put_cstr(ds, "cookie="); >>> + ds_put_hex(ds, nl_attr_get(a), nl_attr_get_size(a)); >>> + break; >>> + } >>> + } >>> + 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 +1334,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_PSAMPLE: >>> + format_odp_psample_action(ds, a); >>> + break; >>> case OVS_ACTION_ATTR_UNSPEC: >>> case __OVS_ACTION_ATTR_MAX: >>> default: >>> @@ -2358,6 +2386,50 @@ out: >>> return ret; >>> } >>> >>> +static int >>> +parse_odp_psample_action(const char *s, struct ofpbuf *actions) >>> +{ >>> + char buf[2 * OVS_PSAMPLE_COOKIE_MAX_SIZE + 1]; >>> + uint8_t cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE]; >>> + bool has_group = false; >>> + size_t cookie_len = 0; >>> + uint32_t group; >>> + int n = 0; >>> + >>> + if (!ovs_scan_len(s, &n, "psample(")) { >>> + 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=0x%32[0-9a-fA-F]", buf) && n > 7) { >>> + struct ofpbuf b; >>> + >>> + ofpbuf_use_stub(&b, cookie, OVS_PSAMPLE_COOKIE_MAX_SIZE); >>> + ofpbuf_put_hex(&b, buf, &cookie_len); >>> + ofpbuf_uninit(&b); >>> + continue; >>> + } >>> + return -EINVAL; >>> + } >>> + n++; >>> + >>> + if (!has_group) { >>> + return -EINVAL; >>> + } >>> + >>> + odp_put_psample_action(actions, group, cookie_len ? cookie : NULL, >>> + cookie_len); >> >> Alignment >> > > Ack. > >>> + return n; >>> +} >>> + >>> static int >>> parse_action_list(struct parse_odp_context *context, const char *s, >>> struct ofpbuf *actions) >>> @@ -2719,6 +2791,10 @@ parse_odp_action__(struct parse_odp_context >>> *context, const char *s, >>> } >>> } >>> >>> + if (!strncmp(s, "psample(", 8)) { >>> + return parse_odp_psample_action(s, actions); >>> + } >>> + >>> { >>> struct ovs_action_push_tnl data; >>> int n; >>> @@ -7828,6 +7904,23 @@ 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_psample_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_PSAMPLE); >>> + >>> + nl_msg_put_u32(odp_actions, OVS_PSAMPLE_ATTR_GROUP, group_id); >>> + if (cookie && cookie_len) { >>> + ovs_assert(cookie_len <= OVS_PSAMPLE_COOKIE_MAX_SIZE); >>> + nl_msg_put_unspec(odp_actions, OVS_PSAMPLE_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..e454dbfcd 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_psample_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..15b656233 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_PSAMPLE: >>> case __OVS_ACTION_ATTR_MAX: >>> default: >>> break; >>> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c >>> index 80405b68a..fb12cf419 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_PSAMPLE: >>> case __OVS_ACTION_ATTR_MAX: >>> default: >>> break; >>> diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py >>> index a8f8c067a..572dbebe9 100644 >>> --- a/python/ovs/flow/odp.py >>> +++ b/python/ovs/flow/odp.py >>> @@ -343,6 +343,14 @@ class ODPFlow(Flow): >>> } >>> ) >>> ), >>> + "psample": 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..402b2386d 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) >>> +psample(group=12,cookie=0xf1020304050607080910111213141516) >>> +psample(group=12) >>> +sample(sample=50.0%,actions(psample(group=12,cookie=0xf1020304))) >>> +sample(sample=50.0%,actions(userspace(pid=42,userdata(0102030400000000)),psample(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)) >>> +psample(group_id=12,cookie=0x0102030405060708090a0b0c0d0e0f0f0f) >>> +psample(cookie=0x010203) >>> +psample(group=12,cookie=0x010203,group=12) >>> +psample(group=abc) >>> +psample(group=12,cookie=wrong) >>> +psample() >>> ]) >>> 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.2 >> < >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev