On 24 Apr 2024, at 21:53, Adrian Moreno wrote: > When a OFP_SAMPLE action is xlated and a dpif_psample object has been > configured (via Flow_Sample_Collector_Set table) with the same > collector_set_id, add psample information to the odp sample action.
See comments below. //Eelco > Signed-off-by: Adrian Moreno <[email protected]> > --- > ofproto/ofproto-dpif-psample.c | 20 +++++ > ofproto/ofproto-dpif-psample.h | 3 + > ofproto/ofproto-dpif-xlate.c | 149 +++++++++++++++++++++++++-------- > ofproto/ofproto-dpif-xlate.h | 3 +- > ofproto/ofproto-dpif.c | 2 +- > 5 files changed, 138 insertions(+), 39 deletions(-) > > diff --git a/ofproto/ofproto-dpif-psample.c b/ofproto/ofproto-dpif-psample.c > index a83530ed8..1e4f4bf48 100644 > --- a/ofproto/ofproto-dpif-psample.c > +++ b/ofproto/ofproto-dpif-psample.c > @@ -125,6 +125,26 @@ OVS_EXCLUDED(mutex) > return changed; > } > > +/* Returns the group_id of the exporter with the given collector_set_id, if > it > + * exists. */ > +bool > +dpif_psample_get_group_id(struct dpif_psample *ps, uint32_t collector_set_id, > + uint32_t *group_id) OVS_EXCLUDED(mutex) > +{ > + > + struct psample_exporter_map_node *node; > + bool found = false; > + > + ovs_mutex_lock(&mutex); > + node = dpif_psample_find_exporter_node(ps, collector_set_id); > + if (node) { > + found = true; > + *group_id = node->exporter.group_id; > + } > + ovs_mutex_unlock(&mutex); > + return found; > +} > + > /* Creation and destruction. */ > struct dpif_psample * > dpif_psample_create(void) > diff --git a/ofproto/ofproto-dpif-psample.h b/ofproto/ofproto-dpif-psample.h > index 80ba44fb9..b9f2584af 100644 > --- a/ofproto/ofproto-dpif-psample.h > +++ b/ofproto/ofproto-dpif-psample.h > @@ -18,6 +18,7 @@ > #define OFPROTO_DPIF_PSAMPLE_H 1 > > #include <stdbool.h> > +#include <stdint.h> > > struct dpif_psample; > struct ovs_list; > @@ -28,4 +29,6 @@ struct dpif_psample* dpif_psample_ref(const struct > dpif_psample *); > > bool dpif_psample_set_options(struct dpif_psample *, const struct ovs_list > *); > > +bool dpif_psample_get_group_id(struct dpif_psample *, uint32_t, uint32_t *); No idea what we are passing for the last two arguments, so we should include the name. > + > #endif // OFPROTO_DPIF_PSAMPLE_H > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 7c4950895..1dcf86856 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -47,6 +47,7 @@ > #include "ofproto/ofproto-dpif-ipfix.h" > #include "ofproto/ofproto-dpif-mirror.h" > #include "ofproto/ofproto-dpif-monitor.h" > +#include "ofproto/ofproto-dpif-psample.h" > #include "ofproto/ofproto-dpif-sflow.h" > #include "ofproto/ofproto-dpif-trace.h" > #include "ofproto/ofproto-dpif-xlate-cache.h" > @@ -117,6 +118,7 @@ struct xbridge { > struct dpif_sflow *sflow; /* SFlow handle, or null. */ > struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */ > struct netflow *netflow; /* Netflow handle, or null. */ > + struct dpif_psample *psample; /* Psample handle, or null. */ > struct stp *stp; /* STP or null if disabled. */ > struct rstp *rstp; /* RSTP or null if disabled. */ > > @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct > dpif *, > const struct dpif_sflow *, > const struct dpif_ipfix *, > const struct netflow *, > + const struct dpif_psample *, > bool forward_bpdu, bool has_in_band, > const struct dpif_backer_support *, > const struct xbridge_addr *); > @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge, > const struct dpif_sflow *sflow, > const struct dpif_ipfix *ipfix, > const struct netflow *netflow, > + const struct dpif_psample *psample, > bool forward_bpdu, bool has_in_band, > const struct dpif_backer_support *support, > const struct xbridge_addr *addr) > @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge, > xbridge->ipfix = dpif_ipfix_ref(ipfix); > } > > + if (xbridge->psample != psample) { > + dpif_psample_unref(xbridge->psample); > + xbridge->psample = dpif_psample_ref(psample); > + } > + > if (xbridge->stp != stp) { > stp_unref(xbridge->stp); > xbridge->stp = stp_ref(stp); > @@ -1214,8 +1223,9 @@ xlate_xbridge_copy(struct xbridge *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->psample, 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); > } > @@ -1373,6 +1383,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const > char *name, > const struct dpif_sflow *sflow, > const struct dpif_ipfix *ipfix, > const struct netflow *netflow, > + const struct dpif_psample *psample, > 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, > + netflow, psample, forward_bpdu, has_in_band, support, > xbridge_addr); > > if (xbridge_addr != old_addr) { > @@ -3357,6 +3368,11 @@ xlate_normal(struct xlate_ctx *ctx) > } > } > > +struct psample_args { > + uint32_t group_id; > + struct ofpbuf cookie; > +}; > + > /* 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. > @@ -3370,7 +3386,8 @@ xlate_normal(struct xlate_ctx *ctx) > static size_t > compose_sample_action(struct xlate_ctx *ctx, > const uint32_t probability, > - const struct user_action_cookie *cookie, > + const struct user_action_cookie *upcall, > + const struct psample_args *psample, > const odp_port_t tunnel_out_port, > bool include_actions) > { > @@ -3379,6 +3396,9 @@ compose_sample_action(struct xlate_ctx *ctx, > return 0; > } > > + /* Either upcall or psample data must be provided */ > + ovs_assert(upcall || psample); Why? Can’t we support both to be present? > + > /* 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; > @@ -3386,15 +3406,31 @@ compose_sample_action(struct xlate_ctx *ctx, > > /* When meter action is not required, avoid generate sample action > * for 100% sampling rate. */ > - bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX; > + bool is_sample = (probability < UINT32_MAX || meter_id != UINT32_MAX || > + psample); > 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); > - actions_offset = nl_msg_start_nested(ctx->odp_actions, > + if (psample) { > + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PSAMPLE_GROUP, > + psample->group_id); > + if (psample->cookie.size) { > + nl_msg_put_unspec(ctx->odp_actions, > + OVS_SAMPLE_ATTR_PSAMPLE_COOKIE, > + psample->cookie.data, > + psample->cookie.size); Do we need any max cookie check? > + } > + } > + if (upcall) { > + actions_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_SAMPLE_ATTR_ACTIONS); > + } else { > + nl_msg_end_nested(ctx->odp_actions, sample_offset); > + return 0; > + } > } > > if (meter_id != UINT32_MAX) { > @@ -3405,11 +3441,11 @@ compose_sample_action(struct xlate_ctx *ctx, > 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, > + int res = odp_put_userspace_action(pid, upcall, sizeof *upcall, > tunnel_out_port, include_actions, > ctx->odp_actions, &cookie_offset); > ovs_assert(res == 0); > - if (is_sample) { > + if (actions_offset) { > nl_msg_end_nested(ctx->odp_actions, actions_offset); > nl_msg_end_nested(ctx->odp_actions, sample_offset); > } > @@ -3440,7 +3476,7 @@ compose_sflow_action(struct xlate_ctx *ctx) > cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > > return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), > - &cookie, ODPP_NONE, true); > + &cookie, NULL, ODPP_NONE, true); > } > > /* If flow IPFIX is enabled, make sure IPFIX flow sample action > @@ -3490,7 +3526,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t > output_odp_port) > > compose_sample_action(ctx, > dpif_ipfix_get_bridge_exporter_probability(ipfix), > - &cookie, tunnel_out_port, false); > + &cookie, NULL, tunnel_out_port, false); > } > > /* Fix "sample" action according to data collected while composing ODP > actions, > @@ -5847,23 +5883,15 @@ xlate_fin_timeout(struct xlate_ctx *ctx, > } > > static void > -xlate_sample_action(struct xlate_ctx *ctx, > - const struct ofpact_sample *os) > +xlate_fill_ipfix_sample(struct xlate_ctx *ctx, > + const struct ofpact_sample *os, > + const struct dpif_ipfix *ipfix, > + struct user_action_cookie *cookie, > + odp_port_t *tunnel_out_port) > { > odp_port_t output_odp_port = ODPP_NONE; > - odp_port_t tunnel_out_port = ODPP_NONE; > - struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; > bool emit_set_tunnel = false; > > - if (!ipfix) { > - return; > - } > - > - /* Scale the probability from 16-bit to 32-bit while representing > - * the same percentage. */ > - uint32_t probability = > - ((uint32_t) os->probability << 16) | os->probability; > - > /* If ofp_port in flow sample action is equel to ofp_port, > * this sample action is a input port action. */ > if (os->sampling_port != OFPP_NONE && > @@ -5879,7 +5907,7 @@ xlate_sample_action(struct xlate_ctx *ctx, > if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix, > > os->collector_set_id) > && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) { > - tunnel_out_port = output_odp_port; > + *tunnel_out_port = output_odp_port; > emit_set_tunnel = true; > } > } > @@ -5913,20 +5941,67 @@ xlate_sample_action(struct xlate_ctx *ctx, > } > } > > - struct user_action_cookie cookie; > + cookie->type = USER_ACTION_COOKIE_FLOW_SAMPLE; > + cookie->ofp_in_port = ctx->xin->flow.in_port.ofp_port; > + cookie->ofproto_uuid = ctx->xbridge->ofproto->uuid; > + cookie->flow_sample.probability = os->probability; > + cookie->flow_sample.collector_set_id = os->collector_set_id; > + cookie->flow_sample.obs_domain_id = os->obs_domain_id; > + cookie->flow_sample.obs_point_id = os->obs_point_id; > + cookie->flow_sample.output_odp_port = output_odp_port; > + cookie->flow_sample.direction = os->direction; > +} > > - memset(&cookie, 0, sizeof cookie); > - cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE; > - cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > - cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > - cookie.flow_sample.probability = os->probability; > - cookie.flow_sample.collector_set_id = os->collector_set_id; > - cookie.flow_sample.obs_domain_id = os->obs_domain_id; > - cookie.flow_sample.obs_point_id = os->obs_point_id; > - cookie.flow_sample.output_odp_port = output_odp_port; > - cookie.flow_sample.direction = os->direction; > - > - compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false); > +static void > +xlate_sample_action(struct xlate_ctx *ctx, > + const struct ofpact_sample *os) > +{ > + struct dpif_psample *psample = ctx->xbridge->psample; > + struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; > + struct user_action_cookie *upcall_cookie = NULL; > + struct psample_args *psample_args = NULL; > + odp_port_t tunnel_out_port = ODPP_NONE; > + uint32_t group_id; > + > + if (!ipfix && !psample) { > + return; > + } > + > + /* Scale the probability from 16-bit to 32-bit while representing > + * the same percentage. */ > + uint32_t probability = > + ((uint32_t) os->probability << 16) | os->probability; > + > + if (ipfix) { > + upcall_cookie = xzalloc(sizeof *upcall_cookie); > + xlate_fill_ipfix_sample(ctx, os, ipfix, upcall_cookie, > + &tunnel_out_port); > + } > + if (psample) { > + if (dpif_psample_get_group_id(psample, See earlier lock contention concern on taking the mutex() for parallel upcalls. > + os->collector_set_id, > + &group_id)) { > + psample_args = xzalloc(sizeof *psample_args); > + ofpbuf_init(&psample_args->cookie, OVS_PSAMPLE_COOKIE_MAX_SIZE); > + > + psample_args->group_id = group_id; > + ofpbuf_put(&psample_args->cookie, &os->obs_domain_id, > + sizeof(os->obs_domain_id)); > + ofpbuf_put(&psample_args->cookie, &os->obs_point_id, > + sizeof(os->obs_point_id)); > + } > + } > + > + compose_sample_action(ctx, probability, upcall_cookie, psample_args, > + tunnel_out_port, false); > + > + if (upcall_cookie) { > + free(upcall_cookie); > + } > + if (psample_args) { > + ofpbuf_uninit(&psample_args->cookie); > + free(psample_args); > + } Can we avoid these small memory allocations? > } > > /* Determine if an datapath action translated from the openflow action > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > index 05b46fb26..a2835082b 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -177,7 +177,8 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char > *name, struct dpif *, > struct rstp *, const struct mcast_snooping *, > const struct mbridge *, const struct dpif_sflow *, > const struct dpif_ipfix *, const struct netflow *, > - bool forward_bpdu, bool has_in_band, > + const struct dpif_psample *, bool forward_bpdu, > + bool has_in_band, > const struct dpif_backer_support *support); > void xlate_remove_ofproto(struct ofproto_dpif *); > struct ofproto_dpif *xlate_ofproto_lookup(const struct uuid *uuid); > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index fbb83a9ec..64c06322e 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -486,7 +486,7 @@ type_run(const char *type) > ofproto->backer->dpif, ofproto->ml, > ofproto->stp, ofproto->rstp, ofproto->ms, > ofproto->mbridge, ofproto->sflow, > ofproto->ipfix, > - ofproto->netflow, > + ofproto->netflow, ofproto->psample, > ofproto->up.forward_bpdu, > connmgr_has_in_band(ofproto->up.connmgr), > &ofproto->backer->rt_support); > -- > 2.44.0 < _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
