| Message ID | 20240630195740.1469727-6-amorenoz@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: openvswitch: Add sample multicasting. | expand |
On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote: > Add support for a new action: psample. > > 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. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > 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 | 47 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > 5 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample > + type: nest > + nested-attributes: psample-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: psample-attrs > + enum-name: ovs-psample-attr > + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE, so this size will be validated correctly. But how likely is that those 2 constants will have different values in the future? Would it be reasonable to create more strict dependency between those macros, e.g.: #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE or, at least, add a comment that the size shouldn't be bigger than TC_COOKIE_MAX_SIZE? I'm just considering the risk of exceeding the array from the patch #2 when somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future. Thanks, Michal
On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote: > On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote: > > Add support for a new action: psample. > > > > 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. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > 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 | 47 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > > 5 files changed, 124 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > > index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample > > + type: nest > > + nested-attributes: psample-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: psample-attrs > > + enum-name: ovs-psample-attr > > + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 > > In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your > cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE, > so this size will be validated correctly. > But how likely is that those 2 constants will have different values in the > future? > Would it be reasonable to create more strict dependency between those > macros, e.g.: > > #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE > > or, at least, add a comment that the size shouldn't be bigger than > TC_COOKIE_MAX_SIZE? > I'm just considering the risk of exceeding the array from the patch #2 when > somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future. > > Thanks, > Michal > Hi Michal, Thanks for sharing your thoughts. I tried to keep the dependency between both cookie sizes loose. I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie and even if it does, backwards compatibility needs to be guaranteed: meaning OVS userspace will have to detect the new size and use it or fall back to a smaller cookie for older kernels. All this needs to be known and worked on in userspace. On the other hand, I intentionally made OVS's "psample" action as similar as possible to act_psample, including setting the same cookie size to begin with. The reason is that I think we should try to implement tc-flower offloading of this action using act_sample, plus 16 seemed a very reasonable max value. When we decide to support offloading the "psample" action, this must be done entirely in userspace. OVS must create a act_sample action (instead of the OVS "psample" one) via netlink. In no circumstances the openvswitch kmod interacts with tc directly. Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and TC_COOKIE_MAX_SIZE does not *and* we already support offloading this action to tc, the only consequence is that OVS userspace has a problem because the tc's netlink interface will reject cookies larger than TC_COOKIE_MAX_SIZE [1]. This guarantees that the array in patch #2 is never overflown. OVS will have to deal with the different sizes and try to squeeze the data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether. Psample does not have a size limit so different parts of the kernel can use psample with different internal max-sizes without any restriction. I hope this clears your concerns. [1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299 Thanks. Adrián
On 6/30/24 21:57, Adrian Moreno wrote: > Add support for a new action: psample. > > 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. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > 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 | 47 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > 5 files changed, 124 insertions(+), 1 deletion(-) Thanks for addressing the comments! The new name also seems reasonable to me. Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Adrian Moreno <amorenoz@redhat.com> writes: > Add support for a new action: psample. > > 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. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- Hi Adrian, Just some nits below. > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > net/openvswitch/Kconfig | 1 + > net/openvswitch/actions.c | 47 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > 5 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample > + type: nest > + nested-attributes: psample-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: psample-attrs > + enum-name: ovs-psample-attr > + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 > +/** > + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE > + * action. > + * > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > + * sample. > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that > + * contains user-defined metadata. The maximum length is > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes. > + * > + * Sends the packet to the psample multicast group with the specified group and > + * cookie. It is possible to combine this action with the > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. > + */ > +enum ovs_psample_attr { > + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */ > + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > + > + /* private: */ > + __OVS_PSAMPLE_ATTR_MAX > +}; > + > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_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_PSAMPLE: 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_PSAMPLE, /* Nested OVS_PSAMPLE_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..a035b7e677dd 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,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +#if IS_ENABLED(CONFIG_PSAMPLE) > +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > + const struct nlattr *attr) > +{ > + 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_PSAMPLE_ATTR_GROUP: > + psample_group.group_num = nla_get_u32(a); > + break; > + > + case OVS_PSAMPLE_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); > +} > +#else > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > + const struct nlattr *attr) {} I noticed that this got flagged in patchwork since it is 'static inline' while being part of a complete translation unit - but I also see some other places where that has been done. I guess it should be just 'static' though. I don't feel very strongly about it. > +#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 +1540,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_PSAMPLE: > + execute_psample(dp, skb, a); > + OVS_CB(skb)->cutlen = 0; We may want to document that trunc is also impacted by psample calls. Right now, it is only mentioned for a single OUTPUT action. Alternatively, we could either ignore trunc, or not reset here. > + 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..c92bdc4dfe19 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_PSAMPLE: > 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,28 @@ static int validate_and_copy_check_pkt_len(struct net *net, > return 0; > } > > +static int validate_psample(const struct nlattr *attr) > +{ > + static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = { > + [OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > + [OVS_PSAMPLE_ATTR_COOKIE] = { > + .type = NLA_BINARY, > + .len = OVS_PSAMPLE_COOKIE_MAX_SIZE, > + }, > + }; > + struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1]; > + int err; > + > + if (!IS_ENABLED(CONFIG_PSAMPLE)) > + return -EOPNOTSUPP; > + > + err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL); > + if (err) > + return err; > + > + return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL; > +} > + > static int copy_action(const struct nlattr *from, > struct sw_flow_actions **sfa, bool log) > { > @@ -3212,6 +3235,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_PSAMPLE] = (u32)-1, > }; > const struct ovs_action_push_vlan *vlan; > int type = nla_type(a); > @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > return -EINVAL; > break; > > + case OVS_ACTION_ATTR_PSAMPLE: > + err = validate_psample(a); > + if (err) > + return err; > + break; > + > default: > OVS_NLERR(log, "Unknown Action type %d", type); > return -EINVAL;
On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > Adrian Moreno <amorenoz@redhat.com> writes: > > > Add support for a new action: psample. > > > > 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. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > --- > > Hi Adrian, > > Just some nits below. > > > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > > net/openvswitch/Kconfig | 1 + > > net/openvswitch/actions.c | 47 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > > 5 files changed, 124 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > > index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample > > + type: nest > > + nested-attributes: psample-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: psample-attrs > > + enum-name: ovs-psample-attr > > + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 > > +/** > > + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE > > + * action. > > + * > > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > > + * sample. > > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that > > + * contains user-defined metadata. The maximum length is > > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes. > > + * > > + * Sends the packet to the psample multicast group with the specified group and > > + * cookie. It is possible to combine this action with the > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. > > + */ > > +enum ovs_psample_attr { > > + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */ > > + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > + > > + /* private: */ > > + __OVS_PSAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_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_PSAMPLE: 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_PSAMPLE, /* Nested OVS_PSAMPLE_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..a035b7e677dd 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,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > return 0; > > } > > > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > > + const struct nlattr *attr) > > +{ > > + 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_PSAMPLE_ATTR_GROUP: > > + psample_group.group_num = nla_get_u32(a); > > + break; > > + > > + case OVS_PSAMPLE_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); > > +} > > +#else > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > > + const struct nlattr *attr) {} > > I noticed that this got flagged in patchwork since it is 'static inline' > while being part of a complete translation unit - but I also see some > other places where that has been done. I guess it should be just > 'static' though. I don't feel very strongly about it. > We had a bit of discussion about this with Ilya. It seems "static inline" is a common pattern around the kernel. The coding style documentation says: "Generally, inline functions are preferable to macros resembling functions." So I think this "inline" is correct but I might be missing something. > > +#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 +1540,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_PSAMPLE: > > + execute_psample(dp, skb, a); > > + OVS_CB(skb)->cutlen = 0; > > We may want to document that trunc is also impacted by psample calls. > Right now, it is only mentioned for a single OUTPUT action. > Alternatively, we could either ignore trunc, or not reset here. The uAPI header says: " Sends the packet to the psample multicast group with the specified group and cookie. It is possible to combine this action with the %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. " Isn't this enough? What else would you add to make it even more clear? > > > + 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..c92bdc4dfe19 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_PSAMPLE: > > 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,28 @@ static int validate_and_copy_check_pkt_len(struct net *net, > > return 0; > > } > > > > +static int validate_psample(const struct nlattr *attr) > > +{ > > + static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = { > > + [OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > > + [OVS_PSAMPLE_ATTR_COOKIE] = { > > + .type = NLA_BINARY, > > + .len = OVS_PSAMPLE_COOKIE_MAX_SIZE, > > + }, > > + }; > > + struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1]; > > + int err; > > + > > + if (!IS_ENABLED(CONFIG_PSAMPLE)) > > + return -EOPNOTSUPP; > > + > > + err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL); > > + if (err) > > + return err; > > + > > + return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL; > > +} > > + > > static int copy_action(const struct nlattr *from, > > struct sw_flow_actions **sfa, bool log) > > { > > @@ -3212,6 +3235,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_PSAMPLE] = (u32)-1, > > }; > > const struct ovs_action_push_vlan *vlan; > > int type = nla_type(a); > > @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > > return -EINVAL; > > break; > > > > + case OVS_ACTION_ATTR_PSAMPLE: > > + err = validate_psample(a); > > + if (err) > > + return err; > > + break; > > + > > default: > > OVS_NLERR(log, "Unknown Action type %d", type); > > return -EINVAL; >
On Tue, Jul 02, 2024 at 03:05:02AM GMT, Adrián Moreno wrote: > On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > > Adrian Moreno <amorenoz@redhat.com> writes: > > > > > Add support for a new action: psample. > > > > > > 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. > > > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > > --- > > > > Hi Adrian, > > > > Just some nits below. > > > > > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > > > include/uapi/linux/openvswitch.h | 28 ++++++++++++++ > > > net/openvswitch/Kconfig | 1 + > > > net/openvswitch/actions.c | 47 +++++++++++++++++++++++ > > > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > > > 5 files changed, 124 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > > > index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample > > > + type: nest > > > + nested-attributes: psample-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: psample-attrs > > > + enum-name: ovs-psample-attr > > > + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 > > > +/** > > > + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE > > > + * action. > > > + * > > > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > > > + * sample. > > > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that > > > + * contains user-defined metadata. The maximum length is > > > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes. > > > + * > > > + * Sends the packet to the psample multicast group with the specified group and > > > + * cookie. It is possible to combine this action with the > > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. > > > + */ > > > +enum ovs_psample_attr { > > > + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */ > > > + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > > + > > > + /* private: */ > > > + __OVS_PSAMPLE_ATTR_MAX > > > +}; > > > + > > > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_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_PSAMPLE: 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_PSAMPLE, /* Nested OVS_PSAMPLE_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..a035b7e677dd 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,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > > return 0; > > > } > > > > > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > + const struct nlattr *attr) > > > +{ > > > + 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_PSAMPLE_ATTR_GROUP: > > > + psample_group.group_num = nla_get_u32(a); > > > + break; > > > + > > > + case OVS_PSAMPLE_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); > > > +} > > > +#else > > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > + const struct nlattr *attr) {} > > > > I noticed that this got flagged in patchwork since it is 'static inline' > > while being part of a complete translation unit - but I also see some > > other places where that has been done. I guess it should be just > > 'static' though. I don't feel very strongly about it. > > > > We had a bit of discussion about this with Ilya. It seems "static > inline" is a common pattern around the kernel. The coding style > documentation says: > "Generally, inline functions are preferable to macros resembling functions." > > So I think this "inline" is correct but I might be missing something. > > > > +#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 +1540,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_PSAMPLE: > > > + execute_psample(dp, skb, a); > > > + OVS_CB(skb)->cutlen = 0; > > > > We may want to document that trunc is also impacted by psample calls. > > Right now, it is only mentioned for a single OUTPUT action. > > Alternatively, we could either ignore trunc, or not reset here. > > The uAPI header says: > > " > Sends the packet to the psample multicast group with the specified group and > cookie. It is possible to combine this action with the > %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. > " > > Isn't this enough? What else would you add to make it even more clear? > BTW trunc also affects userspace and it's not explicitly documented. > > > > > + 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..c92bdc4dfe19 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_PSAMPLE: > > > 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,28 @@ static int validate_and_copy_check_pkt_len(struct net *net, > > > return 0; > > > } > > > > > > +static int validate_psample(const struct nlattr *attr) > > > +{ > > > + static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = { > > > + [OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, > > > + [OVS_PSAMPLE_ATTR_COOKIE] = { > > > + .type = NLA_BINARY, > > > + .len = OVS_PSAMPLE_COOKIE_MAX_SIZE, > > > + }, > > > + }; > > > + struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1]; > > > + int err; > > > + > > > + if (!IS_ENABLED(CONFIG_PSAMPLE)) > > > + return -EOPNOTSUPP; > > > + > > > + err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL); > > > + if (err) > > > + return err; > > > + > > > + return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL; > > > +} > > > + > > > static int copy_action(const struct nlattr *from, > > > struct sw_flow_actions **sfa, bool log) > > > { > > > @@ -3212,6 +3235,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_PSAMPLE] = (u32)-1, > > > }; > > > const struct ovs_action_push_vlan *vlan; > > > int type = nla_type(a); > > > @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > > > return -EINVAL; > > > break; > > > > > > + case OVS_ACTION_ATTR_PSAMPLE: > > > + err = validate_psample(a); > > > + if (err) > > > + return err; > > > + break; > > > + > > > default: > > > OVS_NLERR(log, "Unknown Action type %d", type); > > > return -EINVAL; > >
On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote: > On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > > Adrian Moreno <amorenoz@redhat.com> writes: ... > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c ... > > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > > return 0; > > > } > > > > > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > + const struct nlattr *attr) > > > +{ > > > + 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_PSAMPLE_ATTR_GROUP: > > > + psample_group.group_num = nla_get_u32(a); > > > + break; > > > + > > > + case OVS_PSAMPLE_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); > > > +} > > > +#else > > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > + const struct nlattr *attr) {} > > > > I noticed that this got flagged in patchwork since it is 'static inline' > > while being part of a complete translation unit - but I also see some > > other places where that has been done. I guess it should be just > > 'static' though. I don't feel very strongly about it. > > > > We had a bit of discussion about this with Ilya. It seems "static > inline" is a common pattern around the kernel. The coding style > documentation says: > "Generally, inline functions are preferable to macros resembling functions." > > So I think this "inline" is correct but I might be missing something. Hi Adrián, TL;DR: Please remove this inline keyword For Kernel networking code at least it is strongly preferred not to use inline in .c files unless there is a demonstrable - usually performance - reason to do so. Rather, it is preferred to let the compiler decide when to inline such functions. OTOH, the inline keyword in .h files is fine. ...
On Tue, Jul 02, 2024 at 10:37:26AM GMT, Simon Horman wrote: > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote: > > On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > > > Adrian Moreno <amorenoz@redhat.com> writes: > > ... > > > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > ... > > > > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > > > return 0; > > > > } > > > > > > > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > > + const struct nlattr *attr) > > > > +{ > > > > + 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_PSAMPLE_ATTR_GROUP: > > > > + psample_group.group_num = nla_get_u32(a); > > > > + break; > > > > + > > > > + case OVS_PSAMPLE_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); > > > > +} > > > > +#else > > > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > > > > + const struct nlattr *attr) {} > > > > > > I noticed that this got flagged in patchwork since it is 'static inline' > > > while being part of a complete translation unit - but I also see some > > > other places where that has been done. I guess it should be just > > > 'static' though. I don't feel very strongly about it. > > > > > > > We had a bit of discussion about this with Ilya. It seems "static > > inline" is a common pattern around the kernel. The coding style > > documentation says: > > "Generally, inline functions are preferable to macros resembling functions." > > > > So I think this "inline" is correct but I might be missing something. > > Hi Adrián, > > TL;DR: Please remove this inline keyword > > For Kernel networking code at least it is strongly preferred not > to use inline in .c files unless there is a demonstrable - usually > performance - reason to do so. Rather, it is preferred to let the > compiler decide when to inline such functions. OTOH, the inline > keyword in .h files is fine. > Ok. I'll send a new version. Thanks. Adrián
On 7/2/24 11:37, Simon Horman wrote: > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote: >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: >>> Adrian Moreno <amorenoz@redhat.com> writes: > > ... > >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > ... > >>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >>>> return 0; >>>> } >>>> >>>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb, >>>> + const struct nlattr *attr) >>>> +{ >>>> + 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_PSAMPLE_ATTR_GROUP: >>>> + psample_group.group_num = nla_get_u32(a); >>>> + break; >>>> + >>>> + case OVS_PSAMPLE_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); >>>> +} >>>> +#else >>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, >>>> + const struct nlattr *attr) {} >>> >>> I noticed that this got flagged in patchwork since it is 'static inline' >>> while being part of a complete translation unit - but I also see some >>> other places where that has been done. I guess it should be just >>> 'static' though. I don't feel very strongly about it. >>> >> >> We had a bit of discussion about this with Ilya. It seems "static >> inline" is a common pattern around the kernel. The coding style >> documentation says: >> "Generally, inline functions are preferable to macros resembling functions." >> >> So I think this "inline" is correct but I might be missing something. > > Hi Adrián, > > TL;DR: Please remove this inline keyword > > For Kernel networking code at least it is strongly preferred not > to use inline in .c files unless there is a demonstrable - usually > performance - reason to do so. Rather, it is preferred to let the > compiler decide when to inline such functions. OTOH, the inline > keyword in .h files is fine. FWIW, the main reason for 'inline' here is not performance, but silencing compiler's potential 'maybe unused' warnings: Function-like macros with unused parameters should be replaced by static inline functions to avoid the issue of unused variables I think, the rule for static inline functions in .c files is at odds with the 'Conditional Compilation' section of coding style. The section does recommend to avoid conditional function declaration in .c files, but I'm not sure it is reasonable to export internal static functions for that reason. In this particular case we can either define a macro, which is discouraged by the coding style: Generally, inline functions are preferable to macros resembling functions. Or create a static inline function, that is against rule of no static inline functions in .c files. Or create a simple static function and mark all the arguments as unused, which kind of compliant to the coding style, but the least pretty. Best regards, Ilya Maximets.
On Mon, Jul 01, 2024 at 12:56:43PM +0000, Adrián Moreno wrote: > On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote: > > On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote: > > > Add support for a new action: psample. > > > > > > 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. > > > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > > 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 | 47 +++++++++++++++++++++++ > > > net/openvswitch/flow_netlink.c | 32 ++++++++++++++- > > > 5 files changed, 124 insertions(+), 1 deletion(-) > > > [...] > > > @@ -914,6 +914,31 @@ struct check_pkt_len_arg { > > > }; > > > #endif > > > > > > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16 > > > > In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your > > cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE, > > so this size will be validated correctly. > > But how likely is that those 2 constants will have different values in the > > future? > > Would it be reasonable to create more strict dependency between those > > macros, e.g.: > > > > #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE > > > > or, at least, add a comment that the size shouldn't be bigger than > > TC_COOKIE_MAX_SIZE? > > I'm just considering the risk of exceeding the array from the patch #2 when > > somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future. > > > > Thanks, > > Michal > > > > Hi Michal, > > Thanks for sharing your thoughts. > > I tried to keep the dependency between both cookie sizes loose. > > I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify > OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie > and even if it does, backwards compatibility needs to be guaranteed: > meaning OVS userspace will have to detect the new size and use it or > fall back to a smaller cookie for older kernels. All this needs to be > known and worked on in userspace. > > On the other hand, I intentionally made OVS's "psample" action as > similar as possible to act_psample, including setting the same cookie > size to begin with. The reason is that I think we should try to implement > tc-flower offloading of this action using act_sample, plus 16 seemed a > very reasonable max value. > > When we decide to support offloading the "psample" action, this must > be done entirely in userspace. OVS must create a act_sample action > (instead of the OVS "psample" one) via netlink. In no circumstances the > openvswitch kmod interacts with tc directly. > > Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and > TC_COOKIE_MAX_SIZE does not *and* we already support offloading this > action to tc, the only consequence is that OVS userspace has a > problem because the tc's netlink interface will reject cookies larger > than TC_COOKIE_MAX_SIZE [1]. > This guarantees that the array in patch #2 is never overflown. > > OVS will have to deal with the different sizes and try to squeeze the > data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether. > > Psample does not have a size limit so different parts of the kernel can > use psample with different internal max-sizes without any restriction. > > I hope this clears your concerns. > > [1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299 > > Thanks. > Adrián > Thank you, Adrian, for your detailed explanation. I wasn't aware of the internal validation of that parameter using the mechanism from [1]. Sorry for asking the questions I should have answered by studying the code more carefully. I have no concerns about it now. Thanks, Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote: > On 7/2/24 11:37, Simon Horman wrote: > > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote: > >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > >>> Adrian Moreno <amorenoz@redhat.com> writes: > > > > ... > > > >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > > > ... > > > >>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > >>>> return 0; > >>>> } > >>>> > >>>> +#if IS_ENABLED(CONFIG_PSAMPLE) > >>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > >>>> + const struct nlattr *attr) > >>>> +{ > >>>> + 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_PSAMPLE_ATTR_GROUP: > >>>> + psample_group.group_num = nla_get_u32(a); > >>>> + break; > >>>> + > >>>> + case OVS_PSAMPLE_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); > >>>> +} > >>>> +#else > >>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, > >>>> + const struct nlattr *attr) {} > >>> > >>> I noticed that this got flagged in patchwork since it is 'static inline' > >>> while being part of a complete translation unit - but I also see some > >>> other places where that has been done. I guess it should be just > >>> 'static' though. I don't feel very strongly about it. > >>> > >> > >> We had a bit of discussion about this with Ilya. It seems "static > >> inline" is a common pattern around the kernel. The coding style > >> documentation says: > >> "Generally, inline functions are preferable to macros resembling functions." > >> > >> So I think this "inline" is correct but I might be missing something. > > > > Hi Adrián, > > > > TL;DR: Please remove this inline keyword > > > > For Kernel networking code at least it is strongly preferred not > > to use inline in .c files unless there is a demonstrable - usually > > performance - reason to do so. Rather, it is preferred to let the > > compiler decide when to inline such functions. OTOH, the inline > > keyword in .h files is fine. > > FWIW, the main reason for 'inline' here is not performance, but silencing > compiler's potential 'maybe unused' warnings: > > Function-like macros with unused parameters should be replaced by static > inline functions to avoid the issue of unused variables > > I think, the rule for static inline functions in .c files is at odds with > the 'Conditional Compilation' section of coding style. The section does > recommend to avoid conditional function declaration in .c files, but I'm not > sure it is reasonable to export internal static functions for that reason. > > In this particular case we can either define a macro, which is discouraged > by the coding style: > > Generally, inline functions are preferable to macros resembling functions. > > Or create a static inline function, that is against rule of no static > inline functions in .c files. > > Or create a simple static function and mark all the arguments as unused, > which kind of compliant to the coding style, but the least pretty. Hi Ilya, I guess I would lean towards the last option. But in any case, thanks for pointing out that this is complex: I had not realised that when I wrote my previous email.
diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample + type: nest + nested-attributes: psample-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: psample-attrs + enum-name: ovs-psample-attr + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16 +/** + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE + * action. + * + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the + * sample. + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that + * contains user-defined metadata. The maximum length is + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes. + * + * Sends the packet to the psample multicast group with the specified group and + * cookie. It is possible to combine this action with the + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample. + */ +enum ovs_psample_attr { + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */ + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ + + /* private: */ + __OVS_PSAMPLE_ATTR_MAX +}; + +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_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_PSAMPLE: 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_PSAMPLE, /* Nested OVS_PSAMPLE_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..a035b7e677dd 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,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) return 0; } +#if IS_ENABLED(CONFIG_PSAMPLE) +static void execute_psample(struct datapath *dp, struct sk_buff *skb, + const struct nlattr *attr) +{ + 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_PSAMPLE_ATTR_GROUP: + psample_group.group_num = nla_get_u32(a); + break; + + case OVS_PSAMPLE_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); +} +#else +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb, + const struct nlattr *attr) {} +#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 +1540,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_PSAMPLE: + execute_psample(dp, skb, 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..c92bdc4dfe19 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_PSAMPLE: 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,28 @@ static int validate_and_copy_check_pkt_len(struct net *net, return 0; } +static int validate_psample(const struct nlattr *attr) +{ + static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = { + [OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 }, + [OVS_PSAMPLE_ATTR_COOKIE] = { + .type = NLA_BINARY, + .len = OVS_PSAMPLE_COOKIE_MAX_SIZE, + }, + }; + struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1]; + int err; + + if (!IS_ENABLED(CONFIG_PSAMPLE)) + return -EOPNOTSUPP; + + err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL); + if (err) + return err; + + return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL; +} + static int copy_action(const struct nlattr *from, struct sw_flow_actions **sfa, bool log) { @@ -3212,6 +3235,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_PSAMPLE] = (u32)-1, }; const struct ovs_action_push_vlan *vlan; int type = nla_type(a); @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, return -EINVAL; break; + case OVS_ACTION_ATTR_PSAMPLE: + err = validate_psample(a); + if (err) + return err; + break; + default: OVS_NLERR(log, "Unknown Action type %d", type); return -EINVAL;