On Mon, Jul 01, 2024 at 01:30:12PM GMT, Eelco Chaudron wrote: > > > On 28 Jun 2024, at 18:40, Adrián Moreno wrote: > > > On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote: > >> On 5 Jun 2024, at 22:23, Adrian Moreno wrote: > >> > >>> Use the newly added emit_sample to implement OpenFlow sample() actions > >>> with local sampling configuration. > >> > >> See some comments below. > >> > >> Cheers, > >> > >> Eelco > >> > >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> > >>> --- > >>> ofproto/ofproto-dpif-lsample.c | 17 ++++ > >>> ofproto/ofproto-dpif-lsample.h | 6 ++ > >>> ofproto/ofproto-dpif-xlate.c | 163 ++++++++++++++++++++++++--------- > >>> ofproto/ofproto-dpif-xlate.h | 3 +- > >>> ofproto/ofproto-dpif.c | 2 +- > >>> 5 files changed, 144 insertions(+), 47 deletions(-) > >>> > >>> diff --git a/ofproto/ofproto-dpif-lsample.c > >>> b/ofproto/ofproto-dpif-lsample.c > >>> index 7bdafac19..2c0b5da89 100644 > >>> --- a/ofproto/ofproto-dpif-lsample.c > >>> +++ b/ofproto/ofproto-dpif-lsample.c > >>> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample > >>> *lsample, > >>> return changed; > >>> } > >>> > >>> +/* Returns the group_id of the exporter with the given collector_set_id, > >>> if it > >>> + * exists. */ > >> > >> nit: The below will fit on one line > >> > >> /* Returns the exporter group_id for a given collector_set_id, if it > >> exists. */ > >> > > > > Ack. > > > >>> +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 c23ea8372..f13425575 100644 > >>> --- a/ofproto/ofproto-dpif-lsample.h > >>> +++ b/ofproto/ofproto-dpif-lsample.h > >>> @@ -19,6 +19,7 @@ > >>> > >>> #include <stdbool.h> > >>> #include <stdlib.h> > >>> +#include <stdint.h> > >> > >> Maybe add in alphabetical order. > >> > > > > Ack. > > > >>> struct dpif_lsample; > >>> struct ofproto_lsample_options; > >>> @@ -31,4 +32,9 @@ 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 */ > >>> + > >> > >> Accedantely added a newline? > >> > > > > Ack. > > > >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > >>> index 7c4950895..5bd215d31 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-lsample.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_lsample *lsample; /* Local sample 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_lsample *, > >>> 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_lsample *lsample, > >> > >> nit: I would move above netflow, as you also do the actual init/unref > >> before > >> netflow. > > > > Ack. > > > >>> 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->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); > >>> @@ -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->lsample, 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_lsample *lsample, > >> > >> nit: move before netflow. > >> > > > > Ack. > > > >>> 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, lsample, 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,6 +3369,11 @@ xlate_normal(struct xlate_ctx *ctx) > >>> } > >>> } > >>> > >>> +struct emit_sample_args { > >>> + uint32_t group_id; > >>> + struct ofpbuf cookie; > >>> +}; > >> > >> nit: I like the structs to be defined at the top of the file. Not sure what > >> the preferred place is. > >> > > > > Given this struct is only used to pass parameters to the function > > inmediately below, it seems kind of self-contained. But I agree at the > > top might also be OK. > > > >>> /* 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 +3387,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, > >> > >> Guess the parameter is still a cookie, so it makes not sense to rename it > >> to > >> upcall. If you want to add more meaning you could call it user_cookie. If > >> you > >> do you need to update the help text above. > >> > > > > I've refactored this part in the next version to reduce the amount of > > arguments and have a more readable argument generation. > > > >>> + const struct emit_sample_args *emit, > >>> const odp_port_t tunnel_out_port, > >>> bool include_actions) > >>> { > >>> @@ -3379,14 +3397,20 @@ compose_sample_action(struct xlate_ctx *ctx, > >>> return 0; > >>> } > >>> > >>> + /* At least one of upcall or emit_sample config must be provided. */ > >>> + ovs_assert(upcall || emit); > >>> + > >>> /* 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; > >>> + bool is_sample = (probability < UINT32_MAX || > >>> + (upcall && meter_id != UINT32_MAX)); > >> > >> Maybe update the help message a bit to be clear the meter is only for the > >> slow patch (userspace) action, and we also omit if sample for 100% > >> probability. > >> > > > > Ack. > > > >>> size_t sample_offset = 0, actions_offset = 0; > >>> if (is_sample) { > >>> sample_offset = nl_msg_start_nested(ctx->odp_actions, > >>> @@ -3397,19 +3421,26 @@ compose_sample_action(struct xlate_ctx *ctx, > >>> OVS_SAMPLE_ATTR_ACTIONS); > >>> } > >>> > >>> - if (meter_id != UINT32_MAX) { > >>> - nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, > >>> meter_id); > >>> + if (emit) { > >>> + odp_put_emit_sample_action(ctx->odp_actions, emit->group_id, > >>> + emit->cookie.data, emit->cookie.size); > >>> } > >>> > >>> - 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) { > >>> + if (upcall) { > >>> + 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, upcall, sizeof *upcall, > >>> + tunnel_out_port, > >>> include_actions, > >>> + ctx->odp_actions, > >>> &cookie_offset); > >>> + ovs_assert(res == 0); > >>> + } > >>> + > >>> + if (actions_offset) { > >> > >> Why is if (sample) no longer used, it makes more sense. > >> > > > > Sure, we can change this for "if (is_sample) {" > > > >>> nl_msg_end_nested(ctx->odp_actions, actions_offset); > >>> nl_msg_end_nested(ctx->odp_actions, sample_offset); > >>> } > >>> @@ -3440,7 +3471,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 +3521,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 +5878,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 +5902,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 +5936,70 @@ 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); > >> > >> Why does is the memset() removed from the new code? Are we sure we are > >> setting all the fields? > >> > >>> - 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) > >>> +{ > >>> + uint8_t cookie_buf[sizeof(os->obs_domain_id) + > >>> sizeof(os->obs_point_id)]; > >>> + struct dpif_lsample *lsample = ctx->xbridge->lsample; > >>> + struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; > >>> + struct emit_sample_args emit_sample_args; > >>> + struct user_action_cookie upcall_cookie; > >> > >> As the action is called userspace we might just want to call the cookie as > >> such for consistency, i.e. user_cookie/userspace_cookie. > >> > > > > This will change in next version and I think it will have better > > names... I said "I think" :-). > > > >>> + odp_port_t tunnel_out_port = ODPP_NONE; > >>> + bool emit_sample = false; > >>> + uint32_t group_id; > >>> + > >>> + if (!ipfix && !lsample) { > >>> + 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) { > >>> + xlate_fill_ipfix_sample(ctx, os, ipfix, &upcall_cookie, > >>> + &tunnel_out_port); > >>> + } > >> > >> New line here. > >> > > > > Ack. > > > >>> + if (lsample) { > >>> + if (dpif_lsample_get_group_id(lsample, > >>> + os->collector_set_id, > >>> + &group_id)) { > >> > >> Maybe just do &emit_sample_args.group_id, it saves the variable and > >> setting it > >> below. Also all the ofbuf_xx() function are in order ;) > >> > >>> + emit_sample = true; > >>> + ofpbuf_use_stub(&emit_sample_args.cookie, cookie_buf, > >>> + sizeof cookie_buf); > >>> + > >>> + emit_sample_args.group_id = group_id; > >>> + ofpbuf_put(&emit_sample_args.cookie, &os->obs_domain_id, > >>> + sizeof(os->obs_domain_id)); > >>> + ofpbuf_put(&emit_sample_args.cookie, &os->obs_point_id, > >>> + sizeof(os->obs_point_id)); > >> > >> Guess the endianness discussion needs to be fixed here. > >> > > > > Yes. > > > >>> + } > >> > >> Is the dpif_lsample_get_group_id() a common case? Do we need a log message > >> or > >> coverage counter? > >> > > > > You mean for samples for which we cannot find a group_id? > > Yes, the corner case.
dpif_lsample_get_group_id() will check if there is as Local Sampling entry associated with the current SAMPLE action. OTOH, Local Sampling and IPFIX can be configured simmultaneously with or without sharing the collector id. For instance: ovs-vsctl -- --id=@br0 get Bridge br0 \ -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \ -- create Flow_Sample_Collector_Set id=1 bridge=@br0 ipfix=@ipfix \ -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12], With this config, all actions such as the following ... actions:...,sample(sample=123,collector_set_id=1) ... will trigger this: lsample will be non-NULL (it is configured with local_group_id=12 for collector_set_id=2), but we will fail to find a valid group_id for collector_set_id 1. Not sure how rare this configuration might be (for someone starting to use local sampling on an already IPFIX-enabled system it doesn't look bad to me). But when configured this way, the lookup failure can happen quite often (on every sample action translation) and it doesn't seem to be relevant enough for a coverage counter. Thanks. Adrián > > >>> + } > >>> + > >>> + if (!ipfix && !emit_sample) { > >>> + return; > >>> + } > >>> + > >>> + compose_sample_action(ctx, probability, ipfix ? &upcall_cookie : > >>> NULL, > >>> + emit_sample ? &emit_sample_args : NULL, > >>> + tunnel_out_port, false); > >>> + > >>> + if (emit_sample) { > >>> + ofpbuf_uninit(&emit_sample_args.cookie); > >>> + } > >>> } > >>> > >>> /* 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..164ff3713 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_lsample *, bool forward_bpdu, > >> > >> Maybe move it after dpif_sflow to be consistent. > >> > > > > Ack. > > > >>> + 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 067f60df3..48167751f 100644 > >>> --- a/ofproto/ofproto-dpif.c > >>> +++ b/ofproto/ofproto-dpif.c > >>> @@ -485,7 +485,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->lsample, > >>> ofproto->up.forward_bpdu, > >>> connmgr_has_in_band(ofproto->up.connmgr), > >>> &ofproto->backer->rt_support); > >>> -- > >>> 2.45.1 > >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev