| Message ID | 20240625205204.3199050-6-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: openvswitch: Add sample multicasting. | expand |
On 6/25/24 22:51, 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 <amorenoz@redhat.com> > --- > Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > net/openvswitch/Kconfig | 1 + > net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > 5 files changed, 123 insertions(+), 1 deletion(-) > > 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..8cfa1b3f6b06 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > + > + /* private: */ > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > /** > * enum ovs_action_attr - Action types. > * > @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external > + * observers via psample. > * > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all > * fields within a header are modifiable, e.g. the IPv4 protocol and fragment > @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig > index 29a7081858cd..2535f3f9f462 100644 > --- a/net/openvswitch/Kconfig > +++ b/net/openvswitch/Kconfig > @@ -10,6 +10,7 @@ config OPENVSWITCH > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > (!NF_NAT || NF_NAT) && \ > (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) > + depends on PSAMPLE || !PSAMPLE > select LIBCRC32C > select MPLS > select NET_MPLS_GSO > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +static void execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > + const struct sw_flow_key *key, The 'key' is not used in the function. > + const struct nlattr *attr) > +{ > +#if IS_ENABLED(CONFIG_PSAMPLE) IIUC, the general coding style guideline is to compile out the whole function, instead of only the parts. i.e. something like: #if IS_ENABLED(CONFIG_PSAMPLE) static void execute_emit_sample(...) { <body> } #else #define execute_emit_sample(dp, skb, attr) #endif Otherwise, we'll also need to mark the arguments with __maybe_unused. The rest of the patch looks good to me. Best regards, Ilya Maximets.
On 25 Jun 2024, at 22:51, 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. I’ll add the same comment as I had in the user space part, and that is that I feel from an OVS perspective this action should be called emit_local() instead of emit_sample() to make it Datapath independent. Or quoting the earlier comment: “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.” > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > net/openvswitch/Kconfig | 1 + > net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > 5 files changed, 123 insertions(+), 1 deletion(-) > > 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..8cfa1b3f6b06 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. Although this include file is kernel-related, it will probably be re-used for other datapaths, so should we be more general here? > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ As we start a new set of attributes maybe it would be good starting it off in alphabetical order? > + > + /* private: */ > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > /** > * enum ovs_action_attr - Action types. > * > @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external > + * observers via psample. > * > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all > * fields within a header are modifiable, e.g. the IPv4 protocol and fragment > @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig > index 29a7081858cd..2535f3f9f462 100644 > --- a/net/openvswitch/Kconfig > +++ b/net/openvswitch/Kconfig > @@ -10,6 +10,7 @@ config OPENVSWITCH > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > (!NF_NAT || NF_NAT) && \ > (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) > + depends on PSAMPLE || !PSAMPLE > select LIBCRC32C > select MPLS > select NET_MPLS_GSO > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +static void 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) Same comment as Ilya on key and IS_ENABLED() over function. > + struct psample_group psample_group = {}; > + struct psample_metadata md = {}; > + const struct nlattr *a; > + int rem; > + > + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { > + 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); Do we need to check for any max cookie length? > + break; > + } > + } > + > + psample_group.net = ovs_dp_get_net(dp); > + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > + > + psample_sample_packet(&psample_group, skb, 0, &md); > +#endif > +} > + > /* Execute a list of actions against 'skb'. */ > static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > struct sw_flow_key *key, > @@ -1502,6 +1538,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > ovs_kfree_skb_reason(skb, reason); > return 0; > } > + > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > + execute_emit_sample(dp, skb, key, a); > + OVS_CB(skb)->cutlen = 0; > + if (nla_is_last(a, rem)) { > + consume_skb(skb); > + return 0; > + } > + break; > } > > if (unlikely(err)) { > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index f224d9bcea5e..29c8cdc44433 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) > case OVS_ACTION_ATTR_TRUNC: > case OVS_ACTION_ATTR_USERSPACE: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > break; > > case OVS_ACTION_ATTR_CT: > @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) > /* Whenever new actions are added, the need to update this > * function should be considered. > */ > - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24); > + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25); > > if (!actions) > return; > @@ -3157,6 +3158,29 @@ static int validate_and_copy_check_pkt_len(struct net *net, > return 0; > } > > +static int validate_emit_sample(const struct nlattr *attr) > +{ > + static const struct nla_policy policy[OVS_EMIT_SAMPLE_ATTR_MAX + 1] = { > + [OVS_EMIT_SAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > + [OVS_EMIT_SAMPLE_ATTR_COOKIE] = { > + .type = NLA_BINARY, > + .len = OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE, > + }, > + }; > + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; > + int err; > + > + if (!IS_ENABLED(CONFIG_PSAMPLE)) > + return -EOPNOTSUPP; > + > + err = nla_parse_nested(a, OVS_EMIT_SAMPLE_ATTR_MAX, attr, policy, > + NULL); > + if (err) > + return err; > + > + return a[OVS_EMIT_SAMPLE_ATTR_GROUP] ? 0 : -EINVAL; So we are ok with not having a cookie? Did you inform Cookie Monster ;) Also, update the include help text to reflect this. > +} > + > static int copy_action(const struct nlattr *from, > struct sw_flow_actions **sfa, bool log) > { > @@ -3212,6 +3236,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls), > [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1, > [OVS_ACTION_ATTR_DROP] = sizeof(u32), > + [OVS_ACTION_ATTR_EMIT_SAMPLE] = (u32)-1, > }; > const struct ovs_action_push_vlan *vlan; > int type = nla_type(a); > @@ -3490,6 +3515,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > return -EINVAL; > break; > > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > + err = validate_emit_sample(a); > + if (err) > + return err; > + break; > + > default: > OVS_NLERR(log, "Unknown Action type %d", type); > return -EINVAL; > -- > 2.45.1
On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote: > On 6/25/24 22:51, 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 <amorenoz@redhat.com> > > --- > > Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > > net/openvswitch/Kconfig | 1 + > > net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > > 5 files changed, 123 insertions(+), 1 deletion(-) > > > > 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..8cfa1b3f6b06 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. > > + */ > > +enum ovs_emit_sample_attr { > > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > + > > + /* private: */ > > + __OVS_EMIT_SAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > > + > > /** > > * enum ovs_action_attr - Action types. > > * > > @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external > > + * observers via psample. > > * > > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all > > * fields within a header are modifiable, e.g. the IPv4 protocol and fragment > > @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig > > index 29a7081858cd..2535f3f9f462 100644 > > --- a/net/openvswitch/Kconfig > > +++ b/net/openvswitch/Kconfig > > @@ -10,6 +10,7 @@ config OPENVSWITCH > > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > > (!NF_NAT || NF_NAT) && \ > > (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) > > + depends on PSAMPLE || !PSAMPLE > > select LIBCRC32C > > select MPLS > > select NET_MPLS_GSO > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > return 0; > > } > > > > +static void execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > > + const struct sw_flow_key *key, > > The 'key' is not used in the function. > > > + const struct nlattr *attr) > > +{ > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > IIUC, the general coding style guideline is to compile out the whole > function, instead of only the parts. i.e. something like: > > #if IS_ENABLED(CONFIG_PSAMPLE) > static void execute_emit_sample(...) { > <body> > } > #else > #define execute_emit_sample(dp, skb, attr) > #endif > > > Otherwise, we'll also need to mark the arguments with __maybe_unused. Thanks for the suggestion, will submit another version with this fix. Adrián > > The rest of the patch looks good to me. > > Best regards, Ilya Maximets. >
On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > > > On 25 Jun 2024, at 22:51, 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. > > I’ll add the same comment as I had in the user space part, and that > is that I feel from an OVS perspective this action should be called > emit_local() instead of emit_sample() to make it Datapath independent. > Or quoting the earlier comment: > > > “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.” > This is a blurry semantic area. IMO, "sample" is the act of extracting (potentially a piece of) someting, in this case, a packet. It is common to only take some packets as samples, so this action usually comes with some kind of "rate", but even if the rate is 1, it's still sampling in this context. OTOH, OVS kernel design tries to be super-modular and define small combinable actions, so the rate or probability generation is done with another action which is (IMHO unfortunately) named "sample". With that interpretation of the term it would actually make more sense to rename "sample" to something like "random" (of course I'm not suggestion we do it). "sample" without any nested action that actually sends the packet somewhere is not sampling, it's just doing something or not based on a probability. Where as "emit_sample" is sampling even if it's not nested inside a "sample". Having said that, I don't have a super strong favor for "emit_sample". I'm OK with "emit_local" or "emit_packet" or even just "emit". I don't think any term will fully satisfy everyone so I hope we can find a reasonable compromise. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > --- > > Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > > net/openvswitch/Kconfig | 1 + > > net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > > 5 files changed, 123 insertions(+), 1 deletion(-) > > > > 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..8cfa1b3f6b06 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. > > Although this include file is kernel-related, it will probably be re-used for > other datapaths, so should we be more general here? > The uAPI header documentation will be used for other datapaths? How so? At some point we should document what the action does from the kernel pov, right? Where should we do that if not here? > > + */ > > +enum ovs_emit_sample_attr { > > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > As we start a new set of attributes maybe it would be good starting it off in > alphabetical order? > Having an optional attribute before a mandatory one seems strange to me, wouldn't you agree? > > + > > + /* private: */ > > + __OVS_EMIT_SAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > > + > > /** > > * enum ovs_action_attr - Action types. > > * > > @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external > > + * observers via psample. > > * > > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all > > * fields within a header are modifiable, e.g. the IPv4 protocol and fragment > > @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig > > index 29a7081858cd..2535f3f9f462 100644 > > --- a/net/openvswitch/Kconfig > > +++ b/net/openvswitch/Kconfig > > @@ -10,6 +10,7 @@ config OPENVSWITCH > > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > > (!NF_NAT || NF_NAT) && \ > > (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) > > + depends on PSAMPLE || !PSAMPLE > > select LIBCRC32C > > select MPLS > > select NET_MPLS_GSO > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > return 0; > > } > > > > +static void 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) > > Same comment as Ilya on key and IS_ENABLED() over function. > > > + struct psample_group psample_group = {}; > > + struct psample_metadata md = {}; > > + const struct nlattr *a; > > + int rem; > > + > > + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { > > + 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); > > Do we need to check for any max cookie length? > I don't think so. validate_emit_sample() makes sure the attribute's length within bounds and checking it in the fast path just in case some other memory-corrupting bug has changed it seems an overkill. > > + break; > > + } > > + } > > + > > + psample_group.net = ovs_dp_get_net(dp); > > + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; > > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > > + > > + psample_sample_packet(&psample_group, skb, 0, &md); > > +#endif > > +} > > + > > /* Execute a list of actions against 'skb'. */ > > static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > > struct sw_flow_key *key, > > @@ -1502,6 +1538,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > > ovs_kfree_skb_reason(skb, reason); > > return 0; > > } > > + > > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > > + execute_emit_sample(dp, skb, key, a); > > + OVS_CB(skb)->cutlen = 0; > > + if (nla_is_last(a, rem)) { > > + consume_skb(skb); > > + return 0; > > + } > > + break; > > } > > > > if (unlikely(err)) { > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > > index f224d9bcea5e..29c8cdc44433 100644 > > --- a/net/openvswitch/flow_netlink.c > > +++ b/net/openvswitch/flow_netlink.c > > @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) > > case OVS_ACTION_ATTR_TRUNC: > > case OVS_ACTION_ATTR_USERSPACE: > > case OVS_ACTION_ATTR_DROP: > > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > > break; > > > > case OVS_ACTION_ATTR_CT: > > @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) > > /* Whenever new actions are added, the need to update this > > * function should be considered. > > */ > > - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24); > > + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25); > > > > if (!actions) > > return; > > @@ -3157,6 +3158,29 @@ static int validate_and_copy_check_pkt_len(struct net *net, > > return 0; > > } > > > > +static int validate_emit_sample(const struct nlattr *attr) > > +{ > > + static const struct nla_policy policy[OVS_EMIT_SAMPLE_ATTR_MAX + 1] = { > > + [OVS_EMIT_SAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > > + [OVS_EMIT_SAMPLE_ATTR_COOKIE] = { > > + .type = NLA_BINARY, > > + .len = OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE, > > + }, > > + }; > > + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; > > + int err; > > + > > + if (!IS_ENABLED(CONFIG_PSAMPLE)) > > + return -EOPNOTSUPP; > > + > > + err = nla_parse_nested(a, OVS_EMIT_SAMPLE_ATTR_MAX, attr, policy, > > + NULL); > > + if (err) > > + return err; > > + > > + return a[OVS_EMIT_SAMPLE_ATTR_GROUP] ? 0 : -EINVAL; > > So we are ok with not having a cookie? Did you inform Cookie Monster ;) > Also, update the include help text to reflect this. > You mean the uapi header? The enum is defined as: enum ovs_emit_sample_attr { OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ Isn't that clear enough? > > +} > > + > > static int copy_action(const struct nlattr *from, > > struct sw_flow_actions **sfa, bool log) > > { > > @@ -3212,6 +3236,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > > [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls), > > [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1, > > [OVS_ACTION_ATTR_DROP] = sizeof(u32), > > + [OVS_ACTION_ATTR_EMIT_SAMPLE] = (u32)-1, > > }; > > const struct ovs_action_push_vlan *vlan; > > int type = nla_type(a); > > @@ -3490,6 +3515,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > > return -EINVAL; > > break; > > > > + case OVS_ACTION_ATTR_EMIT_SAMPLE: > > + err = validate_emit_sample(a); > > + if (err) > > + return err; > > + break; > > + > > default: > > OVS_NLERR(log, "Unknown Action type %d", type); > > return -EINVAL; > > -- > > 2.45.1 >
On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >> >> On 25 Jun 2024, at 22:51, 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. >> >> I’ll add the same comment as I had in the user space part, and that >> is that I feel from an OVS perspective this action should be called >> emit_local() instead of emit_sample() to make it Datapath independent. >> Or quoting the earlier comment: >> >> >> “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.” >> > > This is a blurry semantic area. > IMO, "sample" is the act of extracting (potentially a piece of) > someting, in this case, a packet. It is common to only take some packets > as samples, so this action usually comes with some kind of "rate", but > even if the rate is 1, it's still sampling in this context. > > OTOH, OVS kernel design tries to be super-modular and define small > combinable actions, so the rate or probability generation is done with > another action which is (IMHO unfortunately) named "sample". > > With that interpretation of the term it would actually make more sense > to rename "sample" to something like "random" (of course I'm not > suggestion we do it). "sample" without any nested action that actually > sends the packet somewhere is not sampling, it's just doing something or > not based on a probability. Where as "emit_sample" is sampling even if > it's not nested inside a "sample". You're assuming we are extracting a packet for sampling, but this function can be used for various other purposes. For instance, it could handle the packet outside of the OVS pipeline through an eBPF program (so we are not taking a sample, but continue packet processing outside of the OVS pipeline). Calling it emit_sampling() in such cases could be very confusing. > Having said that, I don't have a super strong favor for "emit_sample". I'm > OK with "emit_local" or "emit_packet" or even just "emit". > I don't think any term will fully satisfy everyone so I hope we can find > a reasonable compromise. My preference would be emit_local() as we hand it off to some local datapath entity. >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>> net/openvswitch/Kconfig | 1 + >>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>> 5 files changed, 123 insertions(+), 1 deletion(-) >>> >>> 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..8cfa1b3f6b06 100644 >>> --- a/include/uapi/linux/openvswitch.h >>> +++ b/include/uapi/linux/openvswitch.h >>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >> >> Although this include file is kernel-related, it will probably be re-used for >> other datapaths, so should we be more general here? >> > > The uAPI header documentation will be used for other datapaths? How so? > At some point we should document what the action does from the kernel > pov, right? Where should we do that if not here? Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? * For the Linux kernel it 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_GROUP = 1, /* u32 number. */ >>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >> >> As we start a new set of attributes maybe it would be good starting it off in >> alphabetical order? >> > > Having an optional attribute before a mandatory one seems strange to me, > wouldn't you agree? I don't mind, but I don't have a strong opinion on it. If others don't mind, I would leave it as is. >>> + >>> + /* private: */ >>> + __OVS_EMIT_SAMPLE_ATTR_MAX >>> +}; >>> + >>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >>> + >>> /** >>> * enum ovs_action_attr - Action types. >>> * >>> @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external >>> + * observers via psample. * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to a data path * local observer. >>> * >>> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all >>> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment >>> @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig >>> index 29a7081858cd..2535f3f9f462 100644 >>> --- a/net/openvswitch/Kconfig >>> +++ b/net/openvswitch/Kconfig >>> @@ -10,6 +10,7 @@ config OPENVSWITCH >>> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ >>> (!NF_NAT || NF_NAT) && \ >>> (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) >>> + depends on PSAMPLE || !PSAMPLE >>> select LIBCRC32C >>> select MPLS >>> select NET_MPLS_GSO >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>> index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >>> return 0; >>> } >>> >>> +static void 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) >> >> Same comment as Ilya on key and IS_ENABLED() over function. >> >>> + struct psample_group psample_group = {}; >>> + struct psample_metadata md = {}; >>> + const struct nlattr *a; >>> + int rem; >>> + >>> + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { >>> + 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); >> >> Do we need to check for any max cookie length? >> > > I don't think so. validate_emit_sample() makes sure the attribute's > length within bounds and checking it in the fast path just in case > some other memory-corrupting bug has changed it seems an overkill. ACK >>> + break; >>> + } >>> + } >>> + >>> + psample_group.net = ovs_dp_get_net(dp); >>> + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; >>> + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; >>> + >>> + psample_sample_packet(&psample_group, skb, 0, &md); >>> +#endif >>> +} >>> + >>> /* Execute a list of actions against 'skb'. */ >>> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, >>> struct sw_flow_key *key, >>> @@ -1502,6 +1538,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, >>> ovs_kfree_skb_reason(skb, reason); >>> return 0; >>> } >>> + >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: >>> + execute_emit_sample(dp, skb, key, a); >>> + OVS_CB(skb)->cutlen = 0; >>> + if (nla_is_last(a, rem)) { >>> + consume_skb(skb); >>> + return 0; >>> + } >>> + break; >>> } >>> >>> if (unlikely(err)) { >>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c >>> index f224d9bcea5e..29c8cdc44433 100644 >>> --- a/net/openvswitch/flow_netlink.c >>> +++ b/net/openvswitch/flow_netlink.c >>> @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) >>> case OVS_ACTION_ATTR_TRUNC: >>> case OVS_ACTION_ATTR_USERSPACE: >>> case OVS_ACTION_ATTR_DROP: >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: >>> break; >>> >>> case OVS_ACTION_ATTR_CT: >>> @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) >>> /* Whenever new actions are added, the need to update this >>> * function should be considered. >>> */ >>> - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24); >>> + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25); >>> >>> if (!actions) >>> return; >>> @@ -3157,6 +3158,29 @@ static int validate_and_copy_check_pkt_len(struct net *net, >>> return 0; >>> } >>> >>> +static int validate_emit_sample(const struct nlattr *attr) >>> +{ >>> + static const struct nla_policy policy[OVS_EMIT_SAMPLE_ATTR_MAX + 1] = { >>> + [OVS_EMIT_SAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, >>> + [OVS_EMIT_SAMPLE_ATTR_COOKIE] = { >>> + .type = NLA_BINARY, >>> + .len = OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE, >>> + }, >>> + }; >>> + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; >>> + int err; >>> + >>> + if (!IS_ENABLED(CONFIG_PSAMPLE)) >>> + return -EOPNOTSUPP; >>> + >>> + err = nla_parse_nested(a, OVS_EMIT_SAMPLE_ATTR_MAX, attr, policy, >>> + NULL); >>> + if (err) >>> + return err; >>> + >>> + return a[OVS_EMIT_SAMPLE_ATTR_GROUP] ? 0 : -EINVAL; >> >> So we are ok with not having a cookie? Did you inform Cookie Monster ;) >> Also, update the include help text to reflect this. >> > > You mean the uapi header? The enum is defined as: > > enum ovs_emit_sample_attr { > OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > Isn't that clear enough? Missed it as I was looking for it here: * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains * user-defined metadata. The maximum length is OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE * bytes. Maybe change it here too by adding option, “An optional variable-length binary cookie”? >>> +} >>> + >>> static int copy_action(const struct nlattr *from, >>> struct sw_flow_actions **sfa, bool log) >>> { >>> @@ -3212,6 +3236,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, >>> [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls), >>> [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1, >>> [OVS_ACTION_ATTR_DROP] = sizeof(u32), >>> + [OVS_ACTION_ATTR_EMIT_SAMPLE] = (u32)-1, >>> }; >>> const struct ovs_action_push_vlan *vlan; >>> int type = nla_type(a); >>> @@ -3490,6 +3515,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, >>> return -EINVAL; >>> break; >>> >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: >>> + err = validate_emit_sample(a); >>> + if (err) >>> + return err; >>> + break; >>> + >>> default: >>> OVS_NLERR(log, "Unknown Action type %d", type); >>> return -EINVAL; >>> -- >>> 2.45.1 >>
On 6/26/24 22:07, Adrián Moreno wrote: > On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote: >> On 6/25/24 22:51, 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 <amorenoz@redhat.com> >>> --- >>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>> net/openvswitch/Kconfig | 1 + >>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>> 5 files changed, 123 insertions(+), 1 deletion(-) >>> >>> 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..8cfa1b3f6b06 100644 >>> --- a/include/uapi/linux/openvswitch.h >>> +++ b/include/uapi/linux/openvswitch.h >>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >>> + */ >>> +enum ovs_emit_sample_attr { >>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>> + >>> + /* private: */ >>> + __OVS_EMIT_SAMPLE_ATTR_MAX >>> +}; >>> + >>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >>> + >>> /** >>> * enum ovs_action_attr - Action types. >>> * >>> @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external >>> + * observers via psample. >>> * >>> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all >>> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment >>> @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig >>> index 29a7081858cd..2535f3f9f462 100644 >>> --- a/net/openvswitch/Kconfig >>> +++ b/net/openvswitch/Kconfig >>> @@ -10,6 +10,7 @@ config OPENVSWITCH >>> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ >>> (!NF_NAT || NF_NAT) && \ >>> (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) >>> + depends on PSAMPLE || !PSAMPLE >>> select LIBCRC32C >>> select MPLS >>> select NET_MPLS_GSO >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>> index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >>> return 0; >>> } >>> >>> +static void execute_emit_sample(struct datapath *dp, struct sk_buff *skb, >>> + const struct sw_flow_key *key, >> >> The 'key' is not used in the function. >> >>> + const struct nlattr *attr) >>> +{ >>> +#if IS_ENABLED(CONFIG_PSAMPLE) >> >> IIUC, the general coding style guideline is to compile out the whole >> function, instead of only the parts. i.e. something like: >> >> #if IS_ENABLED(CONFIG_PSAMPLE) >> static void execute_emit_sample(...) { >> <body> >> } >> #else >> #define execute_emit_sample(dp, skb, attr) >> #endif >> >> >> Otherwise, we'll also need to mark the arguments with __maybe_unused. > > Thanks for the suggestion, will submit another version with this fix. It seems like current coding style document prefers static inline functions over macros in this case. So, that might be a better choice. So, up to you, unless maintainers have a strong opinion one way or another. > > Adrián > >> >> The rest of the patch looks good to me. >> >> Best regards, Ilya Maximets. >> >
On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > > > On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > > > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >> > >> > >> On 25 Jun 2024, at 22:51, 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. > >> > >> I’ll add the same comment as I had in the user space part, and that > >> is that I feel from an OVS perspective this action should be called > >> emit_local() instead of emit_sample() to make it Datapath independent. > >> Or quoting the earlier comment: > >> > >> > >> “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.” > >> > > > > This is a blurry semantic area. > > IMO, "sample" is the act of extracting (potentially a piece of) > > someting, in this case, a packet. It is common to only take some packets > > as samples, so this action usually comes with some kind of "rate", but > > even if the rate is 1, it's still sampling in this context. > > > > OTOH, OVS kernel design tries to be super-modular and define small > > combinable actions, so the rate or probability generation is done with > > another action which is (IMHO unfortunately) named "sample". > > > > With that interpretation of the term it would actually make more sense > > to rename "sample" to something like "random" (of course I'm not > > suggestion we do it). "sample" without any nested action that actually > > sends the packet somewhere is not sampling, it's just doing something or > > not based on a probability. Where as "emit_sample" is sampling even if > > it's not nested inside a "sample". > > You're assuming we are extracting a packet for sampling, but this function > can be used for various other purposes. For instance, it could handle the > packet outside of the OVS pipeline through an eBPF program (so we are not > taking a sample, but continue packet processing outside of the OVS > pipeline). Calling it emit_sampling() in such cases could be very > confusing. > Well, I guess that would be clearly abusing the action. You could say that of anything really. You could hook into skb_consume and continue processing the skb but that doesn't change the intended behavior of the drop action. The intended behavior of the action is sampling, as is the intended behavior of "psample". > > Having said that, I don't have a super strong favor for "emit_sample". I'm > > OK with "emit_local" or "emit_packet" or even just "emit". > > I don't think any term will fully satisfy everyone so I hope we can find > > a reasonable compromise. > > My preference would be emit_local() as we hand it off to some local > datapath entity. > I'm OK removing the controversial term. Let's see what others think. > >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > >>> --- > >>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > >>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > >>> net/openvswitch/Kconfig | 1 + > >>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > >>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > >>> 5 files changed, 123 insertions(+), 1 deletion(-) > >>> > >>> 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..8cfa1b3f6b06 100644 > >>> --- a/include/uapi/linux/openvswitch.h > >>> +++ b/include/uapi/linux/openvswitch.h > >>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. > >> > >> Although this include file is kernel-related, it will probably be re-used for > >> other datapaths, so should we be more general here? > >> > > > > The uAPI header documentation will be used for other datapaths? How so? > > At some point we should document what the action does from the kernel > > pov, right? Where should we do that if not here? > > Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? > > * For the Linux kernel it 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. > I know we reuse the kernel attributes I don't think the uAPI documentation should be less expressive just because some userspace application decides to reuse parts of it. There are many kernel-specific terms all over the uAPI ("netdev", "netlink pid", "skb", even the action "userspace") that do not make sense in a non-kernel datapath. Maybe we can add such a comment in the copy of the header we store in the ovs tree? > >>> + */ > >>> +enum ovs_emit_sample_attr { > >>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > >>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > >> > >> As we start a new set of attributes maybe it would be good starting it off in > >> alphabetical order? > >> > > > > Having an optional attribute before a mandatory one seems strange to me, > > wouldn't you agree? > > I don't mind, but I don't have a strong opinion on it. If others don't mind, > I would leave it as is. > I think I prefer to put mandatory attributes first. > >>> + > >>> + /* private: */ > >>> + __OVS_EMIT_SAMPLE_ATTR_MAX > >>> +}; > >>> + > >>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > >>> + > >>> /** > >>> * enum ovs_action_attr - Action types. > >>> * > >>> @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external > >>> + * observers via psample. > > * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to a data path > * local observer. > > >>> * > >>> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all > >>> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment > >>> @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig > >>> index 29a7081858cd..2535f3f9f462 100644 > >>> --- a/net/openvswitch/Kconfig > >>> +++ b/net/openvswitch/Kconfig > >>> @@ -10,6 +10,7 @@ config OPENVSWITCH > >>> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ > >>> (!NF_NAT || NF_NAT) && \ > >>> (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) > >>> + depends on PSAMPLE || !PSAMPLE > >>> select LIBCRC32C > >>> select MPLS > >>> select NET_MPLS_GSO > >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > >>> index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > >>> return 0; > >>> } > >>> > >>> +static void 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) > >> > >> Same comment as Ilya on key and IS_ENABLED() over function. > >> > >>> + struct psample_group psample_group = {}; > >>> + struct psample_metadata md = {}; > >>> + const struct nlattr *a; > >>> + int rem; > >>> + > >>> + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { > >>> + 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); > >> > >> Do we need to check for any max cookie length? > >> > > > > I don't think so. validate_emit_sample() makes sure the attribute's > > length within bounds and checking it in the fast path just in case > > some other memory-corrupting bug has changed it seems an overkill. > > ACK > > >>> + break; > >>> + } > >>> + } > >>> + > >>> + psample_group.net = ovs_dp_get_net(dp); > >>> + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; > >>> + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > >>> + > >>> + psample_sample_packet(&psample_group, skb, 0, &md); > >>> +#endif > >>> +} > >>> + > >>> /* Execute a list of actions against 'skb'. */ > >>> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > >>> struct sw_flow_key *key, > >>> @@ -1502,6 +1538,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > >>> ovs_kfree_skb_reason(skb, reason); > >>> return 0; > >>> } > >>> + > >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: > >>> + execute_emit_sample(dp, skb, key, a); > >>> + OVS_CB(skb)->cutlen = 0; > >>> + if (nla_is_last(a, rem)) { > >>> + consume_skb(skb); > >>> + return 0; > >>> + } > >>> + break; > >>> } > >>> > >>> if (unlikely(err)) { > >>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > >>> index f224d9bcea5e..29c8cdc44433 100644 > >>> --- a/net/openvswitch/flow_netlink.c > >>> +++ b/net/openvswitch/flow_netlink.c > >>> @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) > >>> case OVS_ACTION_ATTR_TRUNC: > >>> case OVS_ACTION_ATTR_USERSPACE: > >>> case OVS_ACTION_ATTR_DROP: > >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: > >>> break; > >>> > >>> case OVS_ACTION_ATTR_CT: > >>> @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) > >>> /* Whenever new actions are added, the need to update this > >>> * function should be considered. > >>> */ > >>> - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24); > >>> + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25); > >>> > >>> if (!actions) > >>> return; > >>> @@ -3157,6 +3158,29 @@ static int validate_and_copy_check_pkt_len(struct net *net, > >>> return 0; > >>> } > >>> > >>> +static int validate_emit_sample(const struct nlattr *attr) > >>> +{ > >>> + static const struct nla_policy policy[OVS_EMIT_SAMPLE_ATTR_MAX + 1] = { > >>> + [OVS_EMIT_SAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > >>> + [OVS_EMIT_SAMPLE_ATTR_COOKIE] = { > >>> + .type = NLA_BINARY, > >>> + .len = OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE, > >>> + }, > >>> + }; > >>> + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; > >>> + int err; > >>> + > >>> + if (!IS_ENABLED(CONFIG_PSAMPLE)) > >>> + return -EOPNOTSUPP; > >>> + > >>> + err = nla_parse_nested(a, OVS_EMIT_SAMPLE_ATTR_MAX, attr, policy, > >>> + NULL); > >>> + if (err) > >>> + return err; > >>> + > >>> + return a[OVS_EMIT_SAMPLE_ATTR_GROUP] ? 0 : -EINVAL; > >> > >> So we are ok with not having a cookie? Did you inform Cookie Monster ;) > >> Also, update the include help text to reflect this. > >> > > > > You mean the uapi header? The enum is defined as: > > > > enum ovs_emit_sample_attr { > > OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > > OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > > > Isn't that clear enough? > > Missed it as I was looking for it here: > > * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > * user-defined metadata. The maximum length is OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE > * bytes. > > Maybe change it here too by adding option, “An optional variable-length binary cookie”? > Sure. > >>> +} > >>> + > >>> static int copy_action(const struct nlattr *from, > >>> struct sw_flow_actions **sfa, bool log) > >>> { > >>> @@ -3212,6 +3236,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > >>> [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls), > >>> [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1, > >>> [OVS_ACTION_ATTR_DROP] = sizeof(u32), > >>> + [OVS_ACTION_ATTR_EMIT_SAMPLE] = (u32)-1, > >>> }; > >>> const struct ovs_action_push_vlan *vlan; > >>> int type = nla_type(a); > >>> @@ -3490,6 +3515,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > >>> return -EINVAL; > >>> break; > >>> > >>> + case OVS_ACTION_ATTR_EMIT_SAMPLE: > >>> + err = validate_emit_sample(a); > >>> + if (err) > >>> + return err; > >>> + break; > >>> + > >>> default: > >>> OVS_NLERR(log, "Unknown Action type %d", type); > >>> return -EINVAL; > >>> -- > >>> 2.45.1 > >> >
On 6/27/24 09:52, Adrián Moreno wrote: > On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >> >> >> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >> >>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>> >>>> >>>> On 25 Jun 2024, at 22:51, 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. >>>> >>>> I’ll add the same comment as I had in the user space part, and that >>>> is that I feel from an OVS perspective this action should be called >>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>> Or quoting the earlier comment: >>>> >>>> >>>> “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.” >>>> >>> >>> This is a blurry semantic area. >>> IMO, "sample" is the act of extracting (potentially a piece of) >>> someting, in this case, a packet. It is common to only take some packets >>> as samples, so this action usually comes with some kind of "rate", but >>> even if the rate is 1, it's still sampling in this context. >>> >>> OTOH, OVS kernel design tries to be super-modular and define small >>> combinable actions, so the rate or probability generation is done with >>> another action which is (IMHO unfortunately) named "sample". >>> >>> With that interpretation of the term it would actually make more sense >>> to rename "sample" to something like "random" (of course I'm not >>> suggestion we do it). "sample" without any nested action that actually >>> sends the packet somewhere is not sampling, it's just doing something or >>> not based on a probability. Where as "emit_sample" is sampling even if >>> it's not nested inside a "sample". >> >> You're assuming we are extracting a packet for sampling, but this function >> can be used for various other purposes. For instance, it could handle the >> packet outside of the OVS pipeline through an eBPF program (so we are not >> taking a sample, but continue packet processing outside of the OVS >> pipeline). Calling it emit_sampling() in such cases could be very >> confusing. We can't change the implementation of the action once it is part of uAPI. We have to document where users can find these packets and we can't just change the destination later. >> > > Well, I guess that would be clearly abusing the action. You could say > that of anything really. You could hook into skb_consume and continue > processing the skb but that doesn't change the intended behavior of the > drop action. > > The intended behavior of the action is sampling, as is the intended > behavior of "psample". The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", that is it takes some packets from the whole packet stream and executes actions of them. Without tying this to observability purposes the name makes sense as the first definition of the word is "to take a representative part or a single item from a larger whole or group". Now, our new action doesn't have this particular semantic in a way that it doesn't take a part of a whole packet stream but rather using the part already taken. However, it is directly tied to the parent OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent action. If there is no parent, then probability is assumed to be 100%, but that's just a corner case. The name of a psample module has the same semantics in its name, it doesn't sample on it's own, but it is assuming that sampling was performed as it relays the rate of it. And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and the psample module, the emit_sample() name makes sense to me. > >>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>> OK with "emit_local" or "emit_packet" or even just "emit". The 'local' or 'packet' variants are not descriptive enough on what we're trying to achieve and do not explain why the probability is attached to the action, i.e. do not explain the link between this action and the OVS_ACTION_ATTR_SAMPLE. emit_Psample() would be overly specific, I agree, but making the name too generic will also make it hard to add new actions. If we use some overly broad term for this one, we may have to deal with overlapping semantics in the future. >>> I don't think any term will fully satisfy everyone so I hope we can find >>> a reasonable compromise. >> >> My preference would be emit_local() as we hand it off to some local >> datapath entity. What is "local datapath entity" ? psample module is not part of OVS datapath. And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented by a bridge port on a datapath level, that will be another source of confusion as it can be interpreted as sending a packet via a local bridge port. >> > > I'm OK removing the controversial term. Let's see what others think. > >>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>>> --- >>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>>>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>>>> net/openvswitch/Kconfig | 1 + >>>>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>>>> 5 files changed, 123 insertions(+), 1 deletion(-) >>>>> >>>>> 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..8cfa1b3f6b06 100644 >>>>> --- a/include/uapi/linux/openvswitch.h >>>>> +++ b/include/uapi/linux/openvswitch.h >>>>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >>>> >>>> Although this include file is kernel-related, it will probably be re-used for >>>> other datapaths, so should we be more general here? >>>> >>> >>> The uAPI header documentation will be used for other datapaths? How so? >>> At some point we should document what the action does from the kernel >>> pov, right? Where should we do that if not here? >> >> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? >> >> * For the Linux kernel it 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. >> > > I know we reuse the kernel attributes I don't think the uAPI > documentation should be less expressive just because some userspace > application decides to reuse parts of it. > > There are many kernel-specific terms all over the uAPI ("netdev", > "netlink pid", "skb", even the action "userspace") that do not make > sense in a non-kernel datapath. +1 This is a kernel uAPI header it describes the behavior of the kernel. Having parts like "For the Linux kernel" in here is awkward. > > Maybe we can add such a comment in the copy of the header we store in > the ovs tree? Makes sense to me. If we'll want to implement a similar action in userspace datapath, we'll have to have a separate documentation for it anyway, since the packets will end up in a different place for users to collect. > > >>>>> + */ >>>>> +enum ovs_emit_sample_attr { >>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>>> >>>> As we start a new set of attributes maybe it would be good starting it off in >>>> alphabetical order? >>>> >>> >>> Having an optional attribute before a mandatory one seems strange to me, >>> wouldn't you agree? >> >> I don't mind, but I don't have a strong opinion on it. If others don't mind, >> I would leave it as is. >> > > I think I prefer to put mandatory attributes first. That's my thought as well. Though that might be broken if we ever need more attributes. But we do not extend individual actions that often. Best regards, Ilya Maximets.
On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > On 6/27/24 09:52, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>> >>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 25 Jun 2024, at 22:51, 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. >>>>> >>>>> I’ll add the same comment as I had in the user space part, and that >>>>> is that I feel from an OVS perspective this action should be called >>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>> Or quoting the earlier comment: >>>>> >>>>> >>>>> “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.” >>>>> >>>> >>>> This is a blurry semantic area. >>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>> someting, in this case, a packet. It is common to only take some packets >>>> as samples, so this action usually comes with some kind of "rate", but >>>> even if the rate is 1, it's still sampling in this context. >>>> >>>> OTOH, OVS kernel design tries to be super-modular and define small >>>> combinable actions, so the rate or probability generation is done with >>>> another action which is (IMHO unfortunately) named "sample". >>>> >>>> With that interpretation of the term it would actually make more sense >>>> to rename "sample" to something like "random" (of course I'm not >>>> suggestion we do it). "sample" without any nested action that actually >>>> sends the packet somewhere is not sampling, it's just doing something or >>>> not based on a probability. Where as "emit_sample" is sampling even if >>>> it's not nested inside a "sample". >>> >>> You're assuming we are extracting a packet for sampling, but this function >>> can be used for various other purposes. For instance, it could handle the >>> packet outside of the OVS pipeline through an eBPF program (so we are not >>> taking a sample, but continue packet processing outside of the OVS >>> pipeline). Calling it emit_sampling() in such cases could be very >>> confusing. > > We can't change the implementation of the action once it is part of uAPI. > We have to document where users can find these packets and we can't just > change the destination later. I'm not suggesting we change the uAPI implementation, but we could use the emit_xxx() action with an eBPF probe on the action to perform other tasks. This is just an example. >> Well, I guess that would be clearly abusing the action. You could say >> that of anything really. You could hook into skb_consume and continue >> processing the skb but that doesn't change the intended behavior of the >> drop action. >> >> The intended behavior of the action is sampling, as is the intended >> behavior of "psample". > > The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > that is it takes some packets from the whole packet stream and executes > actions of them. Without tying this to observability purposes the name > makes sense as the first definition of the word is "to take a representative > part or a single item from a larger whole or group". > > Now, our new action doesn't have this particular semantic in a way that > it doesn't take a part of a whole packet stream but rather using the > part already taken. However, it is directly tied to the parent > OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent > action. If there is no parent, then probability is assumed to be 100%, > but that's just a corner case. The name of a psample module has the > same semantics in its name, it doesn't sample on it's own, but it is > assuming that sampling was performed as it relays the rate of it. > > And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and > the psample module, the emit_sample() name makes sense to me. This is the part I don't like. emit_sample() should be treated as a standalone action. While it may have potential dependencies on OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it independently. >>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>> OK with "emit_local" or "emit_packet" or even just "emit". > > The 'local' or 'packet' variants are not descriptive enough on what we're > trying to achieve and do not explain why the probability is attached to > the action, i.e. do not explain the link between this action and the > OVS_ACTION_ATTR_SAMPLE. > > emit_Psample() would be overly specific, I agree, but making the name too > generic will also make it hard to add new actions. If we use some overly > broad term for this one, we may have to deal with overlapping semantics in > the future. > >>>> I don't think any term will fully satisfy everyone so I hope we can find >>>> a reasonable compromise. >>> >>> My preference would be emit_local() as we hand it off to some local >>> datapath entity. > > What is "local datapath entity" ? psample module is not part of OVS datapath. > And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented > by a bridge port on a datapath level, that will be another source of confusion > as it can be interpreted as sending a packet via a local bridge port. I guess I hinted at a local exit point in the specific netdev/netlink datapath, where exit is to the local host. So maybe we should call it emit_localhost? >> I'm OK removing the controversial term. Let's see what others think. >> >>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>>>> --- >>>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>>>>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>>>>> net/openvswitch/Kconfig | 1 + >>>>>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>>>>> 5 files changed, 123 insertions(+), 1 deletion(-) >>>>>> >>>>>> 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..8cfa1b3f6b06 100644 >>>>>> --- a/include/uapi/linux/openvswitch.h >>>>>> +++ b/include/uapi/linux/openvswitch.h >>>>>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >>>>> >>>>> Although this include file is kernel-related, it will probably be re-used for >>>>> other datapaths, so should we be more general here? >>>>> >>>> >>>> The uAPI header documentation will be used for other datapaths? How so? >>>> At some point we should document what the action does from the kernel >>>> pov, right? Where should we do that if not here? >>> >>> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? >>> >>> * For the Linux kernel it 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. >>> >> >> I know we reuse the kernel attributes I don't think the uAPI >> documentation should be less expressive just because some userspace >> application decides to reuse parts of it. >> >> There are many kernel-specific terms all over the uAPI ("netdev", >> "netlink pid", "skb", even the action "userspace") that do not make >> sense in a non-kernel datapath. > > +1 > > This is a kernel uAPI header it describes the behavior of the kernel. > Having parts like "For the Linux kernel" in here is awkward. > >> >> Maybe we can add such a comment in the copy of the header we store in >> the ovs tree? > > Makes sense to me. > > If we'll want to implement a similar action in userspace datapath, > we'll have to have a separate documentation for it anyway, since > the packets will end up in a different place for users to collect. > >> >> >>>>>> + */ >>>>>> +enum ovs_emit_sample_attr { >>>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>>>> >>>>> As we start a new set of attributes maybe it would be good starting it off in >>>>> alphabetical order? >>>>> >>>> >>>> Having an optional attribute before a mandatory one seems strange to me, >>>> wouldn't you agree? >>> >>> I don't mind, but I don't have a strong opinion on it. If others don't mind, >>> I would leave it as is. >>> >> >> I think I prefer to put mandatory attributes first. > > That's my thought as well. Though that might be broken if we ever need > more attributes. But we do not extend individual actions that often. > > Best regards, Ilya Maximets.
On 6/27/24 11:14, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> On 6/27/24 09:52, Adrián Moreno wrote: >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>>> >>>> >>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>>> >>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>>> >>>>>> >>>>>> On 25 Jun 2024, at 22:51, 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. >>>>>> >>>>>> I’ll add the same comment as I had in the user space part, and that >>>>>> is that I feel from an OVS perspective this action should be called >>>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>>> Or quoting the earlier comment: >>>>>> >>>>>> >>>>>> “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.” >>>>>> >>>>> >>>>> This is a blurry semantic area. >>>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>>> someting, in this case, a packet. It is common to only take some packets >>>>> as samples, so this action usually comes with some kind of "rate", but >>>>> even if the rate is 1, it's still sampling in this context. >>>>> >>>>> OTOH, OVS kernel design tries to be super-modular and define small >>>>> combinable actions, so the rate or probability generation is done with >>>>> another action which is (IMHO unfortunately) named "sample". >>>>> >>>>> With that interpretation of the term it would actually make more sense >>>>> to rename "sample" to something like "random" (of course I'm not >>>>> suggestion we do it). "sample" without any nested action that actually >>>>> sends the packet somewhere is not sampling, it's just doing something or >>>>> not based on a probability. Where as "emit_sample" is sampling even if >>>>> it's not nested inside a "sample". >>>> >>>> You're assuming we are extracting a packet for sampling, but this function >>>> can be used for various other purposes. For instance, it could handle the >>>> packet outside of the OVS pipeline through an eBPF program (so we are not >>>> taking a sample, but continue packet processing outside of the OVS >>>> pipeline). Calling it emit_sampling() in such cases could be very >>>> confusing. >> >> We can't change the implementation of the action once it is part of uAPI. >> We have to document where users can find these packets and we can't just >> change the destination later. > > I'm not suggesting we change the uAPI implementation, but we could use the > emit_xxx() action with an eBPF probe on the action to perform other tasks. > This is just an example. Yeah, but as Adrian said below, you could do that with any action and this doesn't change the semantics of the action itself. > >>> Well, I guess that would be clearly abusing the action. You could say >>> that of anything really. You could hook into skb_consume and continue >>> processing the skb but that doesn't change the intended behavior of the >>> drop action. >>> >>> The intended behavior of the action is sampling, as is the intended >>> behavior of "psample". >> >> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >> that is it takes some packets from the whole packet stream and executes >> actions of them. Without tying this to observability purposes the name >> makes sense as the first definition of the word is "to take a representative >> part or a single item from a larger whole or group". >> >> Now, our new action doesn't have this particular semantic in a way that >> it doesn't take a part of a whole packet stream but rather using the >> part already taken. However, it is directly tied to the parent >> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >> action. If there is no parent, then probability is assumed to be 100%, >> but that's just a corner case. The name of a psample module has the >> same semantics in its name, it doesn't sample on it's own, but it is >> assuming that sampling was performed as it relays the rate of it. >> >> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >> the psample module, the emit_sample() name makes sense to me. > > This is the part I don't like. emit_sample() should be treated as a > standalone action. While it may have potential dependencies on > OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it > independently. It is fine to use it, we just assume implicit 100% sampling. > >>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>>> OK with "emit_local" or "emit_packet" or even just "emit". >> >> The 'local' or 'packet' variants are not descriptive enough on what we're >> trying to achieve and do not explain why the probability is attached to >> the action, i.e. do not explain the link between this action and the >> OVS_ACTION_ATTR_SAMPLE. >> >> emit_Psample() would be overly specific, I agree, but making the name too >> generic will also make it hard to add new actions. If we use some overly >> broad term for this one, we may have to deal with overlapping semantics in >> the future. >> >>>>> I don't think any term will fully satisfy everyone so I hope we can find >>>>> a reasonable compromise. >>>> >>>> My preference would be emit_local() as we hand it off to some local >>>> datapath entity. >> >> What is "local datapath entity" ? psample module is not part of OVS datapath. >> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented >> by a bridge port on a datapath level, that will be another source of confusion >> as it can be interpreted as sending a packet via a local bridge port. > > I guess I hinted at a local exit point in the specific netdev/netlink datapath, > where exit is to the local host. So maybe we should call it emit_localhost? For me sending to localhost means sending to a loopback interface or otherwise sending the packet to the host networking stack. And we're not doing that. > >>> I'm OK removing the controversial term. Let's see what others think. >>> >>>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>>>>> --- >>>>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>>>>>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>>>>>> net/openvswitch/Kconfig | 1 + >>>>>>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>>>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>>>>>> 5 files changed, 123 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> 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..8cfa1b3f6b06 100644 >>>>>>> --- a/include/uapi/linux/openvswitch.h >>>>>>> +++ b/include/uapi/linux/openvswitch.h >>>>>>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >>>>>> >>>>>> Although this include file is kernel-related, it will probably be re-used for >>>>>> other datapaths, so should we be more general here? >>>>>> >>>>> >>>>> The uAPI header documentation will be used for other datapaths? How so? >>>>> At some point we should document what the action does from the kernel >>>>> pov, right? Where should we do that if not here? >>>> >>>> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? >>>> >>>> * For the Linux kernel it 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. >>>> >>> >>> I know we reuse the kernel attributes I don't think the uAPI >>> documentation should be less expressive just because some userspace >>> application decides to reuse parts of it. >>> >>> There are many kernel-specific terms all over the uAPI ("netdev", >>> "netlink pid", "skb", even the action "userspace") that do not make >>> sense in a non-kernel datapath. >> >> +1 >> >> This is a kernel uAPI header it describes the behavior of the kernel. >> Having parts like "For the Linux kernel" in here is awkward. >> >>> >>> Maybe we can add such a comment in the copy of the header we store in >>> the ovs tree? >> >> Makes sense to me. >> >> If we'll want to implement a similar action in userspace datapath, >> we'll have to have a separate documentation for it anyway, since >> the packets will end up in a different place for users to collect. >> >>> >>> >>>>>>> + */ >>>>>>> +enum ovs_emit_sample_attr { >>>>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>>>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>>>>> >>>>>> As we start a new set of attributes maybe it would be good starting it off in >>>>>> alphabetical order? >>>>>> >>>>> >>>>> Having an optional attribute before a mandatory one seems strange to me, >>>>> wouldn't you agree? >>>> >>>> I don't mind, but I don't have a strong opinion on it. If others don't mind, >>>> I would leave it as is. >>>> >>> >>> I think I prefer to put mandatory attributes first. >> >> That's my thought as well. Though that might be broken if we ever need >> more attributes. But we do not extend individual actions that often. >> >> Best regards, Ilya Maximets. >
On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > On 6/27/24 11:14, Eelco Chaudron wrote: >> >> >> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >> >>> On 6/27/24 09:52, Adrián Moreno wrote: >>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>>>> >>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>>>> >>>>>>> >>>>>>> On 25 Jun 2024, at 22:51, 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. >>>>>>> >>>>>>> I’ll add the same comment as I had in the user space part, and that >>>>>>> is that I feel from an OVS perspective this action should be called >>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>>>> Or quoting the earlier comment: >>>>>>> >>>>>>> >>>>>>> “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.” >>>>>>> >>>>>> >>>>>> This is a blurry semantic area. >>>>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>>>> someting, in this case, a packet. It is common to only take some packets >>>>>> as samples, so this action usually comes with some kind of "rate", but >>>>>> even if the rate is 1, it's still sampling in this context. >>>>>> >>>>>> OTOH, OVS kernel design tries to be super-modular and define small >>>>>> combinable actions, so the rate or probability generation is done with >>>>>> another action which is (IMHO unfortunately) named "sample". >>>>>> >>>>>> With that interpretation of the term it would actually make more sense >>>>>> to rename "sample" to something like "random" (of course I'm not >>>>>> suggestion we do it). "sample" without any nested action that actually >>>>>> sends the packet somewhere is not sampling, it's just doing something or >>>>>> not based on a probability. Where as "emit_sample" is sampling even if >>>>>> it's not nested inside a "sample". >>>>> >>>>> You're assuming we are extracting a packet for sampling, but this function >>>>> can be used for various other purposes. For instance, it could handle the >>>>> packet outside of the OVS pipeline through an eBPF program (so we are not >>>>> taking a sample, but continue packet processing outside of the OVS >>>>> pipeline). Calling it emit_sampling() in such cases could be very >>>>> confusing. >>> >>> We can't change the implementation of the action once it is part of uAPI. >>> We have to document where users can find these packets and we can't just >>> change the destination later. >> >> I'm not suggesting we change the uAPI implementation, but we could use the >> emit_xxx() action with an eBPF probe on the action to perform other tasks. >> This is just an example. > > Yeah, but as Adrian said below, you could do that with any action and > this doesn't change the semantics of the action itself. Well this was just an example, what if we have some other need for getting a packet to userspace through emit_local() other than sampling? The emit_sample() action naming in this case makes no sense. >>>> Well, I guess that would be clearly abusing the action. You could say >>>> that of anything really. You could hook into skb_consume and continue >>>> processing the skb but that doesn't change the intended behavior of the >>>> drop action. >>>> >>>> The intended behavior of the action is sampling, as is the intended >>>> behavior of "psample". >>> >>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >>> that is it takes some packets from the whole packet stream and executes >>> actions of them. Without tying this to observability purposes the name >>> makes sense as the first definition of the word is "to take a representative >>> part or a single item from a larger whole or group". >>> >>> Now, our new action doesn't have this particular semantic in a way that >>> it doesn't take a part of a whole packet stream but rather using the >>> part already taken. However, it is directly tied to the parent >>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >>> action. If there is no parent, then probability is assumed to be 100%, >>> but that's just a corner case. The name of a psample module has the >>> same semantics in its name, it doesn't sample on it's own, but it is >>> assuming that sampling was performed as it relays the rate of it. >>> >>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >>> the psample module, the emit_sample() name makes sense to me. >> >> This is the part I don't like. emit_sample() should be treated as a >> standalone action. While it may have potential dependencies on >> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it >> independently. > > It is fine to use it, we just assume implicit 100% sampling. Agreed, but the name does not make sense ;) I do not think we currently have any actions that explicitly depend on each other (there might be attributes carried over) and I want to keep it as such. >>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>>>> OK with "emit_local" or "emit_packet" or even just "emit". >>> >>> The 'local' or 'packet' variants are not descriptive enough on what we're >>> trying to achieve and do not explain why the probability is attached to >>> the action, i.e. do not explain the link between this action and the >>> OVS_ACTION_ATTR_SAMPLE. >>> >>> emit_Psample() would be overly specific, I agree, but making the name too >>> generic will also make it hard to add new actions. If we use some overly >>> broad term for this one, we may have to deal with overlapping semantics in >>> the future. >>> >>>>>> I don't think any term will fully satisfy everyone so I hope we can find >>>>>> a reasonable compromise. >>>>> >>>>> My preference would be emit_local() as we hand it off to some local >>>>> datapath entity. >>> >>> What is "local datapath entity" ? psample module is not part of OVS datapath. >>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented >>> by a bridge port on a datapath level, that will be another source of confusion >>> as it can be interpreted as sending a packet via a local bridge port. >> >> I guess I hinted at a local exit point in the specific netdev/netlink datapath, >> where exit is to the local host. So maybe we should call it emit_localhost? > > For me sending to localhost means sending to a loopback interface or otherwise > sending the packet to the host networking stack. And we're not doing that. That might be confusing too... Maybe emit_external()? >>>> I'm OK removing the controversial term. Let's see what others think. >>>> >>>>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>>>>>> --- >>>>>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ >>>>>>>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ >>>>>>>> net/openvswitch/Kconfig | 1 + >>>>>>>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ >>>>>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- >>>>>>>> 5 files changed, 123 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> 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..8cfa1b3f6b06 100644 >>>>>>>> --- a/include/uapi/linux/openvswitch.h >>>>>>>> +++ b/include/uapi/linux/openvswitch.h >>>>>>>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. >>>>>>> >>>>>>> Although this include file is kernel-related, it will probably be re-used for >>>>>>> other datapaths, so should we be more general here? >>>>>>> >>>>>> >>>>>> The uAPI header documentation will be used for other datapaths? How so? >>>>>> At some point we should document what the action does from the kernel >>>>>> pov, right? Where should we do that if not here? >>>>> >>>>> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? >>>>> >>>>> * For the Linux kernel it 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. >>>>> >>>> >>>> I know we reuse the kernel attributes I don't think the uAPI >>>> documentation should be less expressive just because some userspace >>>> application decides to reuse parts of it. >>>> >>>> There are many kernel-specific terms all over the uAPI ("netdev", >>>> "netlink pid", "skb", even the action "userspace") that do not make >>>> sense in a non-kernel datapath. >>> >>> +1 >>> >>> This is a kernel uAPI header it describes the behavior of the kernel. >>> Having parts like "For the Linux kernel" in here is awkward. >>> >>>> >>>> Maybe we can add such a comment in the copy of the header we store in >>>> the ovs tree? >>> >>> Makes sense to me. >>> >>> If we'll want to implement a similar action in userspace datapath, >>> we'll have to have a separate documentation for it anyway, since >>> the packets will end up in a different place for users to collect. >>> >>>> >>>> >>>>>>>> + */ >>>>>>>> +enum ovs_emit_sample_attr { >>>>>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ >>>>>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>>>>>> >>>>>>> As we start a new set of attributes maybe it would be good starting it off in >>>>>>> alphabetical order? >>>>>>> >>>>>> >>>>>> Having an optional attribute before a mandatory one seems strange to me, >>>>>> wouldn't you agree? >>>>> >>>>> I don't mind, but I don't have a strong opinion on it. If others don't mind, >>>>> I would leave it as is. >>>>> >>>> >>>> I think I prefer to put mandatory attributes first. >>> >>> That's my thought as well. Though that might be broken if we ever need >>> more attributes. But we do not extend individual actions that often. >>> >>> Best regards, Ilya Maximets. >>
On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > > > On 6/27/24 11:14, Eelco Chaudron wrote: > >> > >> > >> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> > >>> On 6/27/24 09:52, Adrián Moreno wrote: > >>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > >>>>> > >>>>> > >>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > >>>>> > >>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 25 Jun 2024, at 22:51, 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. > >>>>>>> > >>>>>>> I’ll add the same comment as I had in the user space part, and that > >>>>>>> is that I feel from an OVS perspective this action should be called > >>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. > >>>>>>> Or quoting the earlier comment: > >>>>>>> > >>>>>>> > >>>>>>> “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.” > >>>>>>> > >>>>>> > >>>>>> This is a blurry semantic area. > >>>>>> IMO, "sample" is the act of extracting (potentially a piece of) > >>>>>> someting, in this case, a packet. It is common to only take some packets > >>>>>> as samples, so this action usually comes with some kind of "rate", but > >>>>>> even if the rate is 1, it's still sampling in this context. > >>>>>> > >>>>>> OTOH, OVS kernel design tries to be super-modular and define small > >>>>>> combinable actions, so the rate or probability generation is done with > >>>>>> another action which is (IMHO unfortunately) named "sample". > >>>>>> > >>>>>> With that interpretation of the term it would actually make more sense > >>>>>> to rename "sample" to something like "random" (of course I'm not > >>>>>> suggestion we do it). "sample" without any nested action that actually > >>>>>> sends the packet somewhere is not sampling, it's just doing something or > >>>>>> not based on a probability. Where as "emit_sample" is sampling even if > >>>>>> it's not nested inside a "sample". > >>>>> > >>>>> You're assuming we are extracting a packet for sampling, but this function > >>>>> can be used for various other purposes. For instance, it could handle the > >>>>> packet outside of the OVS pipeline through an eBPF program (so we are not > >>>>> taking a sample, but continue packet processing outside of the OVS > >>>>> pipeline). Calling it emit_sampling() in such cases could be very > >>>>> confusing. > >>> > >>> We can't change the implementation of the action once it is part of uAPI. > >>> We have to document where users can find these packets and we can't just > >>> change the destination later. > >> > >> I'm not suggesting we change the uAPI implementation, but we could use the > >> emit_xxx() action with an eBPF probe on the action to perform other tasks. > >> This is just an example. > > > > Yeah, but as Adrian said below, you could do that with any action and > > this doesn't change the semantics of the action itself. > > Well this was just an example, what if we have some other need for getting > a packet to userspace through emit_local() other than sampling? The > emit_sample() action naming in this case makes no sense. > > >>>> Well, I guess that would be clearly abusing the action. You could say > >>>> that of anything really. You could hook into skb_consume and continue > >>>> processing the skb but that doesn't change the intended behavior of the > >>>> drop action. > >>>> > >>>> The intended behavior of the action is sampling, as is the intended > >>>> behavior of "psample". > >>> > >>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > >>> that is it takes some packets from the whole packet stream and executes > >>> actions of them. Without tying this to observability purposes the name > >>> makes sense as the first definition of the word is "to take a representative > >>> part or a single item from a larger whole or group". > >>> > >>> Now, our new action doesn't have this particular semantic in a way that > >>> it doesn't take a part of a whole packet stream but rather using the > >>> part already taken. However, it is directly tied to the parent > >>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent > >>> action. If there is no parent, then probability is assumed to be 100%, > >>> but that's just a corner case. The name of a psample module has the > >>> same semantics in its name, it doesn't sample on it's own, but it is > >>> assuming that sampling was performed as it relays the rate of it. > >>> > >>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and > >>> the psample module, the emit_sample() name makes sense to me. > >> > >> This is the part I don't like. emit_sample() should be treated as a > >> standalone action. While it may have potential dependencies on > >> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it > >> independently. > > > > It is fine to use it, we just assume implicit 100% sampling. > > Agreed, but the name does not make sense ;) I do not think we > currently have any actions that explicitly depend on each other > (there might be attributes carried over) and I want to keep it > as such. > > >>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm > >>>>>> OK with "emit_local" or "emit_packet" or even just "emit". > >>> > >>> The 'local' or 'packet' variants are not descriptive enough on what we're > >>> trying to achieve and do not explain why the probability is attached to > >>> the action, i.e. do not explain the link between this action and the > >>> OVS_ACTION_ATTR_SAMPLE. > >>> > >>> emit_Psample() would be overly specific, I agree, but making the name too > >>> generic will also make it hard to add new actions. If we use some overly > >>> broad term for this one, we may have to deal with overlapping semantics in > >>> the future. > >>> > >>>>>> I don't think any term will fully satisfy everyone so I hope we can find > >>>>>> a reasonable compromise. > >>>>> > >>>>> My preference would be emit_local() as we hand it off to some local > >>>>> datapath entity. > >>> > >>> What is "local datapath entity" ? psample module is not part of OVS datapath. > >>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented > >>> by a bridge port on a datapath level, that will be another source of confusion > >>> as it can be interpreted as sending a packet via a local bridge port. > >> > >> I guess I hinted at a local exit point in the specific netdev/netlink datapath, > >> where exit is to the local host. So maybe we should call it emit_localhost? > > > > For me sending to localhost means sending to a loopback interface or otherwise > > sending the packet to the host networking stack. And we're not doing that. > > That might be confusing too... Maybe emit_external()? "External" was the word I used for the original userspace RFC. The rationale being: We're sending the packet to something external from OVS (datapath or userspace). Compared with IPFIX-based observability which where the sample is first processed ("internally") by ovs-vswitchd. In userspace it kept the sampling/observability meaning because it was part of the Flow_Sample_Collector_Set which is intrinsically an observability thing. However, in the datapath we loose that meaning and could be confused with some external packet-processing entity. How about "external_observe" or something that somehow keeps that meaning? > > >>>> I'm OK removing the controversial term. Let's see what others think. > >>>> > >>>>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > >>>>>>>> --- > >>>>>>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ > >>>>>>>> include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > >>>>>>>> net/openvswitch/Kconfig | 1 + > >>>>>>>> net/openvswitch/actions.c | 45 +++++++++++++++++++++++ > >>>>>>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- > >>>>>>>> 5 files changed, 123 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> 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..8cfa1b3f6b06 100644 > >>>>>>>> --- a/include/uapi/linux/openvswitch.h > >>>>>>>> +++ b/include/uapi/linux/openvswitch.h > >>>>>>>> @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. > >>>>>>> > >>>>>>> Although this include file is kernel-related, it will probably be re-used for > >>>>>>> other datapaths, so should we be more general here? > >>>>>>> > >>>>>> > >>>>>> The uAPI header documentation will be used for other datapaths? How so? > >>>>>> At some point we should document what the action does from the kernel > >>>>>> pov, right? Where should we do that if not here? > >>>>> > >>>>> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general? > >>>>> > >>>>> * For the Linux kernel it 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. > >>>>> > >>>> > >>>> I know we reuse the kernel attributes I don't think the uAPI > >>>> documentation should be less expressive just because some userspace > >>>> application decides to reuse parts of it. > >>>> > >>>> There are many kernel-specific terms all over the uAPI ("netdev", > >>>> "netlink pid", "skb", even the action "userspace") that do not make > >>>> sense in a non-kernel datapath. > >>> > >>> +1 > >>> > >>> This is a kernel uAPI header it describes the behavior of the kernel. > >>> Having parts like "For the Linux kernel" in here is awkward. > >>> > >>>> > >>>> Maybe we can add such a comment in the copy of the header we store in > >>>> the ovs tree? > >>> > >>> Makes sense to me. > >>> > >>> If we'll want to implement a similar action in userspace datapath, > >>> we'll have to have a separate documentation for it anyway, since > >>> the packets will end up in a different place for users to collect. > >>> > >>>> > >>>> > >>>>>>>> + */ > >>>>>>>> +enum ovs_emit_sample_attr { > >>>>>>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ > >>>>>>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > >>>>>>> > >>>>>>> As we start a new set of attributes maybe it would be good starting it off in > >>>>>>> alphabetical order? > >>>>>>> > >>>>>> > >>>>>> Having an optional attribute before a mandatory one seems strange to me, > >>>>>> wouldn't you agree? > >>>>> > >>>>> I don't mind, but I don't have a strong opinion on it. If others don't mind, > >>>>> I would leave it as is. > >>>>> > >>>> > >>>> I think I prefer to put mandatory attributes first. > >>> > >>> That's my thought as well. Though that might be broken if we ever need > >>> more attributes. But we do not extend individual actions that often. > >>> > >>> Best regards, Ilya Maximets. > >> >
On 6/27/24 12:15, Adrián Moreno wrote: > On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >> >> >> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >> >>> On 6/27/24 11:14, Eelco Chaudron wrote: >>>> >>>> >>>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >>>> >>>>> On 6/27/24 09:52, Adrián Moreno wrote: >>>>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>>>>>> >>>>>>> >>>>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>>>>>> >>>>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 25 Jun 2024, at 22:51, 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. >>>>>>>>> >>>>>>>>> I’ll add the same comment as I had in the user space part, and that >>>>>>>>> is that I feel from an OVS perspective this action should be called >>>>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>>>>>> Or quoting the earlier comment: >>>>>>>>> >>>>>>>>> >>>>>>>>> “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.” >>>>>>>>> >>>>>>>> >>>>>>>> This is a blurry semantic area. >>>>>>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>>>>>> someting, in this case, a packet. It is common to only take some packets >>>>>>>> as samples, so this action usually comes with some kind of "rate", but >>>>>>>> even if the rate is 1, it's still sampling in this context. >>>>>>>> >>>>>>>> OTOH, OVS kernel design tries to be super-modular and define small >>>>>>>> combinable actions, so the rate or probability generation is done with >>>>>>>> another action which is (IMHO unfortunately) named "sample". >>>>>>>> >>>>>>>> With that interpretation of the term it would actually make more sense >>>>>>>> to rename "sample" to something like "random" (of course I'm not >>>>>>>> suggestion we do it). "sample" without any nested action that actually >>>>>>>> sends the packet somewhere is not sampling, it's just doing something or >>>>>>>> not based on a probability. Where as "emit_sample" is sampling even if >>>>>>>> it's not nested inside a "sample". >>>>>>> >>>>>>> You're assuming we are extracting a packet for sampling, but this function >>>>>>> can be used for various other purposes. For instance, it could handle the >>>>>>> packet outside of the OVS pipeline through an eBPF program (so we are not >>>>>>> taking a sample, but continue packet processing outside of the OVS >>>>>>> pipeline). Calling it emit_sampling() in such cases could be very >>>>>>> confusing. >>>>> >>>>> We can't change the implementation of the action once it is part of uAPI. >>>>> We have to document where users can find these packets and we can't just >>>>> change the destination later. >>>> >>>> I'm not suggesting we change the uAPI implementation, but we could use the >>>> emit_xxx() action with an eBPF probe on the action to perform other tasks. >>>> This is just an example. >>> >>> Yeah, but as Adrian said below, you could do that with any action and >>> this doesn't change the semantics of the action itself. >> >> Well this was just an example, what if we have some other need for getting >> a packet to userspace through emit_local() other than sampling? The >> emit_sample() action naming in this case makes no sense. >> >>>>>> Well, I guess that would be clearly abusing the action. You could say >>>>>> that of anything really. You could hook into skb_consume and continue >>>>>> processing the skb but that doesn't change the intended behavior of the >>>>>> drop action. >>>>>> >>>>>> The intended behavior of the action is sampling, as is the intended >>>>>> behavior of "psample". >>>>> >>>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >>>>> that is it takes some packets from the whole packet stream and executes >>>>> actions of them. Without tying this to observability purposes the name >>>>> makes sense as the first definition of the word is "to take a representative >>>>> part or a single item from a larger whole or group". >>>>> >>>>> Now, our new action doesn't have this particular semantic in a way that >>>>> it doesn't take a part of a whole packet stream but rather using the >>>>> part already taken. However, it is directly tied to the parent >>>>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >>>>> action. If there is no parent, then probability is assumed to be 100%, >>>>> but that's just a corner case. The name of a psample module has the >>>>> same semantics in its name, it doesn't sample on it's own, but it is >>>>> assuming that sampling was performed as it relays the rate of it. >>>>> >>>>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >>>>> the psample module, the emit_sample() name makes sense to me. >>>> >>>> This is the part I don't like. emit_sample() should be treated as a >>>> standalone action. While it may have potential dependencies on >>>> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it >>>> independently. >>> >>> It is fine to use it, we just assume implicit 100% sampling. >> >> Agreed, but the name does not make sense ;) I do not think we >> currently have any actions that explicitly depend on each other >> (there might be attributes carried over) and I want to keep it >> as such. >> >>>>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>>>>>> OK with "emit_local" or "emit_packet" or even just "emit". >>>>> >>>>> The 'local' or 'packet' variants are not descriptive enough on what we're >>>>> trying to achieve and do not explain why the probability is attached to >>>>> the action, i.e. do not explain the link between this action and the >>>>> OVS_ACTION_ATTR_SAMPLE. >>>>> >>>>> emit_Psample() would be overly specific, I agree, but making the name too >>>>> generic will also make it hard to add new actions. If we use some overly >>>>> broad term for this one, we may have to deal with overlapping semantics in >>>>> the future. >>>>> >>>>>>>> I don't think any term will fully satisfy everyone so I hope we can find >>>>>>>> a reasonable compromise. >>>>>>> >>>>>>> My preference would be emit_local() as we hand it off to some local >>>>>>> datapath entity. >>>>> >>>>> What is "local datapath entity" ? psample module is not part of OVS datapath. >>>>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented >>>>> by a bridge port on a datapath level, that will be another source of confusion >>>>> as it can be interpreted as sending a packet via a local bridge port. >>>> >>>> I guess I hinted at a local exit point in the specific netdev/netlink datapath, >>>> where exit is to the local host. So maybe we should call it emit_localhost? >>> >>> For me sending to localhost means sending to a loopback interface or otherwise >>> sending the packet to the host networking stack. And we're not doing that. >> >> That might be confusing too... Maybe emit_external()? > > "External" was the word I used for the original userspace RFC. The > rationale being: We're sending the packet to something external from OVS > (datapath or userspace). Compared with IPFIX-based observability which > where the sample is first processed ("internally") by ovs-vswitchd. > > In userspace it kept the sampling/observability meaning because it was > part of the Flow_Sample_Collector_Set which is intrinsically an > observability thing. > > However, in the datapath we loose that meaning and could be confused > with some external packet-processing entity. How about "external_observe" > or something that somehow keeps that meaning? This semantics conversation doesn't seem productive as we're going in circles around what we already discussed what feels like at least three separate times on this and ovs-dev lists. I'd say if we can't agree on OVS_ACTION_ATTR_EMIT_SAMPLE, then just call it OVS_ACTION_ATTR_SEND_TO_PSAMPLE. Simple, describes exactly what it does. And if we ever want to have "local" sampling for OVS userspace datapath, we can create a userspace-only datapath action for it and call it in a way that describes what it does, e.g. OVS_ACTION_ATTR_SEND_TO_USDT or whatever. Unlike key attributes, we can relatively safely create userspace-only actions without consequences for kernel uAPI. In fact, we have a few such actions. And we can choose which one to use based on which one is supported by the current datapath. Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 6/27/24 12:15, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >>> >>>> On 6/27/24 11:14, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >>>>> >>>>>> On 6/27/24 09:52, Adrián Moreno wrote: >>>>>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>>>>>>> >>>>>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 25 Jun 2024, at 22:51, 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. >>>>>>>>>> >>>>>>>>>> I’ll add the same comment as I had in the user space part, and that >>>>>>>>>> is that I feel from an OVS perspective this action should be called >>>>>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>>>>>>> Or quoting the earlier comment: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> “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.” >>>>>>>>>> >>>>>>>>> >>>>>>>>> This is a blurry semantic area. >>>>>>>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>>>>>>> someting, in this case, a packet. It is common to only take some packets >>>>>>>>> as samples, so this action usually comes with some kind of "rate", but >>>>>>>>> even if the rate is 1, it's still sampling in this context. >>>>>>>>> >>>>>>>>> OTOH, OVS kernel design tries to be super-modular and define small >>>>>>>>> combinable actions, so the rate or probability generation is done with >>>>>>>>> another action which is (IMHO unfortunately) named "sample". >>>>>>>>> >>>>>>>>> With that interpretation of the term it would actually make more sense >>>>>>>>> to rename "sample" to something like "random" (of course I'm not >>>>>>>>> suggestion we do it). "sample" without any nested action that actually >>>>>>>>> sends the packet somewhere is not sampling, it's just doing something or >>>>>>>>> not based on a probability. Where as "emit_sample" is sampling even if >>>>>>>>> it's not nested inside a "sample". >>>>>>>> >>>>>>>> You're assuming we are extracting a packet for sampling, but this function >>>>>>>> can be used for various other purposes. For instance, it could handle the >>>>>>>> packet outside of the OVS pipeline through an eBPF program (so we are not >>>>>>>> taking a sample, but continue packet processing outside of the OVS >>>>>>>> pipeline). Calling it emit_sampling() in such cases could be very >>>>>>>> confusing. >>>>>> >>>>>> We can't change the implementation of the action once it is part of uAPI. >>>>>> We have to document where users can find these packets and we can't just >>>>>> change the destination later. >>>>> >>>>> I'm not suggesting we change the uAPI implementation, but we could use the >>>>> emit_xxx() action with an eBPF probe on the action to perform other tasks. >>>>> This is just an example. >>>> >>>> Yeah, but as Adrian said below, you could do that with any action and >>>> this doesn't change the semantics of the action itself. >>> >>> Well this was just an example, what if we have some other need for getting >>> a packet to userspace through emit_local() other than sampling? The >>> emit_sample() action naming in this case makes no sense. >>> >>>>>>> Well, I guess that would be clearly abusing the action. You could say >>>>>>> that of anything really. You could hook into skb_consume and continue >>>>>>> processing the skb but that doesn't change the intended behavior of the >>>>>>> drop action. >>>>>>> >>>>>>> The intended behavior of the action is sampling, as is the intended >>>>>>> behavior of "psample". >>>>>> >>>>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >>>>>> that is it takes some packets from the whole packet stream and executes >>>>>> actions of them. Without tying this to observability purposes the name >>>>>> makes sense as the first definition of the word is "to take a representative >>>>>> part or a single item from a larger whole or group". >>>>>> >>>>>> Now, our new action doesn't have this particular semantic in a way that >>>>>> it doesn't take a part of a whole packet stream but rather using the >>>>>> part already taken. However, it is directly tied to the parent >>>>>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >>>>>> action. If there is no parent, then probability is assumed to be 100%, >>>>>> but that's just a corner case. The name of a psample module has the >>>>>> same semantics in its name, it doesn't sample on it's own, but it is >>>>>> assuming that sampling was performed as it relays the rate of it. >>>>>> >>>>>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >>>>>> the psample module, the emit_sample() name makes sense to me. >>>>> >>>>> This is the part I don't like. emit_sample() should be treated as a >>>>> standalone action. While it may have potential dependencies on >>>>> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it >>>>> independently. >>>> >>>> It is fine to use it, we just assume implicit 100% sampling. >>> >>> Agreed, but the name does not make sense ;) I do not think we >>> currently have any actions that explicitly depend on each other >>> (there might be attributes carried over) and I want to keep it >>> as such. >>> >>>>>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>>>>>>> OK with "emit_local" or "emit_packet" or even just "emit". >>>>>> >>>>>> The 'local' or 'packet' variants are not descriptive enough on what we're >>>>>> trying to achieve and do not explain why the probability is attached to >>>>>> the action, i.e. do not explain the link between this action and the >>>>>> OVS_ACTION_ATTR_SAMPLE. >>>>>> >>>>>> emit_Psample() would be overly specific, I agree, but making the name too >>>>>> generic will also make it hard to add new actions. If we use some overly >>>>>> broad term for this one, we may have to deal with overlapping semantics in >>>>>> the future. >>>>>> >>>>>>>>> I don't think any term will fully satisfy everyone so I hope we can find >>>>>>>>> a reasonable compromise. >>>>>>>> >>>>>>>> My preference would be emit_local() as we hand it off to some local >>>>>>>> datapath entity. >>>>>> >>>>>> What is "local datapath entity" ? psample module is not part of OVS datapath. >>>>>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented >>>>>> by a bridge port on a datapath level, that will be another source of confusion >>>>>> as it can be interpreted as sending a packet via a local bridge port. >>>>> >>>>> I guess I hinted at a local exit point in the specific netdev/netlink datapath, >>>>> where exit is to the local host. So maybe we should call it emit_localhost? >>>> >>>> For me sending to localhost means sending to a loopback interface or otherwise >>>> sending the packet to the host networking stack. And we're not doing that. >>> >>> That might be confusing too... Maybe emit_external()? >> >> "External" was the word I used for the original userspace RFC. The >> rationale being: We're sending the packet to something external from OVS >> (datapath or userspace). Compared with IPFIX-based observability which >> where the sample is first processed ("internally") by ovs-vswitchd. >> >> In userspace it kept the sampling/observability meaning because it was >> part of the Flow_Sample_Collector_Set which is intrinsically an >> observability thing. >> >> However, in the datapath we loose that meaning and could be confused >> with some external packet-processing entity. How about "external_observe" >> or something that somehow keeps that meaning? > > This semantics conversation doesn't seem productive as we're going in circles > around what we already discussed what feels like at least three separate times > on this and ovs-dev lists. +1 > I'd say if we can't agree on OVS_ACTION_ATTR_EMIT_SAMPLE, then just call > it OVS_ACTION_ATTR_SEND_TO_PSAMPLE. Simple, describes exactly what it does. > And if we ever want to have "local" sampling for OVS userspace datapath, > we can create a userspace-only datapath action for it and call it in a way > that describes what it does, e.g. OVS_ACTION_ATTR_SEND_TO_USDT or whatever. > Unlike key attributes, we can relatively safely create userspace-only actions > without consequences for kernel uAPI. In fact, we have a few such actions. > And we can choose which one to use based on which one is supported by the > current datapath. I'm okay with the emit_sample or with send_to_psample. There are probably hundreds of colors to paint this shed, tbh. We could argue that it could even be an extension to userspace() instead of a separate action, or that we could have a generic socket_write(type=psample) type of action. But in the end, I don't have a strong feeling either way, whether it's: OVS_ACTION_ATTR_EMIT_SAMPLE / emit_sample() OVS_ACTION_ATTR_SEND_TO_PSAMPLE / psample() or emit_psample() OVS_ACTION_ATTR_EMIT_EXTERNAL / emit_external() There aren't really too many differences in them, and it wouldn't bother me in any case. I guess a XXX?psample() action does end up being the clearest since it has 'psample' right in the name and then we can know right away what it is doing. > Best regards, Ilya Maximets.
On 27 Jun 2024, at 12:52, Ilya Maximets wrote: > On 6/27/24 12:15, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >>> >>>> On 6/27/24 11:14, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >>>>> >>>>>> On 6/27/24 09:52, Adrián Moreno wrote: >>>>>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>>>>>>> >>>>>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 25 Jun 2024, at 22:51, 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. >>>>>>>>>> >>>>>>>>>> I’ll add the same comment as I had in the user space part, and that >>>>>>>>>> is that I feel from an OVS perspective this action should be called >>>>>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. >>>>>>>>>> Or quoting the earlier comment: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> “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.” >>>>>>>>>> >>>>>>>>> >>>>>>>>> This is a blurry semantic area. >>>>>>>>> IMO, "sample" is the act of extracting (potentially a piece of) >>>>>>>>> someting, in this case, a packet. It is common to only take some packets >>>>>>>>> as samples, so this action usually comes with some kind of "rate", but >>>>>>>>> even if the rate is 1, it's still sampling in this context. >>>>>>>>> >>>>>>>>> OTOH, OVS kernel design tries to be super-modular and define small >>>>>>>>> combinable actions, so the rate or probability generation is done with >>>>>>>>> another action which is (IMHO unfortunately) named "sample". >>>>>>>>> >>>>>>>>> With that interpretation of the term it would actually make more sense >>>>>>>>> to rename "sample" to something like "random" (of course I'm not >>>>>>>>> suggestion we do it). "sample" without any nested action that actually >>>>>>>>> sends the packet somewhere is not sampling, it's just doing something or >>>>>>>>> not based on a probability. Where as "emit_sample" is sampling even if >>>>>>>>> it's not nested inside a "sample". >>>>>>>> >>>>>>>> You're assuming we are extracting a packet for sampling, but this function >>>>>>>> can be used for various other purposes. For instance, it could handle the >>>>>>>> packet outside of the OVS pipeline through an eBPF program (so we are not >>>>>>>> taking a sample, but continue packet processing outside of the OVS >>>>>>>> pipeline). Calling it emit_sampling() in such cases could be very >>>>>>>> confusing. >>>>>> >>>>>> We can't change the implementation of the action once it is part of uAPI. >>>>>> We have to document where users can find these packets and we can't just >>>>>> change the destination later. >>>>> >>>>> I'm not suggesting we change the uAPI implementation, but we could use the >>>>> emit_xxx() action with an eBPF probe on the action to perform other tasks. >>>>> This is just an example. >>>> >>>> Yeah, but as Adrian said below, you could do that with any action and >>>> this doesn't change the semantics of the action itself. >>> >>> Well this was just an example, what if we have some other need for getting >>> a packet to userspace through emit_local() other than sampling? The >>> emit_sample() action naming in this case makes no sense. >>> >>>>>>> Well, I guess that would be clearly abusing the action. You could say >>>>>>> that of anything really. You could hook into skb_consume and continue >>>>>>> processing the skb but that doesn't change the intended behavior of the >>>>>>> drop action. >>>>>>> >>>>>>> The intended behavior of the action is sampling, as is the intended >>>>>>> behavior of "psample". >>>>>> >>>>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >>>>>> that is it takes some packets from the whole packet stream and executes >>>>>> actions of them. Without tying this to observability purposes the name >>>>>> makes sense as the first definition of the word is "to take a representative >>>>>> part or a single item from a larger whole or group". >>>>>> >>>>>> Now, our new action doesn't have this particular semantic in a way that >>>>>> it doesn't take a part of a whole packet stream but rather using the >>>>>> part already taken. However, it is directly tied to the parent >>>>>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >>>>>> action. If there is no parent, then probability is assumed to be 100%, >>>>>> but that's just a corner case. The name of a psample module has the >>>>>> same semantics in its name, it doesn't sample on it's own, but it is >>>>>> assuming that sampling was performed as it relays the rate of it. >>>>>> >>>>>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >>>>>> the psample module, the emit_sample() name makes sense to me. >>>>> >>>>> This is the part I don't like. emit_sample() should be treated as a >>>>> standalone action. While it may have potential dependencies on >>>>> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it >>>>> independently. >>>> >>>> It is fine to use it, we just assume implicit 100% sampling. >>> >>> Agreed, but the name does not make sense ;) I do not think we >>> currently have any actions that explicitly depend on each other >>> (there might be attributes carried over) and I want to keep it >>> as such. >>> >>>>>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>>>>>>>> OK with "emit_local" or "emit_packet" or even just "emit". >>>>>> >>>>>> The 'local' or 'packet' variants are not descriptive enough on what we're >>>>>> trying to achieve and do not explain why the probability is attached to >>>>>> the action, i.e. do not explain the link between this action and the >>>>>> OVS_ACTION_ATTR_SAMPLE. >>>>>> >>>>>> emit_Psample() would be overly specific, I agree, but making the name too >>>>>> generic will also make it hard to add new actions. If we use some overly >>>>>> broad term for this one, we may have to deal with overlapping semantics in >>>>>> the future. >>>>>> >>>>>>>>> I don't think any term will fully satisfy everyone so I hope we can find >>>>>>>>> a reasonable compromise. >>>>>>>> >>>>>>>> My preference would be emit_local() as we hand it off to some local >>>>>>>> datapath entity. >>>>>> >>>>>> What is "local datapath entity" ? psample module is not part of OVS datapath. >>>>>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented >>>>>> by a bridge port on a datapath level, that will be another source of confusion >>>>>> as it can be interpreted as sending a packet via a local bridge port. >>>>> >>>>> I guess I hinted at a local exit point in the specific netdev/netlink datapath, >>>>> where exit is to the local host. So maybe we should call it emit_localhost? >>>> >>>> For me sending to localhost means sending to a loopback interface or otherwise >>>> sending the packet to the host networking stack. And we're not doing that. >>> >>> That might be confusing too... Maybe emit_external()? >> >> "External" was the word I used for the original userspace RFC. The >> rationale being: We're sending the packet to something external from OVS >> (datapath or userspace). Compared with IPFIX-based observability which >> where the sample is first processed ("internally") by ovs-vswitchd. >> >> In userspace it kept the sampling/observability meaning because it was >> part of the Flow_Sample_Collector_Set which is intrinsically an >> observability thing. >> >> However, in the datapath we loose that meaning and could be confused >> with some external packet-processing entity. How about "external_observe" >> or something that somehow keeps that meaning? > > This semantics conversation doesn't seem productive as we're going in circles > around what we already discussed what feels like at least three separate times > on this and ovs-dev lists. > > I'd say if we can't agree on OVS_ACTION_ATTR_EMIT_SAMPLE, then just call > it OVS_ACTION_ATTR_SEND_TO_PSAMPLE. Simple, describes exactly what it does. > And if we ever want to have "local" sampling for OVS userspace datapath, > we can create a userspace-only datapath action for it and call it in a way > that describes what it does, e.g. OVS_ACTION_ATTR_SEND_TO_USDT or whatever. > Unlike key attributes, we can relatively safely create userspace-only actions > without consequences for kernel uAPI. In fact, we have a few such actions. > And we can choose which one to use based on which one is supported by the > current datapath. My goal was to avoid adding more actions, but this proposal does the opposite. We need a name that represents the actual action’s action: getting a packet quickly from the kernel (data path) to userspace with some meta-data. Considering the actual purpose of the action, what would be a good name (disregarding that the packet might not be related to sampling)? emit_userspace() Maybe there are other people on the list with better naming skills that want to chime in :) //Eelco
On Thu, Jun 27, 2024 at 09:30:08AM GMT, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > > > On 6/27/24 12:15, Adrián Moreno wrote: > >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: > >>> > >>> > >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > >>> > >>>> On 6/27/24 11:14, Eelco Chaudron wrote: > >>>>> > >>>>> > >>>>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >>>>> > >>>>>> On 6/27/24 09:52, Adrián Moreno wrote: > >>>>>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > >>>>>>>> > >>>>>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 25 Jun 2024, at 22:51, 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. > >>>>>>>>>> > >>>>>>>>>> I’ll add the same comment as I had in the user space part, and that > >>>>>>>>>> is that I feel from an OVS perspective this action should be called > >>>>>>>>>> emit_local() instead of emit_sample() to make it Datapath independent. > >>>>>>>>>> Or quoting the earlier comment: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> “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.” > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> This is a blurry semantic area. > >>>>>>>>> IMO, "sample" is the act of extracting (potentially a piece of) > >>>>>>>>> someting, in this case, a packet. It is common to only take some packets > >>>>>>>>> as samples, so this action usually comes with some kind of "rate", but > >>>>>>>>> even if the rate is 1, it's still sampling in this context. > >>>>>>>>> > >>>>>>>>> OTOH, OVS kernel design tries to be super-modular and define small > >>>>>>>>> combinable actions, so the rate or probability generation is done with > >>>>>>>>> another action which is (IMHO unfortunately) named "sample". > >>>>>>>>> > >>>>>>>>> With that interpretation of the term it would actually make more sense > >>>>>>>>> to rename "sample" to something like "random" (of course I'm not > >>>>>>>>> suggestion we do it). "sample" without any nested action that actually > >>>>>>>>> sends the packet somewhere is not sampling, it's just doing something or > >>>>>>>>> not based on a probability. Where as "emit_sample" is sampling even if > >>>>>>>>> it's not nested inside a "sample". > >>>>>>>> > >>>>>>>> You're assuming we are extracting a packet for sampling, but this function > >>>>>>>> can be used for various other purposes. For instance, it could handle the > >>>>>>>> packet outside of the OVS pipeline through an eBPF program (so we are not > >>>>>>>> taking a sample, but continue packet processing outside of the OVS > >>>>>>>> pipeline). Calling it emit_sampling() in such cases could be very > >>>>>>>> confusing. > >>>>>> > >>>>>> We can't change the implementation of the action once it is part of uAPI. > >>>>>> We have to document where users can find these packets and we can't just > >>>>>> change the destination later. > >>>>> > >>>>> I'm not suggesting we change the uAPI implementation, but we could use the > >>>>> emit_xxx() action with an eBPF probe on the action to perform other tasks. > >>>>> This is just an example. > >>>> > >>>> Yeah, but as Adrian said below, you could do that with any action and > >>>> this doesn't change the semantics of the action itself. > >>> > >>> Well this was just an example, what if we have some other need for getting > >>> a packet to userspace through emit_local() other than sampling? The > >>> emit_sample() action naming in this case makes no sense. > >>> > >>>>>>> Well, I guess that would be clearly abusing the action. You could say > >>>>>>> that of anything really. You could hook into skb_consume and continue > >>>>>>> processing the skb but that doesn't change the intended behavior of the > >>>>>>> drop action. > >>>>>>> > >>>>>>> The intended behavior of the action is sampling, as is the intended > >>>>>>> behavior of "psample". > >>>>>> > >>>>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > >>>>>> that is it takes some packets from the whole packet stream and executes > >>>>>> actions of them. Without tying this to observability purposes the name > >>>>>> makes sense as the first definition of the word is "to take a representative > >>>>>> part or a single item from a larger whole or group". > >>>>>> > >>>>>> Now, our new action doesn't have this particular semantic in a way that > >>>>>> it doesn't take a part of a whole packet stream but rather using the > >>>>>> part already taken. However, it is directly tied to the parent > >>>>>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent > >>>>>> action. If there is no parent, then probability is assumed to be 100%, > >>>>>> but that's just a corner case. The name of a psample module has the > >>>>>> same semantics in its name, it doesn't sample on it's own, but it is > >>>>>> assuming that sampling was performed as it relays the rate of it. > >>>>>> > >>>>>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and > >>>>>> the psample module, the emit_sample() name makes sense to me. > >>>>> > >>>>> This is the part I don't like. emit_sample() should be treated as a > >>>>> standalone action. While it may have potential dependencies on > >>>>> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it > >>>>> independently. > >>>> > >>>> It is fine to use it, we just assume implicit 100% sampling. > >>> > >>> Agreed, but the name does not make sense ;) I do not think we > >>> currently have any actions that explicitly depend on each other > >>> (there might be attributes carried over) and I want to keep it > >>> as such. > >>> > >>>>>>>>> Having said that, I don't have a super strong favor for "emit_sample". I'm > >>>>>>>>> OK with "emit_local" or "emit_packet" or even just "emit". > >>>>>> > >>>>>> The 'local' or 'packet' variants are not descriptive enough on what we're > >>>>>> trying to achieve and do not explain why the probability is attached to > >>>>>> the action, i.e. do not explain the link between this action and the > >>>>>> OVS_ACTION_ATTR_SAMPLE. > >>>>>> > >>>>>> emit_Psample() would be overly specific, I agree, but making the name too > >>>>>> generic will also make it hard to add new actions. If we use some overly > >>>>>> broad term for this one, we may have to deal with overlapping semantics in > >>>>>> the future. > >>>>>> > >>>>>>>>> I don't think any term will fully satisfy everyone so I hope we can find > >>>>>>>>> a reasonable compromise. > >>>>>>>> > >>>>>>>> My preference would be emit_local() as we hand it off to some local > >>>>>>>> datapath entity. > >>>>>> > >>>>>> What is "local datapath entity" ? psample module is not part of OVS datapath. > >>>>>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that is represented > >>>>>> by a bridge port on a datapath level, that will be another source of confusion > >>>>>> as it can be interpreted as sending a packet via a local bridge port. > >>>>> > >>>>> I guess I hinted at a local exit point in the specific netdev/netlink datapath, > >>>>> where exit is to the local host. So maybe we should call it emit_localhost? > >>>> > >>>> For me sending to localhost means sending to a loopback interface or otherwise > >>>> sending the packet to the host networking stack. And we're not doing that. > >>> > >>> That might be confusing too... Maybe emit_external()? > >> > >> "External" was the word I used for the original userspace RFC. The > >> rationale being: We're sending the packet to something external from OVS > >> (datapath or userspace). Compared with IPFIX-based observability which > >> where the sample is first processed ("internally") by ovs-vswitchd. > >> > >> In userspace it kept the sampling/observability meaning because it was > >> part of the Flow_Sample_Collector_Set which is intrinsically an > >> observability thing. > >> > >> However, in the datapath we loose that meaning and could be confused > >> with some external packet-processing entity. How about "external_observe" > >> or something that somehow keeps that meaning? > > > > This semantics conversation doesn't seem productive as we're going in circles > > around what we already discussed what feels like at least three separate times > > on this and ovs-dev lists. > > +1 > > > I'd say if we can't agree on OVS_ACTION_ATTR_EMIT_SAMPLE, then just call > > it OVS_ACTION_ATTR_SEND_TO_PSAMPLE. Simple, describes exactly what it does. > > And if we ever want to have "local" sampling for OVS userspace datapath, > > we can create a userspace-only datapath action for it and call it in a way > > that describes what it does, e.g. OVS_ACTION_ATTR_SEND_TO_USDT or whatever. > > Unlike key attributes, we can relatively safely create userspace-only actions > > without consequences for kernel uAPI. In fact, we have a few such actions. > > And we can choose which one to use based on which one is supported by the > > current datapath. > > I'm okay with the emit_sample or with send_to_psample. There are > probably hundreds of colors to paint this shed, tbh. We could argue > that it could even be an extension to userspace() instead of a separate > action, or that we could have a generic socket_write(type=psample) type > of action. But in the end, I don't have a strong feeling either way, > whether it's: > > OVS_ACTION_ATTR_EMIT_SAMPLE / emit_sample() > OVS_ACTION_ATTR_SEND_TO_PSAMPLE / psample() or emit_psample() > OVS_ACTION_ATTR_EMIT_EXTERNAL / emit_external() > > There aren't really too many differences in them, and it wouldn't bother > me in any case. I guess a XXX?psample() action does end up being the > clearest since it has 'psample' right in the name and then we can know > right away what it is doing. > The original purpose of the name was to have the same action for both userspace and kernel so that, name aside, the semantics (ACT_XXXX(group=10,cookie=0x123)) remains the same. If we break that, we risk having userspace and kernel actions differ in ways that makes it difficult to unify at the xlate/OpenFlow/OVSDB layers. But if we can enforce that somehow I guess it's OK. Thanks. Adrián
Adrián Moreno <amorenoz@redhat.com> writes: > On Thu, Jun 27, 2024 at 09:30:08AM GMT, Aaron Conole wrote: >> Ilya Maximets <i.maximets@ovn.org> writes: >> >> > On 6/27/24 12:15, Adrián Moreno wrote: >> >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >> >>> >> >>> >> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >> >>> >> >>>> On 6/27/24 11:14, Eelco Chaudron wrote: >> >>>>> >> >>>>> >> >>>>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >> >>>>> >> >>>>>> On 6/27/24 09:52, Adrián Moreno wrote: >> >>>>>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >> >>>>>>>> >> >>>>>>>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> On 25 Jun 2024, at 22:51, 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. >> >>>>>>>>>> >> >>>>>>>>>> I’ll add the same comment as I had in the user space part, and that >> >>>>>>>>>> is that I feel from an OVS perspective this action should be called >> >>>>>>>>>> emit_local() instead of emit_sample() to make it Datapath >> >>>>>>>>>> independent. >> >>>>>>>>>> Or quoting the earlier comment: >> >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> “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.” >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> This is a blurry semantic area. >> >>>>>>>>> IMO, "sample" is the act of extracting (potentially a piece of) >> >>>>>>>>> someting, in this case, a packet. It is common to only >> >>>>>>>>> take some packets >> >>>>>>>>> as samples, so this action usually comes with some kind of >> >>>>>>>>> "rate", but >> >>>>>>>>> even if the rate is 1, it's still sampling in this context. >> >>>>>>>>> >> >>>>>>>>> OTOH, OVS kernel design tries to be super-modular and define small >> >>>>>>>>> combinable actions, so the rate or probability generation >> >>>>>>>>> is done with >> >>>>>>>>> another action which is (IMHO unfortunately) named "sample". >> >>>>>>>>> >> >>>>>>>>> With that interpretation of the term it would actually >> >>>>>>>>> make more sense >> >>>>>>>>> to rename "sample" to something like "random" (of course I'm not >> >>>>>>>>> suggestion we do it). "sample" without any nested action >> >>>>>>>>> that actually >> >>>>>>>>> sends the packet somewhere is not sampling, it's just >> >>>>>>>>> doing something or >> >>>>>>>>> not based on a probability. Where as "emit_sample" is >> >>>>>>>>> sampling even if >> >>>>>>>>> it's not nested inside a "sample". >> >>>>>>>> >> >>>>>>>> You're assuming we are extracting a packet for sampling, >> >>>>>>>> but this function >> >>>>>>>> can be used for various other purposes. For instance, it >> >>>>>>>> could handle the >> >>>>>>>> packet outside of the OVS pipeline through an eBPF program >> >>>>>>>> (so we are not >> >>>>>>>> taking a sample, but continue packet processing outside of the OVS >> >>>>>>>> pipeline). Calling it emit_sampling() in such cases could be very >> >>>>>>>> confusing. >> >>>>>> >> >>>>>> We can't change the implementation of the action once it is >> >>>>>> part of uAPI. >> >>>>>> We have to document where users can find these packets and we >> >>>>>> can't just >> >>>>>> change the destination later. >> >>>>> >> >>>>> I'm not suggesting we change the uAPI implementation, but we >> >>>>> could use the >> >>>>> emit_xxx() action with an eBPF probe on the action to perform >> >>>>> other tasks. >> >>>>> This is just an example. >> >>>> >> >>>> Yeah, but as Adrian said below, you could do that with any action and >> >>>> this doesn't change the semantics of the action itself. >> >>> >> >>> Well this was just an example, what if we have some other need for getting >> >>> a packet to userspace through emit_local() other than sampling? The >> >>> emit_sample() action naming in this case makes no sense. >> >>> >> >>>>>>> Well, I guess that would be clearly abusing the action. You could say >> >>>>>>> that of anything really. You could hook into skb_consume and continue >> >>>>>>> processing the skb but that doesn't change the intended >> >>>>>>> behavior of the >> >>>>>>> drop action. >> >>>>>>> >> >>>>>>> The intended behavior of the action is sampling, as is the intended >> >>>>>>> behavior of "psample". >> >>>>>> >> >>>>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically >> >>>>>> executes actions", >> >>>>>> that is it takes some packets from the whole packet stream and executes >> >>>>>> actions of them. Without tying this to observability purposes the name >> >>>>>> makes sense as the first definition of the word is "to take a >> >>>>>> representative >> >>>>>> part or a single item from a larger whole or group". >> >>>>>> >> >>>>>> Now, our new action doesn't have this particular semantic in a way that >> >>>>>> it doesn't take a part of a whole packet stream but rather using the >> >>>>>> part already taken. However, it is directly tied to the parent >> >>>>>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability >> >>>>>> of that parent >> >>>>>> action. If there is no parent, then probability is assumed to be 100%, >> >>>>>> but that's just a corner case. The name of a psample module has the >> >>>>>> same semantics in its name, it doesn't sample on it's own, but it is >> >>>>>> assuming that sampling was performed as it relays the rate of it. >> >>>>>> >> >>>>>> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and >> >>>>>> the psample module, the emit_sample() name makes sense to me. >> >>>>> >> >>>>> This is the part I don't like. emit_sample() should be treated as a >> >>>>> standalone action. While it may have potential dependencies on >> >>>>> OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it >> >>>>> independently. >> >>>> >> >>>> It is fine to use it, we just assume implicit 100% sampling. >> >>> >> >>> Agreed, but the name does not make sense ;) I do not think we >> >>> currently have any actions that explicitly depend on each other >> >>> (there might be attributes carried over) and I want to keep it >> >>> as such. >> >>> >> >>>>>>>>> Having said that, I don't have a super strong favor for >> >>>>>>>>> "emit_sample". I'm >> >>>>>>>>> OK with "emit_local" or "emit_packet" or even just "emit". >> >>>>>> >> >>>>>> The 'local' or 'packet' variants are not descriptive enough >> >>>>>> on what we're >> >>>>>> trying to achieve and do not explain why the probability is attached to >> >>>>>> the action, i.e. do not explain the link between this action and the >> >>>>>> OVS_ACTION_ATTR_SAMPLE. >> >>>>>> >> >>>>>> emit_Psample() would be overly specific, I agree, but making >> >>>>>> the name too >> >>>>>> generic will also make it hard to add new actions. If we use >> >>>>>> some overly >> >>>>>> broad term for this one, we may have to deal with overlapping >> >>>>>> semantics in >> >>>>>> the future. >> >>>>>> >> >>>>>>>>> I don't think any term will fully satisfy everyone so I >> >>>>>>>>> hope we can find >> >>>>>>>>> a reasonable compromise. >> >>>>>>>> >> >>>>>>>> My preference would be emit_local() as we hand it off to some local >> >>>>>>>> datapath entity. >> >>>>>> >> >>>>>> What is "local datapath entity" ? psample module is not part >> >>>>>> of OVS datapath. >> >>>>>> And what is "local" ? OpenFlow has the OFPP_LOCAL port that >> >>>>>> is represented >> >>>>>> by a bridge port on a datapath level, that will be another >> >>>>>> source of confusion >> >>>>>> as it can be interpreted as sending a packet via a local bridge port. >> >>>>> >> >>>>> I guess I hinted at a local exit point in the specific >> >>>>> netdev/netlink datapath, >> >>>>> where exit is to the local host. So maybe we should call it >> >>>>> emit_localhost? >> >>>> >> >>>> For me sending to localhost means sending to a loopback >> >>>> interface or otherwise >> >>>> sending the packet to the host networking stack. And we're not >> >>>> doing that. >> >>> >> >>> That might be confusing too... Maybe emit_external()? >> >> >> >> "External" was the word I used for the original userspace RFC. The >> >> rationale being: We're sending the packet to something external from OVS >> >> (datapath or userspace). Compared with IPFIX-based observability which >> >> where the sample is first processed ("internally") by ovs-vswitchd. >> >> >> >> In userspace it kept the sampling/observability meaning because it was >> >> part of the Flow_Sample_Collector_Set which is intrinsically an >> >> observability thing. >> >> >> >> However, in the datapath we loose that meaning and could be confused >> >> with some external packet-processing entity. How about "external_observe" >> >> or something that somehow keeps that meaning? >> > >> > This semantics conversation doesn't seem productive as we're going >> > in circles >> > around what we already discussed what feels like at least three >> > separate times >> > on this and ovs-dev lists. >> >> +1 >> >> > I'd say if we can't agree on OVS_ACTION_ATTR_EMIT_SAMPLE, then just call >> > it OVS_ACTION_ATTR_SEND_TO_PSAMPLE. Simple, describes exactly what it does. >> > And if we ever want to have "local" sampling for OVS userspace datapath, >> > we can create a userspace-only datapath action for it and call it in a way >> > that describes what it does, e.g. OVS_ACTION_ATTR_SEND_TO_USDT or whatever. >> > Unlike key attributes, we can relatively safely create >> > userspace-only actions >> > without consequences for kernel uAPI. In fact, we have a few such actions. >> > And we can choose which one to use based on which one is supported by the >> > current datapath. >> >> I'm okay with the emit_sample or with send_to_psample. There are >> probably hundreds of colors to paint this shed, tbh. We could argue >> that it could even be an extension to userspace() instead of a separate >> action, or that we could have a generic socket_write(type=psample) type >> of action. But in the end, I don't have a strong feeling either way, >> whether it's: >> >> OVS_ACTION_ATTR_EMIT_SAMPLE / emit_sample() >> OVS_ACTION_ATTR_SEND_TO_PSAMPLE / psample() or emit_psample() >> OVS_ACTION_ATTR_EMIT_EXTERNAL / emit_external() >> >> There aren't really too many differences in them, and it wouldn't bother >> me in any case. I guess a XXX?psample() action does end up being the >> clearest since it has 'psample' right in the name and then we can know >> right away what it is doing. >> > > The original purpose of the name was to have the same action for both > userspace and kernel so that, name aside, the semantics > (ACT_XXXX(group=10,cookie=0x123)) remains the same. If we break that, we > risk having userspace and kernel actions differ in ways that makes it > difficult to unify at the xlate/OpenFlow/OVSDB layers. > > But if we can enforce that somehow I guess it's OK. I think it's less important for that. We do have actions that exist which are datapath specific, so there is precedent. > Thanks. > Adrián
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..8cfa1b3f6b06 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -914,6 +914,31 @@ 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 OVS_EMIT_SAMPLE_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 packet being emitted. + */ +enum ovs_emit_sample_attr { + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */ + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ + + /* private: */ + __OVS_EMIT_SAMPLE_ATTR_MAX +}; + +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) + /** * enum ovs_action_attr - Action types. * @@ -966,6 +991,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_EMIT_SAMPLE: Send a sample of the packet to external + * observers via psample. * * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all * fields within a header are modifiable, e.g. the IPv4 protocol and fragment @@ -1004,6 +1031,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/Kconfig b/net/openvswitch/Kconfig index 29a7081858cd..2535f3f9f462 100644 --- a/net/openvswitch/Kconfig +++ b/net/openvswitch/Kconfig @@ -10,6 +10,7 @@ config OPENVSWITCH (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ (!NF_NAT || NF_NAT) && \ (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT))) + depends on PSAMPLE || !PSAMPLE select LIBCRC32C select MPLS select NET_MPLS_GSO diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 964225580824..1f555cbba312 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,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) return 0; } +static void 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 = {}; + const struct nlattr *a; + int rem; + + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { + 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); + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; + + psample_sample_packet(&psample_group, skb, 0, &md); +#endif +} + /* Execute a list of actions against 'skb'. */ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, struct sw_flow_key *key, @@ -1502,6 +1538,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, ovs_kfree_skb_reason(skb, reason); return 0; } + + case OVS_ACTION_ATTR_EMIT_SAMPLE: + execute_emit_sample(dp, skb, key, a); + OVS_CB(skb)->cutlen = 0; + if (nla_is_last(a, rem)) { + consume_skb(skb); + return 0; + } + break; } if (unlikely(err)) { diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index f224d9bcea5e..29c8cdc44433 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) case OVS_ACTION_ATTR_TRUNC: case OVS_ACTION_ATTR_USERSPACE: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_EMIT_SAMPLE: break; case OVS_ACTION_ATTR_CT: @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len) /* Whenever new actions are added, the need to update this * function should be considered. */ - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24); + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25); if (!actions) return; @@ -3157,6 +3158,29 @@ static int validate_and_copy_check_pkt_len(struct net *net, return 0; } +static int validate_emit_sample(const struct nlattr *attr) +{ + static const struct nla_policy policy[OVS_EMIT_SAMPLE_ATTR_MAX + 1] = { + [OVS_EMIT_SAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, + [OVS_EMIT_SAMPLE_ATTR_COOKIE] = { + .type = NLA_BINARY, + .len = OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE, + }, + }; + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; + int err; + + if (!IS_ENABLED(CONFIG_PSAMPLE)) + return -EOPNOTSUPP; + + err = nla_parse_nested(a, OVS_EMIT_SAMPLE_ATTR_MAX, attr, policy, + NULL); + if (err) + return err; + + return a[OVS_EMIT_SAMPLE_ATTR_GROUP] ? 0 : -EINVAL; +} + static int copy_action(const struct nlattr *from, struct sw_flow_actions **sfa, bool log) { @@ -3212,6 +3236,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls), [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1, [OVS_ACTION_ATTR_DROP] = sizeof(u32), + [OVS_ACTION_ATTR_EMIT_SAMPLE] = (u32)-1, }; const struct ovs_action_push_vlan *vlan; int type = nla_type(a); @@ -3490,6 +3515,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, return -EINVAL; break; + case OVS_ACTION_ATTR_EMIT_SAMPLE: + err = validate_emit_sample(a); + if (err) + return err; + break; + default: OVS_NLERR(log, "Unknown Action type %d", type); return -EINVAL;
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 <amorenoz@redhat.com> --- Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++ include/uapi/linux/openvswitch.h | 28 ++++++++++++++ net/openvswitch/Kconfig | 1 + net/openvswitch/actions.c | 45 +++++++++++++++++++++++ net/openvswitch/flow_netlink.c | 33 ++++++++++++++++- 5 files changed, 123 insertions(+), 1 deletion(-)