| Message ID | 20240603185647.2310748-6-amorenoz@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: openvswitch: Add sample multicasting. | expand |
Hi Adrian,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Adrian-Moreno/net-psample-add-user-cookie/20240604-030055
base: net-next/main
patch link: https://lore.kernel.org/r/20240603185647.2310748-6-amorenoz%40redhat.com
patch subject: [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action
config: s390-randconfig-002-20240605 (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406050852.hDtfskO0-lkp@intel.com/
All errors (new ones prefixed by >>):
s390x-linux-ld: net/openvswitch/actions.o: in function `do_execute_actions':
>> actions.c:(.text+0x1d5c): undefined reference to `psample_sample_packet'
On Wed, Jun 05, 2024 at 08:29:22AM GMT, kernel test robot wrote: > Hi Adrian, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on net-next/main] > > url: https://github.com/intel-lab-lkp/linux/commits/Adrian-Moreno/net-psample-add-user-cookie/20240604-030055 > base: net-next/main > patch link: https://lore.kernel.org/r/20240603185647.2310748-6-amorenoz%40redhat.com > patch subject: [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action > config: s390-randconfig-002-20240605 (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/config) > compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202406050852.hDtfskO0-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > s390x-linux-ld: net/openvswitch/actions.o: in function `do_execute_actions': > >> actions.c:(.text+0x1d5c): undefined reference to `psample_sample_packet' > Thanks robot! OK, I think I know what's wrong. There is an optional dependency with PSAMPLE. Openvswitch module does compile without PSAMPLE but there is a link error if OPENVSWITCH=y and PSAMPLE=m. Looking into how to express this in the Kconfig, I'm planning to add the following to the next version of the series. 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
On Mon, Jun 03, 2024 at 08:56:39PM +0200, 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> Hi Adrian, Some minor nits from my side. ... > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > index efc82c318fa2..a0e9dde0584a 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > }; > #endif > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > +/** > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > + * action. > + * > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > + * sample. > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > + * user-defined metadata. The maximum length is 16 bytes. > + * > + * Sends the packet to the psample multicast group with the specified group and > + * cookie. It is possible to combine this action with the > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > + nit: One blank line is enough. Flagged by checkpatch.pl > /** > * enum ovs_action_attr - Action types. > * > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ nit: Please add OVS_ACTION_ATTR_EMIT_SAMPLE to the Kenrel doc for this structure. > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > * from userspace. */ ...
On Wed, Jun 05, 2024 at 07:31:55PM +0000, Adrián Moreno wrote: > On Wed, Jun 05, 2024 at 08:29:22AM GMT, kernel test robot wrote: > > Hi Adrian, > > > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on net-next/main] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Adrian-Moreno/net-psample-add-user-cookie/20240604-030055 > > base: net-next/main > > patch link: https://lore.kernel.org/r/20240603185647.2310748-6-amorenoz%40redhat.com > > patch subject: [PATCH net-next v2 5/9] net: openvswitch: add emit_sample action > > config: s390-randconfig-002-20240605 (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/config) > > compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a) > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240605/202406050852.hDtfskO0-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202406050852.hDtfskO0-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > s390x-linux-ld: net/openvswitch/actions.o: in function `do_execute_actions': > > >> actions.c:(.text+0x1d5c): undefined reference to `psample_sample_packet' > > > > Thanks robot! > > OK, I think I know what's wrong. There is an optional dependency with > PSAMPLE. Openvswitch module does compile without PSAMPLE but there is a > link error if OPENVSWITCH=y and PSAMPLE=m. > > Looking into how to express this in the Kconfig, I'm planning to add the > following to the next version of the series. > > 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 > Thanks Adrián, I both agree that should work, and tested with the config at the link above and found that it does work.
On Wed, Jun 05, 2024 at 08:51:17PM GMT, Simon Horman wrote: > On Mon, Jun 03, 2024 at 08:56:39PM +0200, 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> > > Hi Adrian, > > Some minor nits from my side. > > ... > > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > > index efc82c318fa2..a0e9dde0584a 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > > }; > > #endif > > > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > > +/** > > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > > + * action. > > + * > > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > > + * sample. > > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > > + * user-defined metadata. The maximum length is 16 bytes. > > + * > > + * Sends the packet to the psample multicast group with the specified group and > > + * cookie. It is possible to combine this action with the > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > > + */ > > +enum ovs_emit_sample_attr { > > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > + __OVS_EMIT_SAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > > + > > + > > nit: One blank line is enough. > Ack. > Flagged by checkpatch.pl > > > /** > > * enum ovs_action_attr - Action types. > > * > > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > nit: Please add OVS_ACTION_ATTR_EMIT_SAMPLE to the Kenrel doc > for this structure. > Thanks for spotting this. Will do. > > > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > > * from userspace. */ > > ... >
Adrian Moreno <amorenoz@redhat.com> writes: > 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> > --- I saw some of the nits Simon raised - I'll add one more below. I haven't gone through the series thoroughly enough to make a detailed review. > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > include/uapi/linux/openvswitch.h | 25 ++++++++++++ > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- > 4 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > index 4fdfc6b5cae9..a7ab5593a24f 100644 > --- a/Documentation/netlink/specs/ovs_flow.yaml > +++ b/Documentation/netlink/specs/ovs_flow.yaml > @@ -727,6 +727,12 @@ attribute-sets: > name: dec-ttl > type: nest > nested-attributes: dec-ttl-attrs > + - > + name: emit-sample > + type: nest > + nested-attributes: emit-sample-attrs > + doc: | > + Sends a packet sample to psample for external observation. > - > name: tunnel-key-attrs > enum-name: ovs-tunnel-key-attr > @@ -938,6 +944,17 @@ attribute-sets: > - > name: gbp > type: u32 > + - > + name: emit-sample-attrs > + enum-name: ovs-emit-sample-attr > + name-prefix: ovs-emit-sample-attr- > + attributes: > + - > + name: group > + type: u32 > + - > + name: cookie > + type: binary > > operations: > name-prefix: ovs-flow-cmd- > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > index efc82c318fa2..a0e9dde0584a 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > }; > #endif > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > +/** > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > + * action. > + * > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > + * sample. > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > + * user-defined metadata. The maximum length is 16 bytes. > + * > + * Sends the packet to the psample multicast group with the specified group and > + * cookie. It is possible to combine this action with the > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > + > /** > * enum ovs_action_attr - Action types. > * > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > * from userspace. */ > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 964225580824..3b4dba0ded59 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -24,6 +24,11 @@ > #include <net/checksum.h> > #include <net/dsfield.h> > #include <net/mpls.h> > + > +#if IS_ENABLED(CONFIG_PSAMPLE) > +#include <net/psample.h> > +#endif > + > #include <net/sctp/checksum.h> > > #include "datapath.h" > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > + const struct sw_flow_key *key, > + const struct nlattr *attr) > +{ > +#if IS_ENABLED(CONFIG_PSAMPLE) > + struct psample_group psample_group = {}; > + struct psample_metadata md = {}; > + struct vport *input_vport; > + const struct nlattr *a; > + int rem; > + > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > + a = nla_next(a, &rem)) { > + switch (nla_type(a)) { > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > + psample_group.group_num = nla_get_u32(a); > + break; > + > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > + md.user_cookie = nla_data(a); > + md.user_cookie_len = nla_len(a); > + break; > + } > + } > + > + psample_group.net = ovs_dp_get_net(dp); > + > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > + if (!input_vport) > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); > + > + md.in_ifindex = input_vport->dev->ifindex; > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > + > + psample_sample_packet(&psample_group, skb, 0, &md); > +#endif > + > + return 0; Why this return here? Doesn't seem used anywhere else. > +} > + > /* 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 +1547,11 @@ 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: > + err = execute_emit_sample(dp, skb, key, a); > + OVS_CB(skb)->cutlen = 0; > + break; > } > > if (unlikely(err)) { > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index f224d9bcea5e..eb59ff9c8154 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;
On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote: > Adrian Moreno <amorenoz@redhat.com> writes: > > > 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> > > --- > > I saw some of the nits Simon raised - I'll add one more below. > > I haven't gone through the series thoroughly enough to make a detailed > review. > > > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > > include/uapi/linux/openvswitch.h | 25 ++++++++++++ > > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- > > 4 files changed, 124 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > > index 4fdfc6b5cae9..a7ab5593a24f 100644 > > --- a/Documentation/netlink/specs/ovs_flow.yaml > > +++ b/Documentation/netlink/specs/ovs_flow.yaml > > @@ -727,6 +727,12 @@ attribute-sets: > > name: dec-ttl > > type: nest > > nested-attributes: dec-ttl-attrs > > + - > > + name: emit-sample > > + type: nest > > + nested-attributes: emit-sample-attrs > > + doc: | > > + Sends a packet sample to psample for external observation. > > - > > name: tunnel-key-attrs > > enum-name: ovs-tunnel-key-attr > > @@ -938,6 +944,17 @@ attribute-sets: > > - > > name: gbp > > type: u32 > > + - > > + name: emit-sample-attrs > > + enum-name: ovs-emit-sample-attr > > + name-prefix: ovs-emit-sample-attr- > > + attributes: > > + - > > + name: group > > + type: u32 > > + - > > + name: cookie > > + type: binary > > > > operations: > > name-prefix: ovs-flow-cmd- > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > > index efc82c318fa2..a0e9dde0584a 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > > }; > > #endif > > > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > > +/** > > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > > + * action. > > + * > > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > > + * sample. > > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > > + * user-defined metadata. The maximum length is 16 bytes. > > + * > > + * Sends the packet to the psample multicast group with the specified group and > > + * cookie. It is possible to combine this action with the > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > > + */ > > +enum ovs_emit_sample_attr { > > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > + __OVS_EMIT_SAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > > + > > + > > /** > > * enum ovs_action_attr - Action types. > > * > > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > > * from userspace. */ > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > index 964225580824..3b4dba0ded59 100644 > > --- a/net/openvswitch/actions.c > > +++ b/net/openvswitch/actions.c > > @@ -24,6 +24,11 @@ > > #include <net/checksum.h> > > #include <net/dsfield.h> > > #include <net/mpls.h> > > + > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > +#include <net/psample.h> > > +#endif > > + > > #include <net/sctp/checksum.h> > > > > #include "datapath.h" > > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > return 0; > > } > > > > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > > + const struct sw_flow_key *key, > > + const struct nlattr *attr) > > +{ > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > + struct psample_group psample_group = {}; > > + struct psample_metadata md = {}; > > + struct vport *input_vport; > > + const struct nlattr *a; > > + int rem; > > + > > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > > + a = nla_next(a, &rem)) { > > + switch (nla_type(a)) { > > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > > + psample_group.group_num = nla_get_u32(a); > > + break; > > + > > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > > + md.user_cookie = nla_data(a); > > + md.user_cookie_len = nla_len(a); > > + break; > > + } > > + } > > + > > + psample_group.net = ovs_dp_get_net(dp); > > + > > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > > + if (!input_vport) > > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); > > + > > + md.in_ifindex = input_vport->dev->ifindex; > > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > > + > > + psample_sample_packet(&psample_group, skb, 0, &md); > > +#endif > > + > > + return 0; > > Why this return here? Doesn't seem used anywhere else. > It is being used in "do_execute_actions", right? All non-skb-consuming actions set the value of "err" and break from the switch-case so that the the packet is dropped with OVS_DROP_ACTION_ERROR reason. Am i missing something? > > +} > > + > > /* 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 +1547,11 @@ 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: > > + err = execute_emit_sample(dp, skb, key, a); > > + OVS_CB(skb)->cutlen = 0; > > + break; > > } > > > > if (unlikely(err)) { > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > > index f224d9bcea5e..eb59ff9c8154 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; >
Adrián Moreno <amorenoz@redhat.com> writes: > On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote: >> Adrian Moreno <amorenoz@redhat.com> writes: >> >> > 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> >> > --- >> >> I saw some of the nits Simon raised - I'll add one more below. >> >> I haven't gone through the series thoroughly enough to make a detailed >> review. >> >> > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ >> > include/uapi/linux/openvswitch.h | 25 ++++++++++++ >> > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ >> > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- >> > 4 files changed, 124 insertions(+), 1 deletion(-) >> > >> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml >> > index 4fdfc6b5cae9..a7ab5593a24f 100644 >> > --- a/Documentation/netlink/specs/ovs_flow.yaml >> > +++ b/Documentation/netlink/specs/ovs_flow.yaml >> > @@ -727,6 +727,12 @@ attribute-sets: >> > name: dec-ttl >> > type: nest >> > nested-attributes: dec-ttl-attrs >> > + - >> > + name: emit-sample >> > + type: nest >> > + nested-attributes: emit-sample-attrs >> > + doc: | >> > + Sends a packet sample to psample for external observation. >> > - >> > name: tunnel-key-attrs >> > enum-name: ovs-tunnel-key-attr >> > @@ -938,6 +944,17 @@ attribute-sets: >> > - >> > name: gbp >> > type: u32 >> > + - >> > + name: emit-sample-attrs >> > + enum-name: ovs-emit-sample-attr >> > + name-prefix: ovs-emit-sample-attr- >> > + attributes: >> > + - >> > + name: group >> > + type: u32 >> > + - >> > + name: cookie >> > + type: binary >> > >> > operations: >> > name-prefix: ovs-flow-cmd- >> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h >> > index efc82c318fa2..a0e9dde0584a 100644 >> > --- a/include/uapi/linux/openvswitch.h >> > +++ b/include/uapi/linux/openvswitch.h >> > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { >> > }; >> > #endif >> > >> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 >> > +/** >> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE >> > + * action. >> > + * >> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the >> > + * sample. >> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains >> > + * user-defined metadata. The maximum length is 16 bytes. >> > + * >> > + * Sends the packet to the psample multicast group with the specified group and >> > + * cookie. It is possible to combine this action with the >> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. >> > + */ >> > +enum ovs_emit_sample_attr { >> > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, >> > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ >> > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >> > + __OVS_EMIT_SAMPLE_ATTR_MAX >> > +}; >> > + >> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >> > + >> > + >> > /** >> > * enum ovs_action_attr - Action types. >> > * >> > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { >> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >> > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ >> > OVS_ACTION_ATTR_DROP, /* u32 error code. */ >> > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ >> > >> > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >> > * from userspace. */ >> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >> > index 964225580824..3b4dba0ded59 100644 >> > --- a/net/openvswitch/actions.c >> > +++ b/net/openvswitch/actions.c >> > @@ -24,6 +24,11 @@ >> > #include <net/checksum.h> >> > #include <net/dsfield.h> >> > #include <net/mpls.h> >> > + >> > +#if IS_ENABLED(CONFIG_PSAMPLE) >> > +#include <net/psample.h> >> > +#endif >> > + >> > #include <net/sctp/checksum.h> >> > >> > #include "datapath.h" >> > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >> > return 0; >> > } >> > >> > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, >> > + const struct sw_flow_key *key, >> > + const struct nlattr *attr) >> > +{ >> > +#if IS_ENABLED(CONFIG_PSAMPLE) >> > + struct psample_group psample_group = {}; >> > + struct psample_metadata md = {}; >> > + struct vport *input_vport; >> > + const struct nlattr *a; >> > + int rem; >> > + >> > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; >> > + a = nla_next(a, &rem)) { >> > + switch (nla_type(a)) { >> > + case OVS_EMIT_SAMPLE_ATTR_GROUP: >> > + psample_group.group_num = nla_get_u32(a); >> > + break; >> > + >> > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: >> > + md.user_cookie = nla_data(a); >> > + md.user_cookie_len = nla_len(a); >> > + break; >> > + } >> > + } >> > + >> > + psample_group.net = ovs_dp_get_net(dp); >> > + >> > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); >> > + if (!input_vport) >> > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); >> > + >> > + md.in_ifindex = input_vport->dev->ifindex; >> > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; >> > + >> > + psample_sample_packet(&psample_group, skb, 0, &md); >> > +#endif >> > + >> > + return 0; >> >> Why this return here? Doesn't seem used anywhere else. >> > > It is being used in "do_execute_actions", right? > All non-skb-consuming actions set the value of "err" and break from the > switch-case so that the the packet is dropped with OVS_DROP_ACTION_ERROR reason. > > Am i missing something? I think so. For example, it isn't used when the function cannot possibly error. see the following cases: OVS_ACTION_ATTR_HASH OVS_ACTION_ATTR_TRUNC As you note, these can consume SKB so also don't bother setting err, because they will need to return anyway: OVS_ACTION_ATTR_USERSPACE OVS_ACTION_ATTR_OUTPUT OVS_ACTION_ATTR_DROP And even the following does a weird thing: OVS_ACTION_ATTR_CT because sometimes it will consume, and sometimes not. I think if there isn't a possibility of error being generated (and I guess from the code I see there isn't), then it shouldn't return a useless code, since err will be 0 on each iteration of the loop. >> > +} >> > + >> > /* 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 +1547,11 @@ 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: >> > + err = execute_emit_sample(dp, skb, key, a); >> > + OVS_CB(skb)->cutlen = 0; >> > + break; >> > } >> > >> > if (unlikely(err)) { >> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c >> > index f224d9bcea5e..eb59ff9c8154 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; >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Jun 11, 2024 at 09:54:49AM GMT, Aaron Conole wrote: > Adrián Moreno <amorenoz@redhat.com> writes: > > > On Mon, Jun 10, 2024 at 11:46:14AM GMT, Aaron Conole wrote: > >> Adrian Moreno <amorenoz@redhat.com> writes: > >> > >> > 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> > >> > --- > >> > >> I saw some of the nits Simon raised - I'll add one more below. > >> > >> I haven't gone through the series thoroughly enough to make a detailed > >> review. > >> > >> > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > >> > include/uapi/linux/openvswitch.h | 25 ++++++++++++ > >> > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ > >> > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- > >> > 4 files changed, 124 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > >> > index 4fdfc6b5cae9..a7ab5593a24f 100644 > >> > --- a/Documentation/netlink/specs/ovs_flow.yaml > >> > +++ b/Documentation/netlink/specs/ovs_flow.yaml > >> > @@ -727,6 +727,12 @@ attribute-sets: > >> > name: dec-ttl > >> > type: nest > >> > nested-attributes: dec-ttl-attrs > >> > + - > >> > + name: emit-sample > >> > + type: nest > >> > + nested-attributes: emit-sample-attrs > >> > + doc: | > >> > + Sends a packet sample to psample for external observation. > >> > - > >> > name: tunnel-key-attrs > >> > enum-name: ovs-tunnel-key-attr > >> > @@ -938,6 +944,17 @@ attribute-sets: > >> > - > >> > name: gbp > >> > type: u32 > >> > + - > >> > + name: emit-sample-attrs > >> > + enum-name: ovs-emit-sample-attr > >> > + name-prefix: ovs-emit-sample-attr- > >> > + attributes: > >> > + - > >> > + name: group > >> > + type: u32 > >> > + - > >> > + name: cookie > >> > + type: binary > >> > > >> > operations: > >> > name-prefix: ovs-flow-cmd- > >> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > >> > index efc82c318fa2..a0e9dde0584a 100644 > >> > --- a/include/uapi/linux/openvswitch.h > >> > +++ b/include/uapi/linux/openvswitch.h > >> > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > >> > }; > >> > #endif > >> > > >> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > >> > +/** > >> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > >> > + * action. > >> > + * > >> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > >> > + * sample. > >> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > >> > + * user-defined metadata. The maximum length is 16 bytes. > >> > + * > >> > + * Sends the packet to the psample multicast group with the specified group and > >> > + * cookie. It is possible to combine this action with the > >> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > >> > + */ > >> > +enum ovs_emit_sample_attr { > >> > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > >> > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > >> > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > >> > + __OVS_EMIT_SAMPLE_ATTR_MAX > >> > +}; > >> > + > >> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > >> > + > >> > + > >> > /** > >> > * enum ovs_action_attr - Action types. > >> > * > >> > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > >> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > >> > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > >> > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > >> > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > >> > > >> > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > >> > * from userspace. */ > >> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > >> > index 964225580824..3b4dba0ded59 100644 > >> > --- a/net/openvswitch/actions.c > >> > +++ b/net/openvswitch/actions.c > >> > @@ -24,6 +24,11 @@ > >> > #include <net/checksum.h> > >> > #include <net/dsfield.h> > >> > #include <net/mpls.h> > >> > + > >> > +#if IS_ENABLED(CONFIG_PSAMPLE) > >> > +#include <net/psample.h> > >> > +#endif > >> > + > >> > #include <net/sctp/checksum.h> > >> > > >> > #include "datapath.h" > >> > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > >> > return 0; > >> > } > >> > > >> > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > >> > + const struct sw_flow_key *key, > >> > + const struct nlattr *attr) > >> > +{ > >> > +#if IS_ENABLED(CONFIG_PSAMPLE) > >> > + struct psample_group psample_group = {}; > >> > + struct psample_metadata md = {}; > >> > + struct vport *input_vport; > >> > + const struct nlattr *a; > >> > + int rem; > >> > + > >> > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > >> > + a = nla_next(a, &rem)) { > >> > + switch (nla_type(a)) { > >> > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > >> > + psample_group.group_num = nla_get_u32(a); > >> > + break; > >> > + > >> > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > >> > + md.user_cookie = nla_data(a); > >> > + md.user_cookie_len = nla_len(a); > >> > + break; > >> > + } > >> > + } > >> > + > >> > + psample_group.net = ovs_dp_get_net(dp); > >> > + > >> > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > >> > + if (!input_vport) > >> > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); > >> > + > >> > + md.in_ifindex = input_vport->dev->ifindex; > >> > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > >> > + > >> > + psample_sample_packet(&psample_group, skb, 0, &md); > >> > +#endif > >> > + > >> > + return 0; > >> > >> Why this return here? Doesn't seem used anywhere else. > >> > > > > It is being used in "do_execute_actions", right? > > All non-skb-consuming actions set the value of "err" and break from the > > switch-case so that the the packet is dropped with OVS_DROP_ACTION_ERROR reason. > > > > Am i missing something? > > I think so. For example, it isn't used when the function cannot > possibly error. > > see the following cases: > > OVS_ACTION_ATTR_HASH > OVS_ACTION_ATTR_TRUNC > > As you note, these can consume SKB so also don't bother setting err, > because they will need to return anyway: > > OVS_ACTION_ATTR_USERSPACE > OVS_ACTION_ATTR_OUTPUT > OVS_ACTION_ATTR_DROP > > And even the following does a weird thing: > > OVS_ACTION_ATTR_CT > > because sometimes it will consume, and sometimes not. > > I think if there isn't a possibility of error being generated (and I > guess from the code I see there isn't), then it shouldn't return a > useless code, since err will be 0 on each iteration of the loop. > Oh, so you meant it's actualy not being set. Now I get you. Yes. I figured that could change in the future so I left the structure of returning an error just in case, but it's true that currently the function cannot fail. I'll get rid of it. > >> > +} > >> > + > >> > /* 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 +1547,11 @@ 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: > >> > + err = execute_emit_sample(dp, skb, key, a); > >> > + OVS_CB(skb)->cutlen = 0; > >> > + break; > >> > } > >> > > >> > if (unlikely(err)) { > >> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > >> > index f224d9bcea5e..eb59ff9c8154 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; > >> > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Jun 03, 2024 at 08:56:39PM +0200, 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> ... > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c ... > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > + const struct sw_flow_key *key, > + const struct nlattr *attr) > +{ > +#if IS_ENABLED(CONFIG_PSAMPLE) > + struct psample_group psample_group = {}; > + struct psample_metadata md = {}; > + struct vport *input_vport; > + const struct nlattr *a; > + int rem; > + > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > + a = nla_next(a, &rem)) { > + switch (nla_type(a)) { > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > + psample_group.group_num = nla_get_u32(a); > + break; > + > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > + md.user_cookie = nla_data(a); > + md.user_cookie_len = nla_len(a); > + break; > + } > + } > + > + psample_group.net = ovs_dp_get_net(dp); > + > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > + if (!input_vport) > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); > + > + md.in_ifindex = input_vport->dev->ifindex; > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > + > + psample_sample_packet(&psample_group, skb, 0, &md); > +#endif > + > + return 0; > +} > + > /* 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 +1547,11 @@ 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: > + err = execute_emit_sample(dp, skb, key, a); > + OVS_CB(skb)->cutlen = 0; > + break; > } Hi Adrian, execute_emit_sample always returns 0, and it seems that err will always be 0 when the code above is executed. So perhaps the return type of execute_emit_sample could be changed to void and the code above be updated not to set err. Other than that, which I don't feel particularly strongly about, this looks good to me. ...
On 6/3/24 20:56, Adrian Moreno wrote: > Add support for a new action: emit_sample. > > This action accepts a u32 group id and a variable-length cookie and uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > include/uapi/linux/openvswitch.h | 25 ++++++++++++ > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- > 4 files changed, 124 insertions(+), 1 deletion(-) Some nits below, beside ones already mentioned. > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > index 4fdfc6b5cae9..a7ab5593a24f 100644 > --- a/Documentation/netlink/specs/ovs_flow.yaml > +++ b/Documentation/netlink/specs/ovs_flow.yaml > @@ -727,6 +727,12 @@ attribute-sets: > name: dec-ttl > type: nest > nested-attributes: dec-ttl-attrs > + - > + name: emit-sample > + type: nest > + nested-attributes: emit-sample-attrs > + doc: | > + Sends a packet sample to psample for external observation. > - > name: tunnel-key-attrs > enum-name: ovs-tunnel-key-attr > @@ -938,6 +944,17 @@ attribute-sets: > - > name: gbp > type: u32 > + - > + name: emit-sample-attrs > + enum-name: ovs-emit-sample-attr > + name-prefix: ovs-emit-sample-attr- > + attributes: > + - > + name: group > + type: u32 > + - > + name: cookie > + type: binary > > operations: > name-prefix: ovs-flow-cmd- > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > index efc82c318fa2..a0e9dde0584a 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > }; > #endif > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > +/** > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > + * action. > + * > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > + * sample. > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > + * user-defined metadata. The maximum length is 16 bytes. s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/ > + * > + * Sends the packet to the psample multicast group with the specified group and > + * cookie. It is possible to combine this action with the > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > + */ > +enum ovs_emit_sample_attr { > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > + __OVS_EMIT_SAMPLE_ATTR_MAX > +}; > + > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > + > + > /** > * enum ovs_action_attr - Action types. > * > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > * from userspace. */ > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 964225580824..3b4dba0ded59 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -24,6 +24,11 @@ > #include <net/checksum.h> > #include <net/dsfield.h> > #include <net/mpls.h> > + > +#if IS_ENABLED(CONFIG_PSAMPLE) > +#include <net/psample.h> > +#endif > + > #include <net/sctp/checksum.h> > > #include "datapath.h" > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > return 0; > } > > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > + const struct sw_flow_key *key, > + const struct nlattr *attr) > +{ > +#if IS_ENABLED(CONFIG_PSAMPLE) > + struct psample_group psample_group = {}; > + struct psample_metadata md = {}; > + struct vport *input_vport; > + const struct nlattr *a; > + int rem; > + > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > + a = nla_next(a, &rem)) { Since the action is strictly validated, can use use nla_for_each_attr() or nla_for_each_nested() ? > + switch (nla_type(a)) { > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > + psample_group.group_num = nla_get_u32(a); > + break; > + > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > + md.user_cookie = nla_data(a); > + md.user_cookie_len = nla_len(a); > + break; > + } > + } > + > + psample_group.net = ovs_dp_get_net(dp); > + > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > + if (!input_vport) > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); We may need to check that we actually found the local port. > + > + md.in_ifindex = input_vport->dev->ifindex; > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > + > + psample_sample_packet(&psample_group, skb, 0, &md); > +#endif > + > + return 0; > +} > + > /* 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 +1547,11 @@ 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: > + err = execute_emit_sample(dp, skb, key, a); > + OVS_CB(skb)->cutlen = 0; > + break; > } > > if (unlikely(err)) { > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index f224d9bcea5e..eb59ff9c8154 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 Maybe add a trailing comma here as well, since it's not a one-line definition. Just in case. > + }, > + }; > + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; One too many spaces ^^ > + 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;
On Mon, Jun 17, 2024 at 12:44:45PM GMT, Ilya Maximets wrote: > On 6/3/24 20:56, Adrian Moreno wrote: > > Add support for a new action: emit_sample. > > > > This action accepts a u32 group id and a variable-length cookie and uses > > the psample multicast group to make the packet available for > > observability. > > > > The maximum length of the user-defined cookie is set to 16, same as > > tc_cookie, to discourage using cookies that will not be offloadable. > > > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > --- > > Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ > > include/uapi/linux/openvswitch.h | 25 ++++++++++++ > > net/openvswitch/actions.c | 50 +++++++++++++++++++++++ > > net/openvswitch/flow_netlink.c | 33 ++++++++++++++- > > 4 files changed, 124 insertions(+), 1 deletion(-) > > Some nits below, beside ones already mentioned. > Thanks, Ilya. > > > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml > > index 4fdfc6b5cae9..a7ab5593a24f 100644 > > --- a/Documentation/netlink/specs/ovs_flow.yaml > > +++ b/Documentation/netlink/specs/ovs_flow.yaml > > @@ -727,6 +727,12 @@ attribute-sets: > > name: dec-ttl > > type: nest > > nested-attributes: dec-ttl-attrs > > + - > > + name: emit-sample > > + type: nest > > + nested-attributes: emit-sample-attrs > > + doc: | > > + Sends a packet sample to psample for external observation. > > - > > name: tunnel-key-attrs > > enum-name: ovs-tunnel-key-attr > > @@ -938,6 +944,17 @@ attribute-sets: > > - > > name: gbp > > type: u32 > > + - > > + name: emit-sample-attrs > > + enum-name: ovs-emit-sample-attr > > + name-prefix: ovs-emit-sample-attr- > > + attributes: > > + - > > + name: group > > + type: u32 > > + - > > + name: cookie > > + type: binary > > > > operations: > > name-prefix: ovs-flow-cmd- > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > > index efc82c318fa2..a0e9dde0584a 100644 > > --- a/include/uapi/linux/openvswitch.h > > +++ b/include/uapi/linux/openvswitch.h > > @@ -914,6 +914,30 @@ struct check_pkt_len_arg { > > }; > > #endif > > > > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 > > +/** > > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE > > + * action. > > + * > > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the > > + * sample. > > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains > > + * user-defined metadata. The maximum length is 16 bytes. > > s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/ > > > + * > > + * Sends the packet to the psample multicast group with the specified group and > > + * cookie. It is possible to combine this action with the > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. > > + */ > > +enum ovs_emit_sample_attr { > > + OVS_EMIT_SAMPLE_ATTR_UNPSEC, > > + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ > > + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ > > + __OVS_EMIT_SAMPLE_ATTR_MAX > > +}; > > + > > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) > > + > > + > > /** > > * enum ovs_action_attr - Action types. > > * > > @@ -1004,6 +1028,7 @@ enum ovs_action_attr { > > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > OVS_ACTION_ATTR_DROP, /* u32 error code. */ > > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ > > > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > > * from userspace. */ > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > index 964225580824..3b4dba0ded59 100644 > > --- a/net/openvswitch/actions.c > > +++ b/net/openvswitch/actions.c > > @@ -24,6 +24,11 @@ > > #include <net/checksum.h> > > #include <net/dsfield.h> > > #include <net/mpls.h> > > + > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > +#include <net/psample.h> > > +#endif > > + > > #include <net/sctp/checksum.h> > > > > #include "datapath.h" > > @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) > > return 0; > > } > > > > +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, > > + const struct sw_flow_key *key, > > + const struct nlattr *attr) > > +{ > > +#if IS_ENABLED(CONFIG_PSAMPLE) > > + struct psample_group psample_group = {}; > > + struct psample_metadata md = {}; > > + struct vport *input_vport; > > + const struct nlattr *a; > > + int rem; > > + > > + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; > > + a = nla_next(a, &rem)) { > > Since the action is strictly validated, can use use nla_for_each_attr() > or nla_for_each_nested() ? > Probably, yes. > > + switch (nla_type(a)) { > > + case OVS_EMIT_SAMPLE_ATTR_GROUP: > > + psample_group.group_num = nla_get_u32(a); > > + break; > > + > > + case OVS_EMIT_SAMPLE_ATTR_COOKIE: > > + md.user_cookie = nla_data(a); > > + md.user_cookie_len = nla_len(a); > > + break; > > + } > > + } > > + > > + psample_group.net = ovs_dp_get_net(dp); > > + > > + input_vport = ovs_vport_rcu(dp, key->phy.in_port); > > + if (!input_vport) > > + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); > > We may need to check that we actually found the local port. > Sure. What can cause the local port not to exist? > > + > > + md.in_ifindex = input_vport->dev->ifindex; > > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > > + > > + psample_sample_packet(&psample_group, skb, 0, &md); > > +#endif > > + > > + return 0; > > +} > > + > > /* 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 +1547,11 @@ 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: > > + err = execute_emit_sample(dp, skb, key, a); > > + OVS_CB(skb)->cutlen = 0; > > + break; > > } > > > > if (unlikely(err)) { > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > > index f224d9bcea5e..eb59ff9c8154 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 > > Maybe add a trailing comma here as well, since it's not a one-line definition. > Just in case. > Sure. > > + }, > > + }; > > + struct nlattr *a[OVS_EMIT_SAMPLE_ATTR_MAX + 1]; > > One too many spaces ^^ > Thanks. > > + 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; >
On 6/18/24 09:33, Adrián Moreno wrote: > On Mon, Jun 17, 2024 at 12:44:45PM GMT, Ilya Maximets wrote: >> On 6/3/24 20:56, Adrian Moreno wrote: >>> Add support for a new action: emit_sample. >>> >>> This action accepts a u32 group id and a variable-length cookie and uses >>> the psample multicast group to make the packet available for >>> observability. >>> >>> The maximum length of the user-defined cookie is set to 16, same as >>> tc_cookie, to discourage using cookies that will not be offloadable. >>> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ >>> include/uapi/linux/openvswitch.h | 25 ++++++++++++ >>> net/openvswitch/actions.c | 50 +++++++++++++++++++++++ >>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++- >>> 4 files changed, 124 insertions(+), 1 deletion(-) >> >> Some nits below, beside ones already mentioned. >> > > Thanks, Ilya. > >>> >>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml >>> index 4fdfc6b5cae9..a7ab5593a24f 100644 >>> --- a/Documentation/netlink/specs/ovs_flow.yaml >>> +++ b/Documentation/netlink/specs/ovs_flow.yaml >>> @@ -727,6 +727,12 @@ attribute-sets: >>> name: dec-ttl >>> type: nest >>> nested-attributes: dec-ttl-attrs >>> + - >>> + name: emit-sample >>> + type: nest >>> + nested-attributes: emit-sample-attrs >>> + doc: | >>> + Sends a packet sample to psample for external observation. >>> - >>> name: tunnel-key-attrs >>> enum-name: ovs-tunnel-key-attr >>> @@ -938,6 +944,17 @@ attribute-sets: >>> - >>> name: gbp >>> type: u32 >>> + - >>> + name: emit-sample-attrs >>> + enum-name: ovs-emit-sample-attr >>> + name-prefix: ovs-emit-sample-attr- >>> + attributes: >>> + - >>> + name: group >>> + type: u32 >>> + - >>> + name: cookie >>> + type: binary >>> >>> operations: >>> name-prefix: ovs-flow-cmd- >>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h >>> index efc82c318fa2..a0e9dde0584a 100644 >>> --- a/include/uapi/linux/openvswitch.h >>> +++ b/include/uapi/linux/openvswitch.h >>> @@ -914,6 +914,30 @@ struct check_pkt_len_arg { >>> }; >>> #endif >>> >>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 >>> +/** >>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE >>> + * action. >>> + * >>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the >>> + * sample. >>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains >>> + * user-defined metadata. The maximum length is 16 bytes. >> >> s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/ >> >>> + * >>> + * Sends the packet to the psample multicast group with the specified group and >>> + * cookie. It is possible to combine this action with the >>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. >>> + */ >>> +enum ovs_emit_sample_attr { >>> + OVS_EMIT_SAMPLE_ATTR_UNPSEC, >>> + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ >>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>> + __OVS_EMIT_SAMPLE_ATTR_MAX >>> +}; >>> + >>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >>> + >>> + >>> /** >>> * enum ovs_action_attr - Action types. >>> * >>> @@ -1004,6 +1028,7 @@ enum ovs_action_attr { >>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ >>> OVS_ACTION_ATTR_DROP, /* u32 error code. */ >>> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ >>> >>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >>> * from userspace. */ >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>> index 964225580824..3b4dba0ded59 100644 >>> --- a/net/openvswitch/actions.c >>> +++ b/net/openvswitch/actions.c >>> @@ -24,6 +24,11 @@ >>> #include <net/checksum.h> >>> #include <net/dsfield.h> >>> #include <net/mpls.h> >>> + >>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>> +#include <net/psample.h> >>> +#endif >>> + >>> #include <net/sctp/checksum.h> >>> >>> #include "datapath.h" >>> @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >>> return 0; >>> } >>> >>> +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, >>> + const struct sw_flow_key *key, >>> + const struct nlattr *attr) >>> +{ >>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>> + struct psample_group psample_group = {}; >>> + struct psample_metadata md = {}; >>> + struct vport *input_vport; >>> + const struct nlattr *a; >>> + int rem; >>> + >>> + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; >>> + a = nla_next(a, &rem)) { >> >> Since the action is strictly validated, can use use nla_for_each_attr() >> or nla_for_each_nested() ? >> > > Probably, yes. > >>> + switch (nla_type(a)) { >>> + case OVS_EMIT_SAMPLE_ATTR_GROUP: >>> + psample_group.group_num = nla_get_u32(a); >>> + break; >>> + >>> + case OVS_EMIT_SAMPLE_ATTR_COOKIE: >>> + md.user_cookie = nla_data(a); >>> + md.user_cookie_len = nla_len(a); >>> + break; >>> + } >>> + } >>> + >>> + psample_group.net = ovs_dp_get_net(dp); >>> + >>> + input_vport = ovs_vport_rcu(dp, key->phy.in_port); >>> + if (!input_vport) >>> + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); >> >> We may need to check that we actually found the local port. >> > > Sure. What can cause the local port not to exist? I would assume that since we're only protected by RCU here, there can be a race with datapath destruction that will remove the local port. Best regards, Ilya Maximets.
On 6/18/24 11:47, Ilya Maximets wrote: > On 6/18/24 09:33, Adrián Moreno wrote: >> On Mon, Jun 17, 2024 at 12:44:45PM GMT, Ilya Maximets wrote: >>> On 6/3/24 20:56, Adrian Moreno wrote: >>>> Add support for a new action: emit_sample. >>>> >>>> This action accepts a u32 group id and a variable-length cookie and uses >>>> the psample multicast group to make the packet available for >>>> observability. >>>> >>>> The maximum length of the user-defined cookie is set to 16, same as >>>> tc_cookie, to discourage using cookies that will not be offloadable. >>>> >>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>> --- >>>> Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++ >>>> include/uapi/linux/openvswitch.h | 25 ++++++++++++ >>>> net/openvswitch/actions.c | 50 +++++++++++++++++++++++ >>>> net/openvswitch/flow_netlink.c | 33 ++++++++++++++- >>>> 4 files changed, 124 insertions(+), 1 deletion(-) >>> >>> Some nits below, beside ones already mentioned. >>> >> >> Thanks, Ilya. >> >>>> >>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml >>>> index 4fdfc6b5cae9..a7ab5593a24f 100644 >>>> --- a/Documentation/netlink/specs/ovs_flow.yaml >>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml >>>> @@ -727,6 +727,12 @@ attribute-sets: >>>> name: dec-ttl >>>> type: nest >>>> nested-attributes: dec-ttl-attrs >>>> + - >>>> + name: emit-sample >>>> + type: nest >>>> + nested-attributes: emit-sample-attrs >>>> + doc: | >>>> + Sends a packet sample to psample for external observation. >>>> - >>>> name: tunnel-key-attrs >>>> enum-name: ovs-tunnel-key-attr >>>> @@ -938,6 +944,17 @@ attribute-sets: >>>> - >>>> name: gbp >>>> type: u32 >>>> + - >>>> + name: emit-sample-attrs >>>> + enum-name: ovs-emit-sample-attr >>>> + name-prefix: ovs-emit-sample-attr- >>>> + attributes: >>>> + - >>>> + name: group >>>> + type: u32 >>>> + - >>>> + name: cookie >>>> + type: binary >>>> >>>> operations: >>>> name-prefix: ovs-flow-cmd- >>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h >>>> index efc82c318fa2..a0e9dde0584a 100644 >>>> --- a/include/uapi/linux/openvswitch.h >>>> +++ b/include/uapi/linux/openvswitch.h >>>> @@ -914,6 +914,30 @@ struct check_pkt_len_arg { >>>> }; >>>> #endif >>>> >>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 >>>> +/** >>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE >>>> + * action. >>>> + * >>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the >>>> + * sample. >>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains >>>> + * user-defined metadata. The maximum length is 16 bytes. >>> >>> s/16/OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE/ >>> >>>> + * >>>> + * Sends the packet to the psample multicast group with the specified group and >>>> + * cookie. It is possible to combine this action with the >>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. >>>> + */ >>>> +enum ovs_emit_sample_attr { >>>> + OVS_EMIT_SAMPLE_ATTR_UNPSEC, >>>> + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ >>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ >>>> + __OVS_EMIT_SAMPLE_ATTR_MAX >>>> +}; >>>> + >>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) >>>> + >>>> + >>>> /** >>>> * enum ovs_action_attr - Action types. >>>> * >>>> @@ -1004,6 +1028,7 @@ enum ovs_action_attr { >>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ >>>> OVS_ACTION_ATTR_DROP, /* u32 error code. */ >>>> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ >>>> >>>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >>>> * from userspace. */ >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>>> index 964225580824..3b4dba0ded59 100644 >>>> --- a/net/openvswitch/actions.c >>>> +++ b/net/openvswitch/actions.c >>>> @@ -24,6 +24,11 @@ >>>> #include <net/checksum.h> >>>> #include <net/dsfield.h> >>>> #include <net/mpls.h> >>>> + >>>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>>> +#include <net/psample.h> >>>> +#endif >>>> + >>>> #include <net/sctp/checksum.h> >>>> >>>> #include "datapath.h" >>>> @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) >>>> return 0; >>>> } >>>> >>>> +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, >>>> + const struct sw_flow_key *key, >>>> + const struct nlattr *attr) >>>> +{ >>>> +#if IS_ENABLED(CONFIG_PSAMPLE) >>>> + struct psample_group psample_group = {}; >>>> + struct psample_metadata md = {}; >>>> + struct vport *input_vport; >>>> + const struct nlattr *a; >>>> + int rem; >>>> + >>>> + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; >>>> + a = nla_next(a, &rem)) { >>> >>> Since the action is strictly validated, can use use nla_for_each_attr() >>> or nla_for_each_nested() ? >>> >> >> Probably, yes. >> >>>> + switch (nla_type(a)) { >>>> + case OVS_EMIT_SAMPLE_ATTR_GROUP: >>>> + psample_group.group_num = nla_get_u32(a); >>>> + break; >>>> + >>>> + case OVS_EMIT_SAMPLE_ATTR_COOKIE: >>>> + md.user_cookie = nla_data(a); >>>> + md.user_cookie_len = nla_len(a); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + psample_group.net = ovs_dp_get_net(dp); >>>> + >>>> + input_vport = ovs_vport_rcu(dp, key->phy.in_port); >>>> + if (!input_vport) >>>> + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); >>> >>> We may need to check that we actually found the local port. >>> >> >> Sure. What can cause the local port not to exist? > > I would assume that since we're only protected by RCU here, there can be > a race with datapath destruction that will remove the local port. But, actually, we don't even need to look anything up. The original input vport should be available in OVS_CB(skb)->input_vport. Best regards, Ilya Maximets.
diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml index 4fdfc6b5cae9..a7ab5593a24f 100644 --- a/Documentation/netlink/specs/ovs_flow.yaml +++ b/Documentation/netlink/specs/ovs_flow.yaml @@ -727,6 +727,12 @@ attribute-sets: name: dec-ttl type: nest nested-attributes: dec-ttl-attrs + - + name: emit-sample + type: nest + nested-attributes: emit-sample-attrs + doc: | + Sends a packet sample to psample for external observation. - name: tunnel-key-attrs enum-name: ovs-tunnel-key-attr @@ -938,6 +944,17 @@ attribute-sets: - name: gbp type: u32 + - + name: emit-sample-attrs + enum-name: ovs-emit-sample-attr + name-prefix: ovs-emit-sample-attr- + attributes: + - + name: group + type: u32 + - + name: cookie + type: binary operations: name-prefix: ovs-flow-cmd- diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index efc82c318fa2..a0e9dde0584a 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -914,6 +914,30 @@ struct check_pkt_len_arg { }; #endif +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16 +/** + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE + * action. + * + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the + * sample. + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains + * user-defined metadata. The maximum length is 16 bytes. + * + * Sends the packet to the psample multicast group with the specified group and + * cookie. It is possible to combine this action with the + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted. + */ +enum ovs_emit_sample_attr { + OVS_EMIT_SAMPLE_ATTR_UNPSEC, + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */ + OVS_EMIT_SAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */ + __OVS_EMIT_SAMPLE_ATTR_MAX +}; + +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1) + + /** * enum ovs_action_attr - Action types. * @@ -1004,6 +1028,7 @@ enum ovs_action_attr { OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ OVS_ACTION_ATTR_DROP, /* u32 error code. */ + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */ __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted * from userspace. */ diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 964225580824..3b4dba0ded59 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -24,6 +24,11 @@ #include <net/checksum.h> #include <net/dsfield.h> #include <net/mpls.h> + +#if IS_ENABLED(CONFIG_PSAMPLE) +#include <net/psample.h> +#endif + #include <net/sctp/checksum.h> #include "datapath.h" @@ -1299,6 +1304,46 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key) return 0; } +static int execute_emit_sample(struct datapath *dp, struct sk_buff *skb, + const struct sw_flow_key *key, + const struct nlattr *attr) +{ +#if IS_ENABLED(CONFIG_PSAMPLE) + struct psample_group psample_group = {}; + struct psample_metadata md = {}; + struct vport *input_vport; + const struct nlattr *a; + int rem; + + for (a = nla_data(attr), rem = nla_len(attr); rem > 0; + a = nla_next(a, &rem)) { + switch (nla_type(a)) { + case OVS_EMIT_SAMPLE_ATTR_GROUP: + psample_group.group_num = nla_get_u32(a); + break; + + case OVS_EMIT_SAMPLE_ATTR_COOKIE: + md.user_cookie = nla_data(a); + md.user_cookie_len = nla_len(a); + break; + } + } + + psample_group.net = ovs_dp_get_net(dp); + + input_vport = ovs_vport_rcu(dp, key->phy.in_port); + if (!input_vport) + input_vport = ovs_vport_rcu(dp, OVSP_LOCAL); + + md.in_ifindex = input_vport->dev->ifindex; + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; + + psample_sample_packet(&psample_group, skb, 0, &md); +#endif + + return 0; +} + /* 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 +1547,11 @@ 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: + err = execute_emit_sample(dp, skb, key, a); + OVS_CB(skb)->cutlen = 0; + break; } if (unlikely(err)) { diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index f224d9bcea5e..eb59ff9c8154 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 | 25 ++++++++++++ net/openvswitch/actions.c | 50 +++++++++++++++++++++++ net/openvswitch/flow_netlink.c | 33 ++++++++++++++- 4 files changed, 124 insertions(+), 1 deletion(-)