On 12 Jul 2024, at 1:32, Adrian Moreno wrote: > Use the newly added psample action to implement OpenFlow sample() actions > with local sampling configuration if possible. > > A bit of refactoring in compose_sample_actions arguments helps make it a > bit more readable. > > Signed-off-by: Adrian Moreno <[email protected]>
The patch looks good to me. I do have one comment based on Ilya's earlier feedback. Regardless of the resolution, I'm okay with it, so I'll add my ack. Acked-by: Eelco Chaudron <[email protected]> > --- > ofproto/ofproto-dpif-lsample.c | 16 +++ > ofproto/ofproto-dpif-lsample.h | 5 + > ofproto/ofproto-dpif-xlate.c | 238 ++++++++++++++++++++++----------- > ofproto/ofproto-dpif-xlate.h | 5 +- > ofproto/ofproto-dpif.c | 2 +- > tests/ofproto-dpif.at | 159 ++++++++++++++++++++++ > 6 files changed, 345 insertions(+), 80 deletions(-) > > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c > index a2b2e8059..833ce923f 100644 > --- a/ofproto/ofproto-dpif-lsample.c > +++ b/ofproto/ofproto-dpif-lsample.c > @@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample, > return changed; > } > > +/* Returns the group_id for a given collector_set_id, if it exists. */ > +bool > +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id, > + uint32_t *group_id) > +{ > + struct lsample_exporter_node *node; > + bool found = false; > + > + node = dpif_lsample_find_exporter_node(ps, collector_set_id); > + if (node) { > + found = true; > + *group_id = node->exporter.options.group_id; > + } > + return found; > +} > + > struct dpif_lsample * > dpif_lsample_create(void) > { > diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h > index a491c137d..26517a645 100644 > --- a/ofproto/ofproto-dpif-lsample.h > +++ b/ofproto/ofproto-dpif-lsample.h > @@ -18,6 +18,7 @@ > #define OFPROTO_DPIF_LSAMPLE_H 1 > > #include <stdbool.h> > +#include <stdint.h> > #include <stdlib.h> > > struct dpif_lsample; > @@ -32,4 +33,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *, > const struct ofproto_lsample_options *, > size_t n_opts); > > +bool dpif_lsample_get_group_id(struct dpif_lsample *, > + uint32_t collector_set_id, > + uint32_t *group_id); > + > #endif /* OFPROTO_DPIF_LSAMPLE_H */ > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 7c4950895..9f32982b0 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -45,6 +45,7 @@ > #include "nx-match.h" > #include "odp-execute.h" > #include "ofproto/ofproto-dpif-ipfix.h" > +#include "ofproto/ofproto-dpif-lsample.h" > #include "ofproto/ofproto-dpif-mirror.h" > #include "ofproto/ofproto-dpif-monitor.h" > #include "ofproto/ofproto-dpif-sflow.h" > @@ -116,6 +117,7 @@ struct xbridge { > struct mbridge *mbridge; /* Mirroring. */ > struct dpif_sflow *sflow; /* SFlow handle, or null. */ > struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */ > + struct dpif_lsample *lsample; /* Local sample handle, or null. */ > struct netflow *netflow; /* Netflow handle, or null. */ > struct stp *stp; /* STP or null if disabled. */ > struct rstp *rstp; /* RSTP or null if disabled. */ > @@ -686,6 +688,7 @@ static void xlate_xbridge_set(struct xbridge *, struct > dpif *, > const struct mbridge *, > const struct dpif_sflow *, > const struct dpif_ipfix *, > + const struct dpif_lsample *, > const struct netflow *, > bool forward_bpdu, bool has_in_band, > const struct dpif_backer_support *, > @@ -1069,6 +1072,7 @@ xlate_xbridge_set(struct xbridge *xbridge, > const struct mbridge *mbridge, > const struct dpif_sflow *sflow, > const struct dpif_ipfix *ipfix, > + const struct dpif_lsample *lsample, > const struct netflow *netflow, > bool forward_bpdu, bool has_in_band, > const struct dpif_backer_support *support, > @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge, > xbridge->ipfix = dpif_ipfix_ref(ipfix); > } > > + if (xbridge->lsample != lsample) { > + dpif_lsample_unref(xbridge->lsample); > + xbridge->lsample = dpif_lsample_ref(lsample); > + } > + > if (xbridge->stp != stp) { > stp_unref(xbridge->stp); > xbridge->stp = stp_ref(stp); > @@ -1213,9 +1222,10 @@ xlate_xbridge_copy(struct xbridge *xbridge) > xlate_xbridge_set(new_xbridge, > xbridge->dpif, xbridge->ml, xbridge->stp, > xbridge->rstp, xbridge->ms, xbridge->mbridge, > - xbridge->sflow, xbridge->ipfix, xbridge->netflow, > - xbridge->forward_bpdu, xbridge->has_in_band, > - &xbridge->support, xbridge->addr); > + xbridge->sflow, xbridge->ipfix, xbridge->lsample, > + xbridge->netflow, xbridge->forward_bpdu, > + xbridge->has_in_band, &xbridge->support, > + xbridge->addr); > LIST_FOR_EACH (xbundle, list_node, &xbridge->xbundles) { > xlate_xbundle_copy(new_xbridge, xbundle); > } > @@ -1372,6 +1382,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const > char *name, > const struct mbridge *mbridge, > const struct dpif_sflow *sflow, > const struct dpif_ipfix *ipfix, > + const struct dpif_lsample *lsample, > const struct netflow *netflow, > bool forward_bpdu, bool has_in_band, > const struct dpif_backer_support *support) > @@ -1396,7 +1407,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const > char *name, > old_addr = xbridge->addr; > > xlate_xbridge_set(xbridge, dpif, ml, stp, rstp, ms, mbridge, sflow, > ipfix, > - netflow, forward_bpdu, has_in_band, support, > + lsample, netflow, forward_bpdu, has_in_band, support, > xbridge_addr); > > if (xbridge_addr != old_addr) { > @@ -1428,6 +1439,7 @@ xlate_xbridge_remove(struct xlate_cfg *xcfg, struct > xbridge *xbridge) > mbridge_unref(xbridge->mbridge); > dpif_sflow_unref(xbridge->sflow); > dpif_ipfix_unref(xbridge->ipfix); > + dpif_lsample_unref(xbridge->lsample); > netflow_unref(xbridge->netflow); > stp_unref(xbridge->stp); > rstp_unref(xbridge->rstp); > @@ -3357,58 +3369,91 @@ xlate_normal(struct xlate_ctx *ctx) > } > } > > -/* Appends a "sample" action for sFlow or IPFIX to 'ctx->odp_actions'. The > - * 'probability' is the number of packets out of UINT32_MAX to sample. The > - * 'cookie' is passed back in the callback for each sampled packet. > - * 'tunnel_out_port', if not ODPP_NONE, is added as the > - * OVS_USERSPACE_ATTR_EGRESS_TUN_PORT attribute. If 'include_actions', > - * an OVS_USERSPACE_ATTR_ACTIONS attribute is added. If > - * 'emit_set_tunnel', sample(sampling_port=1) would translate into > - * datapath sample action set(tunnel(...)), sample(...) and it is used > - * for sampling egress tunnel information. > - */ > +/* Psample-related arguments for compose_sample_action. */ > +struct sample_psample_args { > + uint32_t group_id; /* Group to be used in psample. */ > + ovs_32aligned_be64 cookie; /* Cookie to be used in psample. */ > +}; > + > +/* Userspace-related arguments for compose_sample_action. */ > +struct sample_userspace_args { > + struct user_action_cookie cookie; /* Data passed back in the upcall > + * for each sampled packet. */ > + odp_port_t tunnel_out_port; /* If not ODPP_NONE, it is added in > + * OVS_USERSPACE_ATTR_EGRESS_TUN_PORT > + * attribute. */ > + bool include_actions; /* Whether OVS_USERSPACE_ATTR_ACTIONS > + * is to be set. */ > + > +}; > + > +/* Arguments for compose_sample_action. */ > +struct compose_sample_args { > + uint32_t probability; /* Number of packets out of > + * UINT32_MAX to sample. */ > + struct sample_userspace_args *userspace; /* Optional, > + * arguments for userspace. */ > + struct sample_psample_args *psample; /* Optional, > + * arguments for psample. */ > +}; > + > +/* Composes sample action according to 'args'. */ > static size_t > compose_sample_action(struct xlate_ctx *ctx, > - const uint32_t probability, > - const struct user_action_cookie *cookie, > - const odp_port_t tunnel_out_port, > - bool include_actions) > + const struct compose_sample_args *args) > { > - if (probability == 0) { > + if (args->probability == 0) { > /* No need to generate sampling or the inner action. */ > return 0; > } > > + /* At least one of userspace or psample config must be provided. */ > + ovs_assert(args->userspace || args->psample); > + > /* If the slow path meter is configured by the controller, > * insert a meter action before the user space action. */ > struct ofproto *ofproto = &ctx->xin->ofproto->up; > uint32_t meter_id = ofproto->slowpath_meter_id; > + size_t cookie_offset = 0; > > - /* When meter action is not required, avoid generate sample action > - * for 100% sampling rate. */ > - bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX; > + /* The meter action is only used to throttle userspace actions. > + * If they are not needed and the sampling rate is 100%, avoid generating > + * a sample action. */ > + bool is_sample = (args->probability < UINT32_MAX || > + (args->userspace && meter_id != UINT32_MAX)); > size_t sample_offset = 0, actions_offset = 0; > if (is_sample) { > sample_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_ACTION_ATTR_SAMPLE); > nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, > - probability); > + args->probability); > actions_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_SAMPLE_ATTR_ACTIONS); > } > > - if (meter_id != UINT32_MAX) { > - nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id); > + if (args->psample) { > + odp_put_psample_action(ctx->odp_actions, > + args->psample->group_id, > + (void *) &args->psample->cookie, > + sizeof args->psample->cookie); > + } > + > + if (args->userspace) { > + if (meter_id != UINT32_MAX) { > + nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, > meter_id); > + } > + > + odp_port_t odp_port = ofp_port_to_odp_port( > + ctx->xbridge, ctx->xin->flow.in_port.ofp_port); > + uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); > + int res = odp_put_userspace_action(pid, &args->userspace->cookie, > + sizeof args->userspace->cookie, > + args->userspace->tunnel_out_port, > + args->userspace->include_actions, > + ctx->odp_actions, &cookie_offset); > + ovs_assert(res == 0); > } > > - odp_port_t odp_port = ofp_port_to_odp_port( > - ctx->xbridge, ctx->xin->flow.in_port.ofp_port); > - uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); > - size_t cookie_offset; > - int res = odp_put_userspace_action(pid, cookie, sizeof *cookie, > - tunnel_out_port, include_actions, > - ctx->odp_actions, &cookie_offset); > - ovs_assert(res == 0); > if (is_sample) { > nl_msg_end_nested(ctx->odp_actions, actions_offset); > nl_msg_end_nested(ctx->odp_actions, sample_offset); > @@ -3428,19 +3473,24 @@ static size_t > compose_sflow_action(struct xlate_ctx *ctx) > { > struct dpif_sflow *sflow = ctx->xbridge->sflow; > + struct sample_userspace_args userspace; > + struct compose_sample_args args = {}; Not sure what Ilya wanted here. He mentioned all instances, so maybe you also need a memset for args. Ilya? > + > if (!sflow || ctx->xin->flow.in_port.ofp_port == OFPP_NONE) { > return 0; > } > > - struct user_action_cookie cookie; > + memset(&userspace, 0, sizeof userspace); > + userspace.cookie.type = USER_ACTION_COOKIE_SFLOW; > + userspace.cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > + userspace.cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > + userspace.tunnel_out_port = ODPP_NONE; > + userspace.include_actions = true; > > - memset(&cookie, 0, sizeof cookie); > - cookie.type = USER_ACTION_COOKIE_SFLOW; > - cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > - cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > + args.probability = dpif_sflow_get_probability(sflow); > + args.userspace = &userspace; > > - return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), > - &cookie, ODPP_NONE, true); > + return compose_sample_action(ctx, &args); > } > > /* If flow IPFIX is enabled, make sure IPFIX flow sample action > @@ -3451,7 +3501,11 @@ static void > compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) > { > struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; > - odp_port_t tunnel_out_port = ODPP_NONE; > + struct sample_userspace_args userspace; > + struct compose_sample_args args = {}; Same as previous comment. > + > + memset(&userspace, 0, sizeof userspace); > + userspace.tunnel_out_port = ODPP_NONE; <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
