Adrian Moreno <amore...@redhat.com> writes: > On 5/3/24 11:43 AM, Eelco Chaudron wrote: >> On 24 Apr 2024, at 15:50, Adrian Moreno wrote: >> >>> Add support for psample sampling via two new attributes to the >>> OVS_ACTION_ATTR_SAMPLE action. >>> >>> OVS_SAMPLE_ATTR_PSAMPLE_GROUP used to pass an integer psample group_id. >>> OVS_SAMPLE_ATTR_PSAMPLE_COOKIE used to pass a variable-length binary >>> cookie that will be forwared to psample. >>> >>> 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. >>> >>> In order to simplify the internal processing of the action and given the >>> maximum size of the cookie is relatively small, add both fields to the >>> internal-only struct sample_arg. >>> >>> The presence of a group_id mandates that the action shall called the >>> psample module to multicast the packet with such group_id and the >>> user-provided cookie if present. This behavior is orthonogal to >>> also executing the nested actions if present. >>> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >> This is not a full review yet. Just some comments, as I’m looking at >> the user-space patch first and added similar comments. >> I’ll do a proper review of this series once I’m done with user-space >> part. >> //Eelco >> >>> --- >>> Documentation/netlink/specs/ovs_flow.yaml | 6 ++ >>> include/uapi/linux/openvswitch.h | 49 ++++++++++---- >>> net/openvswitch/actions.c | 51 +++++++++++++-- >>> net/openvswitch/flow_netlink.c | 80 ++++++++++++++++++----- >>> 4 files changed, 153 insertions(+), 33 deletions(-) >>> >>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml >>> b/Documentation/netlink/specs/ovs_flow.yaml >>> index 4fdfc6b5cae9..5543c2937225 100644 >>> --- a/Documentation/netlink/specs/ovs_flow.yaml >>> +++ b/Documentation/netlink/specs/ovs_flow.yaml >>> @@ -825,6 +825,12 @@ attribute-sets: >>> name: actions >>> type: nest >>> nested-attributes: action-attrs >>> + - >>> + name: psample_group >>> + type: u32 >>> + - >>> + name: psample_cookie >>> + type: binary >>> - >>> name: userspace-attrs >>> enum-name: ovs-userspace-attr >>> diff --git a/include/uapi/linux/openvswitch.h >>> b/include/uapi/linux/openvswitch.h >>> index efc82c318fa2..e9cd6f3a952d 100644 >>> --- a/include/uapi/linux/openvswitch.h >>> +++ b/include/uapi/linux/openvswitch.h >>> @@ -639,6 +639,7 @@ enum ovs_flow_attr { >>> #define OVS_UFID_F_OMIT_MASK (1 << 1) >>> #define OVS_UFID_F_OMIT_ACTIONS (1 << 2) >>> >>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16 >>> /** >>> * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action. >>> * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with >>> @@ -646,15 +647,27 @@ enum ovs_flow_attr { >>> * %UINT32_MAX samples all packets and intermediate values sample >>> intermediate >>> * fractions of packets. >>> * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event. >>> - * Actions are passed as nested attributes. >>> + * Actions are passed as nested attributes. Optional if >>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set. >>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as psample >>> group. >>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set. >>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie that, >>> if >>> + * provided, will be copied to the psample cookie. >> As there is a limit of to the cookie should we mention it here? >> > > I thought OVS_PSAMPLE_COOKIE_MAX_SIZE was expressive enough but sure, > we can also mention it here. > >>> * >>> - * Executes the specified actions with the given probability on a >>> per-packet >>> - * basis. >>> + * Either OVS_SAMPLE_ATTR_PSAMPLE_GROUP or OVS_SAMPLE_ATTR_ACTIONS must be >>> + * specified. >>> + * >>> + * Executes the specified actions and/or sends the packet to psample >>> + * with the given probability on a per-packet basis. >>> */ >>> enum ovs_sample_attr { >>> OVS_SAMPLE_ATTR_UNSPEC, >>> - OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */ >>> - OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */ >>> + OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */ >>> + OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_ >> Missing * after OVS_ACTION_ATTR_ > > As a matter of fact, adding an * makes checkpatch generate a warning > IIRC. That's why I initially removed it. I can look at fixing > checkpatch instead.
I think we can ignore the warning. Alternatively, consider not changing the comment spacing for the existing comments. >> >>> + * attributes. >>> + */ >>> + OVS_SAMPLE_ATTR_PSAMPLE_GROUP, /* u32 number */ >>> + OVS_SAMPLE_ATTR_PSAMPLE_COOKIE, /* binary */ >> As these are general sample options, I would not add the PSAMPLE >> reference. Other data paths could use a different implementation. So >> I guess OVS_SAMPLE_ATTR_GROUP_ID and OVS_SAMPLE_ATTR_COOKIE would be >> enough. >> > > OK. But isn't the API already psample-ish? I mean that the group_id is > something specific to psample that might not be present in other > datapath implementation. > > >>> __OVS_SAMPLE_ATTR_MAX, >>> >>> #ifdef __KERNEL__ >>> @@ -665,13 +678,27 @@ enum ovs_sample_attr { >>> #define OVS_SAMPLE_ATTR_MAX (__OVS_SAMPLE_ATTR_MAX - 1) >>> >>> #ifdef __KERNEL__ >>> + >>> +/* Definition for flags in struct sample_arg. */ >>> +enum { >>> + /* When set, actions in sample will not change the flows. */ >>> + OVS_SAMPLE_ARG_FLAG_EXEC = 1 << 0, >>> + /* When set, the packet will be sent to psample. */ >>> + OVS_SAMPLE_ARG_FLAG_PSAMPLE = 1 << 1, >>> +}; >>> + >>> struct sample_arg { >>> - bool exec; /* When true, actions in sample will not >>> - * change flow keys. False otherwise. >>> - */ >>> - u32 probability; /* Same value as >>> - * 'OVS_SAMPLE_ATTR_PROBABILITY'. >>> - */ >> Not sure if you can actually do this, you are changing a structure >> that is part of the UAPI. This change breaks backwards >> compatibility. >> > > Hmmm... this the internal argument structure protected by #ifdef > __KERNEL__. It is used in several actions to optimize the internal > action handling (i.e: using a compact struct instead of a list of > netlink attributes). I guess the reason for having it in this file is > because the attribute enum is being reused, but I wouldn't think of > this struct as part of the uAPI. > > If the (#ifdef __KERNEL__) does not exclude this struct from the uAPI > I think we should move it (all of them actually) to some internal > file. > >> >>> + u16 flags; /* Flags that modify the behavior of the >>> + * action. See SAMPLE_ARG_FLAG_*. >>> + */ >>> + u32 probability; /* Same value as >>> + * 'OVS_SAMPLE_ATTR_PROBABILITY'. >>> + */ >>> + u32 group_id; /* Same value as >>> + * 'OVS_SAMPLE_ATTR_PSAMPLE_GROUP'. >>> + */ >>> + u8 cookie_len; /* Length of psample cookie. */ >>> + char cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE]; /* psample cookie data. */ >> Would it make sense for the cookie also to be u8? >> > > Yes, probably. > >>> }; >>> #endif >>> >>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>> index 6fcd7e2ca81f..eb3166986fd2 100644 >>> --- a/net/openvswitch/actions.c >>> +++ b/net/openvswitch/actions.c >>> @@ -24,6 +24,7 @@ >>> #include <net/checksum.h> >>> #include <net/dsfield.h> >>> #include <net/mpls.h> >>> +#include <net/psample.h> >>> #include <net/sctp/checksum.h> >>> >>> #include "datapath.h" >>> @@ -1025,6 +1026,34 @@ static int dec_ttl_exception_handler(struct datapath >>> *dp, struct sk_buff *skb, >>> return 0; >>> } >>> >>> +static int ovs_psample_packet(struct datapath *dp, struct sw_flow_key *key, >>> + const struct sample_arg *arg, >>> + struct sk_buff *skb) >>> +{ >>> + struct psample_group psample_group = {}; >>> + struct psample_metadata md = {}; >>> + struct vport *input_vport; >>> + u32 rate; >>> + >>> + psample_group.group_num = arg->group_id; >>> + 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.user_cookie = arg->cookie_len ? &arg->cookie[0] : NULL; >>> + md.user_cookie_len = arg->cookie_len; >>> + md.trunc_size = skb->len; >>> + >>> + rate = arg->probability ? U32_MAX / arg->probability : 0; >>> + >>> + psample_sample_packet(&psample_group, skb, rate, &md); >> Does this mean now the ovs module, now is dependent on the presence >> of psample? I think we should only support sampling to psample if >> the module exists, else we should return an error. There might be >> distributions not including psample by default. > > Agree. I'll add some compile-time checks the same way we do with nf_nat. > >> >>> + >>> + return 0; >>> +} >>> + >>> /* When 'last' is true, sample() should always consume the 'skb'. >>> * Otherwise, sample() should keep 'skb' intact regardless what >>> * actions are executed within sample(). >>> @@ -1033,11 +1062,12 @@ static int sample(struct datapath *dp, struct >>> sk_buff *skb, >>> struct sw_flow_key *key, const struct nlattr *attr, >>> bool last) >>> { >>> - struct nlattr *actions; >>> + const struct sample_arg *arg; >>> struct nlattr *sample_arg; >>> int rem = nla_len(attr); >>> - const struct sample_arg *arg; >>> + struct nlattr *actions; >>> bool clone_flow_key; >>> + int ret; >>> >>> /* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */ >>> sample_arg = nla_data(attr); >>> @@ -1051,9 +1081,20 @@ static int sample(struct datapath *dp, struct >>> sk_buff *skb, >>> return 0; >>> } >>> >>> - clone_flow_key = !arg->exec; >>> - return clone_execute(dp, skb, key, 0, actions, rem, last, >>> - clone_flow_key); >>> + if (arg->flags & OVS_SAMPLE_ARG_FLAG_PSAMPLE) { >>> + ret = ovs_psample_packet(dp, key, arg, skb); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (nla_ok(actions, rem)) { >>> + clone_flow_key = !(arg->flags & OVS_SAMPLE_ARG_FLAG_EXEC); >>> + ret = clone_execute(dp, skb, key, 0, actions, rem, last, >>> + clone_flow_key); >>> + } else if (last) { >>> + ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION); >>> + } >>> + return ret; >>> } >>> >>> /* When 'last' is true, clone() should always consume the 'skb'. >>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c >>> index f224d9bcea5e..1a348d3905fc 100644 >>> --- a/net/openvswitch/flow_netlink.c >>> +++ b/net/openvswitch/flow_netlink.c >>> @@ -2561,6 +2561,9 @@ static int __ovs_nla_copy_actions(struct net *net, >>> const struct nlattr *attr, >>> u32 mpls_label_count, bool log, >>> u32 depth); >>> >>> +static int copy_action(const struct nlattr *from, >>> + struct sw_flow_actions **sfa, bool log); >>> + >>> static int validate_and_copy_sample(struct net *net, const struct nlattr >>> *attr, >>> const struct sw_flow_key *key, >>> struct sw_flow_actions **sfa, >>> @@ -2569,10 +2572,10 @@ static int validate_and_copy_sample(struct net >>> *net, const struct nlattr *attr, >>> u32 depth) >>> { >>> const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1]; >>> - const struct nlattr *probability, *actions; >>> + const struct nlattr *probability, *actions, *group, *cookie; >>> + struct sample_arg arg = {}; >>> const struct nlattr *a; >>> int rem, start, err; >>> - struct sample_arg arg; >>> >>> memset(attrs, 0, sizeof(attrs)); >>> nla_for_each_nested(a, attr, rem) { >>> @@ -2589,7 +2592,19 @@ static int validate_and_copy_sample(struct net *net, >>> const struct nlattr *attr, >>> return -EINVAL; >>> >>> actions = attrs[OVS_SAMPLE_ATTR_ACTIONS]; >>> - if (!actions || (nla_len(actions) && nla_len(actions) < NLA_HDRLEN)) >>> + if (actions && (!nla_len(actions) || nla_len(actions) < NLA_HDRLEN)) >>> + return -EINVAL; >>> + >>> + group = attrs[OVS_SAMPLE_ATTR_PSAMPLE_GROUP]; >>> + if (group && nla_len(group) != sizeof(u32)) >>> + return -EINVAL; >>> + >>> + cookie = attrs[OVS_SAMPLE_ATTR_PSAMPLE_COOKIE]; >>> + if (cookie && >>> + (!group || nla_len(cookie) > OVS_PSAMPLE_COOKIE_MAX_SIZE)) >>> + return -EINVAL; >>> + >>> + if (!group && !actions) >>> return -EINVAL; >>> >>> /* validation done, copy sample action. */ >>> @@ -2608,7 +2623,19 @@ static int validate_and_copy_sample(struct net *net, >>> const struct nlattr *attr, >>> * If the sample is the last action, it can always be excuted >>> * rather than deferred. >>> */ >>> - arg.exec = last || !actions_may_change_flow(actions); >>> + if (actions && (last || !actions_may_change_flow(actions))) >>> + arg.flags |= OVS_SAMPLE_ARG_FLAG_EXEC; >>> + >>> + if (group) { >>> + arg.flags |= OVS_SAMPLE_ARG_FLAG_PSAMPLE; >>> + arg.group_id = nla_get_u32(group); >>> + } >>> + >>> + if (cookie) { >>> + memcpy(&arg.cookie[0], nla_data(cookie), nla_len(cookie)); >>> + arg.cookie_len = nla_len(cookie); >>> + } >>> + >>> arg.probability = nla_get_u32(probability); >>> >>> err = ovs_nla_add_action(sfa, OVS_SAMPLE_ATTR_ARG, &arg, sizeof(arg), >>> @@ -2616,12 +2643,13 @@ static int validate_and_copy_sample(struct net >>> *net, const struct nlattr *attr, >>> if (err) >>> return err; >>> >>> - err = __ovs_nla_copy_actions(net, actions, key, sfa, >>> - eth_type, vlan_tci, mpls_label_count, log, >>> - depth + 1); >>> - >>> - if (err) >>> - return err; >>> + if (actions) { >>> + err = __ovs_nla_copy_actions(net, actions, key, sfa, >>> + eth_type, vlan_tci, >>> + mpls_label_count, log, depth + 1); >>> + if (err) >>> + return err; >>> + } >>> >>> add_nested_action_end(*sfa, start); >>> >>> @@ -3553,20 +3581,38 @@ static int sample_action_to_attr(const struct >>> nlattr *attr, >>> goto out; >>> } >>> >>> - ac_start = nla_nest_start_noflag(skb, OVS_SAMPLE_ATTR_ACTIONS); >>> - if (!ac_start) { >>> - err = -EMSGSIZE; >>> - goto out; >>> + if (arg->flags & OVS_SAMPLE_ARG_FLAG_PSAMPLE) { >>> + if (nla_put_u32(skb, OVS_SAMPLE_ATTR_PSAMPLE_GROUP, >>> + arg->group_id)) { >>> + err = -EMSGSIZE; >>> + goto out; >>> + } >>> + >>> + if (arg->cookie_len && >>> + nla_put(skb, OVS_SAMPLE_ATTR_PSAMPLE_COOKIE, >>> + arg->cookie_len, &arg->cookie[0])) { >>> + err = -EMSGSIZE; >>> + goto out; >>> + } >>> } >>> >>> - err = ovs_nla_put_actions(actions, rem, skb); >>> + if (nla_ok(actions, rem)) { >>> + ac_start = nla_nest_start_noflag(skb, OVS_SAMPLE_ATTR_ACTIONS); >>> + if (!ac_start) { >>> + err = -EMSGSIZE; >>> + goto out; >>> + } >>> + err = ovs_nla_put_actions(actions, rem, skb); >>> + } >>> >>> out: >>> if (err) { >>> - nla_nest_cancel(skb, ac_start); >>> + if (ac_start) >>> + nla_nest_cancel(skb, ac_start); >>> nla_nest_cancel(skb, start); >>> } else { >>> - nla_nest_end(skb, ac_start); >>> + if (ac_start) >>> + nla_nest_end(skb, ac_start); >>> nla_nest_end(skb, start); >>> } >>> >>> -- 2.44.0 >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev