On 5/10/24 12:06 PM, Eelco Chaudron wrote:
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. //EelcoSigned-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.
Ack.
+ #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?
We can, the or is not exclusive, but I agree the comment might be misleading. I'll rephrase it.
+ /* 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?
I'll add one. Beter safe than sorry.
+ } + } + 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?
Probably. I'll try to rework it. Two of the arguments to compose_sample_action are now optional so having a pointer to the stack will not work unless we add flags to indicate whether they are valid or not. But maybe we can rework it by having something like the follwoing to avoid clogging the argument list;
struct sample_args {
struct user_action_cookie *upcall;
struct psample_args *psample;
bool has_psample;
bool has_upcall;
};
} /* 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
