On Tue, Jul 09, 2024 at 04:15:12PM GMT, Eelco Chaudron wrote:
>
>
> 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?
Not sure how tbh.
In my mind the test should:
- Create a flow with a sample action that xlates into:
sample(sample=99.99%, actions(psample(..),userspace(..))
- Force actions to be run in slow path.
- Check psample is received with the right probability which means OVS
has executed both the sample() and the psample() part in the kernel.
We can send more than one packet to increase our chances.
We can look at ipfix stats to know if we have been very unlucky and we
have not fallen into the sample(), and we can retry but at some point we
must consider the test to have failed. So I don't know if the retry
helps much (compared with just doubling the number of packets sent).
I'm going to add the test and try it a bit but I believe that (assuming
random() is actually random), if we configure a probability of
(U16_MAX-1)/U16_MAX and we repeat the experiment 100 times, that gives us
a really really low probability of failure.
WDYT?
>
> >>> + 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
