On 7/10/24 15:25, Adrián Moreno wrote: > On Wed, Jul 10, 2024 at 02:56:44PM GMT, Ilya Maximets wrote: >> On 7/7/24 22:08, 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 <amore...@redhat.com> >>> --- >>> ofproto/ofproto-dpif-lsample.c | 16 +++ >>> ofproto/ofproto-dpif-lsample.h | 5 + >>> ofproto/ofproto-dpif-xlate.c | 251 +++++++++++++++++++++++---------- >>> ofproto/ofproto-dpif-xlate.h | 5 +- >>> ofproto/ofproto-dpif.c | 2 +- >>> tests/ofproto-dpif.at | 146 +++++++++++++++++++ >>> 6 files changed, 345 insertions(+), 80 deletions(-) >>> >>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c >>> index d675a116f..534ad96f0 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 bee36c9c5..9c1026551 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; >>> @@ -33,4 +34,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..5e8113d5e 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. */ >>> >>> @@ -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,92 @@ 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. */ >>> + struct ofpbuf 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; >>> + >>> + /* 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)); >>> >>> - /* When meter action is not required, avoid generate sample action >>> - * for 100% sampling rate. */ >>> - bool is_sample = probability < UINT32_MAX || 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, >>> + args->psample->cookie.data, >>> + args->psample->cookie.size); >>> + } >>> + >>> + 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 +3474,23 @@ 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 = {}; >>> + >>> if (!sflow || ctx->xin->flow.in_port.ofp_port == OFPP_NONE) { >>> return 0; >>> } >>> >>> - struct user_action_cookie cookie; >>> + 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,10 @@ 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 = {0}; >>> + struct compose_sample_args args = {0}; >>> + >>> + userspace.tunnel_out_port = ODPP_NONE; >>> >>> if (!ipfix || >>> (output_odp_port == ODPP_NONE && >>> @@ -3476,21 +3529,20 @@ compose_ipfix_action(struct xlate_ctx *ctx, >>> odp_port_t output_odp_port) >>> */ >>> if (dpif_ipfix_get_bridge_exporter_tunnel_sampling(ipfix) && >>> dpif_ipfix_is_tunnel_port(ipfix, output_odp_port) ) { >>> - tunnel_out_port = output_odp_port; >>> + userspace.tunnel_out_port = output_odp_port; >>> } >>> } >>> >>> - struct user_action_cookie cookie; >>> + userspace.cookie.type = USER_ACTION_COOKIE_IPFIX; >>> + userspace.cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; >>> + userspace.cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; >>> + userspace.cookie.ipfix.output_odp_port = output_odp_port; >>> + userspace.include_actions = false; >>> >>> - memset(&cookie, 0, sizeof cookie); >>> - cookie.type = USER_ACTION_COOKIE_IPFIX; >>> - cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; >>> - cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; >>> - cookie.ipfix.output_odp_port = output_odp_port; >>> + args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix); >>> + args.userspace = &userspace; >>> >>> - compose_sample_action(ctx, >>> - >>> dpif_ipfix_get_bridge_exporter_probability(ipfix), >>> - &cookie, tunnel_out_port, false); >>> + compose_sample_action(ctx, &args); >>> } >>> >>> /* Fix "sample" action according to data collected while composing ODP >>> actions, >>> @@ -5847,22 +5899,16 @@ 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 sample_userspace_args *userspace) >>> { >>> 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; >>> + memset(userspace, 0, sizeof *userspace); >>> + userspace->tunnel_out_port = ODPP_NONE; >>> >>> /* If ofp_port in flow sample action is equel to ofp_port, >>> * this sample action is a input port action. */ >>> @@ -5879,7 +5925,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; >>> + userspace->tunnel_out_port = output_odp_port; >>> emit_set_tunnel = true; >>> } >>> } >>> @@ -5913,20 +5959,71 @@ xlate_sample_action(struct xlate_ctx *ctx, >>> } >>> } >>> >>> - struct user_action_cookie cookie; >>> + userspace->cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE; >>> + userspace->cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; >>> + userspace->cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; >>> + userspace->cookie.flow_sample.probability = os->probability; >>> + userspace->cookie.flow_sample.collector_set_id = os->collector_set_id; >>> + userspace->cookie.flow_sample.obs_domain_id = os->obs_domain_id; >>> + userspace->cookie.flow_sample.obs_point_id = os->obs_point_id; >>> + userspace->cookie.flow_sample.output_odp_port = output_odp_port; >>> + userspace->cookie.flow_sample.direction = os->direction; >>> + userspace->include_actions = false; >>> +} >>> >>> - 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) >>> +{ >>> + 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 compose_sample_args compose_args = {}; >>> + struct sample_userspace_args userspace; >>> + struct sample_psample_args psample; >>> + >>> + if (!ipfix && !lsample) { >>> + return; >>> + } >>> + >>> + /* Scale the probability from 16-bit to 32-bit while representing >>> + * the same percentage. */ >>> + compose_args.probability = >>> + ((uint32_t) os->probability << 16) | os->probability; >>> + >>> + if (ipfix) { >>> + xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace); >>> + compose_args.userspace = &userspace; >>> + } >>> + >>> + if (lsample && >>> + dpif_lsample_get_group_id(lsample, >>> + os->collector_set_id, >>> + &psample.group_id)) { >> >> This part will have to be re-worked if we'll add a different >> local smapling action in the userspace datapath, for example. >> >> I think, we should check which datapath action is supported here >> and bail. We shouldn't deligate desicion making to odp-util. > > If the datapath action is not supported, "ofproto_set_local_sample" will > fail and xbridge->psample will be NULL, right?
True. But this code assumes that lsample is always implemented via psample. Maybe it's not a problem. I guess, this can be changed later if we'll need a different implementation. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev