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 <[email protected]>
> > ---
> > 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?
> > + }
> > +
> > + 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
