On 6/18/24 09:33, Adrián Moreno wrote: > On Mon, Jun 17, 2024 at 12:44:45PM GMT, Ilya Maximets wrote: >> On 6/3/24 20:56, Adrian Moreno wrote: >>> Add support for a new action: emit_sample. >>> >>> This action accepts a u32 group id and a variable-length cookie and uses >>> the psample multicast group to make the packet available for >>> observability. >>> >>> The maximum length of the user-defined cookie is set to 16, same as >>> tc_cookie, to discourage using cookies that will not be offloadable. >>> >>> Signed-off-by: Adrian Moreno <[email protected]> >>> --- >>> Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ >>> include/uapi/linux/openvswitch.h | 25 ++++++++++++ >>> net/openvswitch/actions.c | 50 +++++++++++++++++++++++ >>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++- >>> 4 files changed, 124 insertions(+), 1 deletion(-) >> >> Some nits below, beside ones already mentioned. >> > > Thanks, Ilya. > >>> >>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml >>> b/Documentation/netlink/specs/ovs_flow.yaml >>> index 4fdfc6b5cae9..a7ab5593a24f 100644 >>> --- a/Documentation/netlink/specs/ovs_flow.yaml >>> +++ b/Documentation/netlink/specs/ovs_flow.yaml >>> @@ -727,6 +727,12 @@ attribute-sets: >>> name: dec-ttl >>> type: nest >>> nested-attributes: dec-ttl-attrs >>> + - >>> + name: emit-sample >>> + type: nest >>> + nested-attributes: emit-sample-attrs >>> + doc: | >>> + Sends a packet sample to psample for external observation. >>> - >>> name: tunnel-key-attrs >>> enum-name: ovs-tunnel-key-attr >>> @@ -938,6 +944,17 @@ attribute-sets: >>> - >>> name: gbp >>> type: u32 >>> + - >>> + name: emit-sample-attrs >>> + enum-name: ovs-emit-sample-attr >>> + name-prefix: ovs-emit-sample-attr- >>> + attributes: >>> + - >>> + name: group >>> + type: u32 >>> + - >>> + name: cookie >>> + type: binary >>> >>> operations: >>> name-prefix: ovs-flow-cmd- >>> diff --git a/include/uapi/linux/openvswitch.h >>> b/include/uapi/linux/openvswitch.h >>> index efc82c318fa2..a0e9dde0584a 100644 >>> --- a/include/uapi/linux/openvswitch.h >>> +++ b/include/uapi/linux/openvswitch.h >>> @@ -914,6 +914,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. >> >> s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/ >> >>> + * >>> + * 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 packet being >>> emitted. >>> + */ >>> +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. */ >>> + __OVS_EMIT_SAMPLE_ATTR_MAX >>> +}; >>> + >>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >>> + >>> + >>> /** >>> * enum ovs_action_attr - Action types. >>> * >>> @@ -1004,6 +1028,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 error code. */ >>> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ >>> >>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >>> * from userspace. */ >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>> index 964225580824..3b4dba0ded59 100644 >>> --- a/net/openvswitch/actions.c >>> +++ b/net/openvswitch/actions.c >>> @@ -24,6 +24,11 @@ >>> #include <net/checksum.h> >>> #include <net/dsfield.h> >>> #include <net/mpls.h> >>> + >>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>> +#include <net/psample.h> >>> +#endif >>> + >>> #include <net/sctp/checksum.h> >>> >>> #include "datapath.h" >>> @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, >>> struct sw_flow_key *key) >>> return 0; >>> } >>> >>> +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, >>> + const struct sw_flow_key *key, >>> + const struct nlattr *attr) >>> +{ >>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>> + struct psample_group psample_group = {}; >>> + struct psample_metadata md = {}; >>> + struct vport *input_vport; >>> + const struct nlattr *a; >>> + int rem; >>> + >>> + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; >>> + a = nla_next(a, &rem)) { >> >> Since the action is strictly validated, can use use nla_for_each_attr() >> or nla_for_each_nested() ? >> > > Probably, yes. > >>> + switch (nla_type(a)) { >>> + case OVS_EMIT_SAMPLE_ATTR_GROUP: >>> + psample_group.group_num = nla_get_u32(a); >>> + break; >>> + >>> + case OVS_EMIT_SAMPLE_ATTR_COOKIE: >>> + md.user_cookie = nla_data(a); >>> + md.user_cookie_len = nla_len(a); >>> + break; >>> + } >>> + } >>> + >>> + psample_group.net = ovs_dp_get_net(dp); >>> + >>> + input_vport = ovs_vport_rcu(dp, key->phy.in_port); >>> + if (!input_vport) >>> + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); >> >> We may need to check that we actually found the local port. >> > > Sure. What can cause the local port not to exist?
I would assume that since we're only protected by RCU here, there can be a race with datapath destruction that will remove the local port. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
