[ovs-dev,v1,06/13] ofproto-dpif-xlate: Use psample for local sample.

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 <amorenoz@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(-)

On 7 Jul 2024, at 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.

Some comments below.

Cheers,

Eelco

> Signed-off-by: Adrian Moreno <amorenoz@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"

Add in alphabetical order?

>  #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. */

I would move above netflow, as you also do the actual init/unref before netflow.

>      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));
>

This new line should be removed due to the way things got merged.

> -    /* 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};

Should both not be {}; same as the previous function?

> +
> +    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)) {
> +        ovs_be32 *data;
> +
> +        ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
> +
> +        data = ofpbuf_put_uninit(&psample.cookie,
> +                                 sizeof(os->obs_domain_id));
> +        *data = htonl(os->obs_domain_id);
> +
> +        data = ofpbuf_put_uninit(&psample.cookie,
> +                                 sizeof(os->obs_point_id));
> +        *data = htonl(os->obs_point_id);
> +
> +        compose_args.psample = &psample;
> +    }
> +
> +    if (!compose_args.userspace && !compose_args.psample) {
> +        return;
> +    }
> +
> +    compose_sample_action(ctx, &compose_args);
> +
> +    if (compose_args.psample) {
> +        ofpbuf_uninit(&compose_args.psample->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..08f9397d8 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -176,8 +176,9 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
>                         const struct mac_learning *, struct stp *,
>                         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_ipfix *, const struct dpif_lsample *,
> +                       const struct netflow *, 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 56830c630..006f67b01 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->lsample, ofproto->netflow,
>                                ofproto->up.forward_bpdu,
>                                connmgr_has_in_band(ofproto->up.connmgr),
>                                &ofproto->backer->rt_support);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0b23fd6c5..2a04977a7 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12136,3 +12136,149 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
>
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - not supported])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=10 \
> +                    -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=sample(probability=32767,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])

If we are only adding a single flow, no need to create a flows.txt file,
just add it here directly. But if you prefer to keep it as is, I’m ok.

> +m4_define([TRACE_PKT], [m4_join([,],
> +    [in_port(1)],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: 2
> +])
> +
> +OVS_VSWITCHD_STOP(["/ignoring local sampling configuration: not supported by this datapath/d"])

I think we should explicitly check for the log message in this test.

> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - sanity check])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=42],

If we break this up in multiple lines, maybe also do in <80 chars?

Guess this is a comment for all test cases below, we should truncate at <80
when possible.

> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),3
> +in_port=2, actions=sample(probability=32767,collector_set_id=20,obs_domain_id=100,obs_point_id=200),3
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +dnl collector_set_id does not match.
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
            [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: 3
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
            [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),3
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - with IPFIX])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +m4_define([EXPECTED_ACT], [m4_join([],
> +    [sample(sample=50.0%,actions(],
> +    [psample(group=42,cookie=0x64000000c8),],
> +    [userspace(pid=0,],
> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> +    [))),],
> +    [2],
> +)])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: EXPECTED_ACT
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - with metered IPFIX])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> +         [0], [ignore])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats bands=type=drop rate=2'])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +m4_define([EXPECTED_ACT], [m4_join([],
> +    [sample(sample=50.0%,actions(],
> +    [psample(group=42,cookie=0x64000000c8),],
> +    [meter(0),],
> +    [userspace(pid=0,],
> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> +    [))),],
> +    [2],
> +)])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: EXPECTED_ACT
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> -- 
> 2.45.2

On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 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.
>
> Some comments below.
>
> Cheers,
>
> Eelco
>
> > Signed-off-by: Adrian Moreno <amorenoz@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"
>
> Add in alphabetical order?

Ack.

>
> >  #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. */
>
> I would move above netflow, as you also do the actual init/unref before netflow.


Ack.


>
> >      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));
> >
>
> This new line should be removed due to the way things got merged.

Ack

>
> > -    /* 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};
>
> Should both not be {}; same as the previous function?
>

Funny you mention it. "{}" is actually not part of the C standard until
C23 gets published (which has not yet happened IIUC). It is an extension
introduced by C++ but pretty much supported by most C compilers. "{0}"
is the pure-C way of it.

I also dislike the inconsistency.
So I guess the correct way would be to write "{0}" in both cases.

More on this:
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm

> > +
> > +    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)) {
> > +        ovs_be32 *data;
> > +
> > +        ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
> > +
> > +        data = ofpbuf_put_uninit(&psample.cookie,
> > +                                 sizeof(os->obs_domain_id));
> > +        *data = htonl(os->obs_domain_id);
> > +
> > +        data = ofpbuf_put_uninit(&psample.cookie,
> > +                                 sizeof(os->obs_point_id));
> > +        *data = htonl(os->obs_point_id);
> > +
> > +        compose_args.psample = &psample;
> > +    }
> > +
> > +    if (!compose_args.userspace && !compose_args.psample) {
> > +        return;
> > +    }
> > +
> > +    compose_sample_action(ctx, &compose_args);
> > +
> > +    if (compose_args.psample) {
> > +        ofpbuf_uninit(&compose_args.psample->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..08f9397d8 100644
> > --- a/ofproto/ofproto-dpif-xlate.h
> > +++ b/ofproto/ofproto-dpif-xlate.h
> > @@ -176,8 +176,9 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
> >                         const struct mac_learning *, struct stp *,
> >                         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_ipfix *, const struct dpif_lsample *,
> > +                       const struct netflow *, 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 56830c630..006f67b01 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->lsample, ofproto->netflow,
> >                                ofproto->up.forward_bpdu,
> >                                connmgr_has_in_band(ofproto->up.connmgr),
> >                                &ofproto->backer->rt_support);
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 0b23fd6c5..2a04977a7 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -12136,3 +12136,149 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
> >
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - not supported])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=10 \
> > +                    -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1 actions=sample(probability=32767,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>
> If we are only adding a single flow, no need to create a flows.txt file,
> just add it here directly. But if you prefer to keep it as is, I’m ok.
>

Ack.

> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [in_port(1)],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: 2
> > +])
> > +
> > +OVS_VSWITCHD_STOP(["/ignoring local sampling configuration: not supported by this datapath/d"])
>
> I think we should explicitly check for the log message in this test.
>

I thought the macro would do that but I see it doesn't. Will change
that.

> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - sanity check])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2 3
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=42],
>
> If we break this up in multiple lines, maybe also do in <80 chars?
>
> Guess this is a comment for all test cases below, we should truncate at <80
> when possible.
>

Yeah, I've tried, defining macros when possible and so on, but flows
make it hard.

In this case I guess it should be easy to just write:

AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
                    -- create Flow_Sample_Collector_Set \
                        id=1 bridge=@br0 local_group_id=42],


> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),3
> > +in_port=2, actions=sample(probability=32767,collector_set_id=20,obs_domain_id=100,obs_point_id=200),3
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +dnl collector_set_id does not match.
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
>             [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: 3
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
>             [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),3
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - with IPFIX])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> > +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +m4_define([EXPECTED_ACT], [m4_join([],
> > +    [sample(sample=50.0%,actions(],
> > +    [psample(group=42,cookie=0x64000000c8),],
> > +    [userspace(pid=0,],
> > +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> > +    [))),],
> > +    [2],
> > +)])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: EXPECTED_ACT
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - with metered IPFIX])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> > +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> > +         [0], [ignore])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats bands=type=drop rate=2'])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +m4_define([EXPECTED_ACT], [m4_join([],
> > +    [sample(sample=50.0%,actions(],
> > +    [psample(group=42,cookie=0x64000000c8),],
> > +    [meter(0),],
> > +    [userspace(pid=0,],
> > +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> > +    [))),],
> > +    [2],
> > +)])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: EXPECTED_ACT
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > --
> > 2.45.2
>

On 9 Jul 2024, at 15:52, Adrián Moreno wrote:

> On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 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.
>>
>> Some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Adrian Moreno <amorenoz@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"
>>
>> Add in alphabetical order?
>
> Ack.
>
>>
>>>  #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. */
>>
>> I would move above netflow, as you also do the actual init/unref before netflow.
>
>
> Ack.
>
>
>>
>>>      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));
>>>
>>
>> This new line should be removed due to the way things got merged.
>
> Ack
>
>>
>>> -    /* 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};
>>
>> Should both not be {}; same as the previous function?
>>
>
> Funny you mention it. "{}" is actually not part of the C standard until
> C23 gets published (which has not yet happened IIUC). It is an extension
> introduced by C++ but pretty much supported by most C compilers. "{0}"
> is the pure-C way of it.
>
> I also dislike the inconsistency.
> So I guess the correct way would be to write "{0}" in both cases.
>
> More on this:
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm

Well, we used to have compiler problem with doing {0} as some compilers only initialized the first element or dump warnings, for some previous patches we went with {}. I’m assuming our compilers are all up to date now, as we get no warning from the robot, so we should be ok. Ilya any feedback on this?

>>> +
>>> +    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)) {
>>> +        ovs_be32 *data;
>>> +
>>> +        ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
>>> +
>>> +        data = ofpbuf_put_uninit(&psample.cookie,
>>> +                                 sizeof(os->obs_domain_id));
>>> +        *data = htonl(os->obs_domain_id);
>>> +
>>> +        data = ofpbuf_put_uninit(&psample.cookie,
>>> +                                 sizeof(os->obs_point_id));
>>> +        *data = htonl(os->obs_point_id);
>>> +
>>> +        compose_args.psample = &psample;
>>> +    }
>>> +
>>> +    if (!compose_args.userspace && !compose_args.psample) {
>>> +        return;
>>> +    }
>>> +
>>> +    compose_sample_action(ctx, &compose_args);
>>> +
>>> +    if (compose_args.psample) {
>>> +        ofpbuf_uninit(&compose_args.psample->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..08f9397d8 100644
>>> --- a/ofproto/ofproto-dpif-xlate.h
>>> +++ b/ofproto/ofproto-dpif-xlate.h
>>> @@ -176,8 +176,9 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
>>>                         const struct mac_learning *, struct stp *,
>>>                         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_ipfix *, const struct dpif_lsample *,
>>> +                       const struct netflow *, 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 56830c630..006f67b01 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->lsample, ofproto->netflow,
>>>                                ofproto->up.forward_bpdu,
>>>                                connmgr_has_in_band(ofproto->up.connmgr),
>>>                                &ofproto->backer->rt_support);
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 0b23fd6c5..2a04977a7 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -12136,3 +12136,149 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
>>>
>>>  OVS_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([ofproto-dpif - Local sampling - not supported])
>>> +OVS_VSWITCHD_START
>>> +add_of_ports br0 1 2
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=10 \
>>> +                    -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
>>> +         [0], [ignore])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=1 actions=sample(probability=32767,obs_domain_id=100,obs_point_id=200),2
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>>
>> If we are only adding a single flow, no need to create a flows.txt file,
>> just add it here directly. But if you prefer to keep it as is, I’m ok.
>>
>
> Ack.
>
>>> +m4_define([TRACE_PKT], [m4_join([,],
>>> +    [in_port(1)],
>>> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
>>> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
>>> +    [icmp(type=8,code=0)])])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'TRACE_PKT'], [0], [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: 2
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP(["/ignoring local sampling configuration: not supported by this datapath/d"])
>>
>> I think we should explicitly check for the log message in this test.
>>
>
> I thought the macro would do that but I see it doesn't. Will change
> that.
>
>>> +AT_CLEANUP
>>> +
>>> +AT_SETUP([ofproto-dpif - Local sampling - sanity check])
>>> +OVS_VSWITCHD_START
>>> +add_of_ports br0 1 2 3
>>> +
>>> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=42],
>>
>> If we break this up in multiple lines, maybe also do in <80 chars?
>>
>> Guess this is a comment for all test cases below, we should truncate at <80
>> when possible.
>>
>
> Yeah, I've tried, defining macros when possible and so on, but flows
> make it hard.
>
> In this case I guess it should be easy to just write:
>
> AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>                     -- create Flow_Sample_Collector_Set \
>                         id=1 bridge=@br0 local_group_id=42],

Yes that woul do do it. Guess we should not over do it, just for these simple check ones.

>>> +         [0], [ignore])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),3
>>> +in_port=2, actions=sample(probability=32767,collector_set_id=20,obs_domain_id=100,obs_point_id=200),3
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>>> +
>>> +m4_define([TRACE_PKT], [m4_join([,],
>>> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
>>> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
>>> +    [icmp(type=8,code=0)])])
>>> +
>>> +dnl collector_set_id does not match.
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
>>             [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: 3
>>> +])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
>>             [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),3
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP()
>>> +AT_CLEANUP
>>> +
>>> +AT_SETUP([ofproto-dpif - Local sampling - with IPFIX])
>>> +OVS_VSWITCHD_START
>>> +add_of_ports br0 1 2
>>> +
>>> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
>>> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
>>> +         [0], [ignore])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>>> +
>>> +m4_define([TRACE_PKT], [m4_join([,],
>>> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
>>> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
>>> +    [icmp(type=8,code=0)])])
>>> +
>>> +m4_define([EXPECTED_ACT], [m4_join([],
>>> +    [sample(sample=50.0%,actions(],
>>> +    [psample(group=42,cookie=0x64000000c8),],
>>> +    [userspace(pid=0,],
>>> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
>>> +    [))),],
>>> +    [2],
>>> +)])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: EXPECTED_ACT
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP()
>>> +AT_CLEANUP
>>> +
>>> +AT_SETUP([ofproto-dpif - Local sampling - with metered IPFIX])
>>> +OVS_VSWITCHD_START
>>> +add_of_ports br0 1 2
>>> +
>>> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
>>> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
>>> +         [0], [ignore])
>>> +
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats bands=type=drop rate=2'])
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
>>> +
>>> +m4_define([TRACE_PKT], [m4_join([,],
>>> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
>>> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
>>> +    [icmp(type=8,code=0)])])
>>> +
>>> +m4_define([EXPECTED_ACT], [m4_join([],
>>> +    [sample(sample=50.0%,actions(],
>>> +    [psample(group=42,cookie=0x64000000c8),],
>>> +    [meter(0),],
>>> +    [userspace(pid=0,],
>>> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
>>> +    [))),],
>>> +    [2],
>>> +)])
>>> +
>>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
>>> +AT_CHECK([tail -1 stdout], [0], [dnl
>>> +Datapath actions: EXPECTED_ACT
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP()
>>> +AT_CLEANUP
>>> --
>>> 2.45.2
>>

On 7/9/24 16:03, Eelco Chaudron wrote:
> 
> 
> On 9 Jul 2024, at 15:52, Adrián Moreno wrote:
> 
>> On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
>>> On 7 Jul 2024, at 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.
>>>
>>> Some comments below.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>> Signed-off-by: Adrian Moreno <amorenoz@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"
>>>
>>> Add in alphabetical order?
>>
>> Ack.
>>
>>>
>>>>  #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. */
>>>
>>> I would move above netflow, as you also do the actual init/unref before netflow.
>>
>>
>> Ack.
>>
>>
>>>
>>>>      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));
>>>>
>>>
>>> This new line should be removed due to the way things got merged.
>>
>> Ack
>>
>>>
>>>> -    /* 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};
>>>
>>> Should both not be {}; same as the previous function?
>>>
>>
>> Funny you mention it. "{}" is actually not part of the C standard until
>> C23 gets published (which has not yet happened IIUC). It is an extension
>> introduced by C++ but pretty much supported by most C compilers. "{0}"
>> is the pure-C way of it.
>>
>> I also dislike the inconsistency.
>> So I guess the correct way would be to write "{0}" in both cases.
>>
>> More on this:
>> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm
> 
> Well, we used to have compiler problem with doing {0} as some compilers only initialized the first element or dump warnings, for some previous patches we went with {}. I’m assuming our compilers are all up to date now, as we get no warning from the robot, so we should be ok. Ilya any feedback on this?

I think, we should keep the memset in all cases.  There are holes in
these structures and we're using memcmp for action list comparisons.

Best regards, Ilya Maximets.

On Wed, Jul 10, 2024 at 02:18:35PM GMT, Ilya Maximets wrote:
> On 7/9/24 16:03, Eelco Chaudron wrote:
> >
> >
> > On 9 Jul 2024, at 15:52, Adrián Moreno wrote:
> >
> >> On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
> >>> On 7 Jul 2024, at 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.
> >>>
> >>> Some comments below.
> >>>
> >>> Cheers,
> >>>
> >>> Eelco
> >>>
> >>>> Signed-off-by: Adrian Moreno <amorenoz@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"
> >>>
> >>> Add in alphabetical order?
> >>
> >> Ack.
> >>
> >>>
> >>>>  #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. */
> >>>
> >>> I would move above netflow, as you also do the actual init/unref before netflow.
> >>
> >>
> >> Ack.
> >>
> >>
> >>>
> >>>>      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));
> >>>>
> >>>
> >>> This new line should be removed due to the way things got merged.
> >>
> >> Ack
> >>
> >>>
> >>>> -    /* 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};
> >>>
> >>> Should both not be {}; same as the previous function?
> >>>
> >>
> >> Funny you mention it. "{}" is actually not part of the C standard until
> >> C23 gets published (which has not yet happened IIUC). It is an extension
> >> introduced by C++ but pretty much supported by most C compilers. "{0}"
> >> is the pure-C way of it.
> >>
> >> I also dislike the inconsistency.
> >> So I guess the correct way would be to write "{0}" in both cases.
> >>
> >> More on this:
> >> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm
> >
> > Well, we used to have compiler problem with doing {0} as some compilers only initialized the first element or dump warnings, for some previous patches we went with {}. I’m assuming our compilers are all up to date now, as we get no warning from the robot, so we should be ok. Ilya any feedback on this?
>
> I think, we should keep the memset in all cases.  There are holes in
> these structures and we're using memcmp for action list comparisons.
>

Ack. Will keep the memset for the userspace cookie. Thanks

> Best regards, Ilya Maximets.
>

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 <amorenoz@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.

> +        ovs_be32 *data;
> +
> +        ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
> +
> +        data = ofpbuf_put_uninit(&psample.cookie,
> +                                 sizeof(os->obs_domain_id));
> +        *data = htonl(os->obs_domain_id);

        ofpbuf_put(&psample.cookie,
                   htonl(os->obs_domain_id), sizeof os->obs_domain_id);

> +
> +        data = ofpbuf_put_uninit(&psample.cookie,
> +                                 sizeof(os->obs_point_id));
> +        *data = htonl(os->obs_point_id);

        ofpbuf_put(&psample.cookie,
                   htonl(os->obs_point_id), sizeof os->obs_point_id);

But also, why this is an ofpbuf in the first place?

We could define it as:

ovs_32aligned_be64 psample_cookie;

psample_cookie.hi = htonl(os->obs_domain_id);
psample_cookie.lo = htonl(os->obs_point_id);

Or something similar.  What do you think?

> +
> +        compose_args.psample = &psample;
> +    }
> +
> +    if (!compose_args.userspace && !compose_args.psample) {
> +        return;
> +    }
> +
> +    compose_sample_action(ctx, &compose_args);
> +
> +    if (compose_args.psample) {
> +        ofpbuf_uninit(&compose_args.psample->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..08f9397d8 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -176,8 +176,9 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
>                         const struct mac_learning *, struct stp *,
>                         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_ipfix *, const struct dpif_lsample *,
> +                       const struct netflow *, 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 56830c630..006f67b01 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->lsample, ofproto->netflow,
>                                ofproto->up.forward_bpdu,
>                                connmgr_has_in_band(ofproto->up.connmgr),
>                                &ofproto->backer->rt_support);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0b23fd6c5..2a04977a7 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12136,3 +12136,149 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
>  
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - not supported])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=10 \
> +                    -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=sample(probability=32767,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [in_port(1)],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: 2
> +])
> +
> +OVS_VSWITCHD_STOP(["/ignoring local sampling configuration: not supported by this datapath/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - sanity check])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=42],
> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),3
> +in_port=2, actions=sample(probability=32767,collector_set_id=20,obs_domain_id=100,obs_point_id=200),3
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +dnl collector_set_id does not match.
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: 3
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),3
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - with IPFIX])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> +         [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +m4_define([EXPECTED_ACT], [m4_join([],
> +    [sample(sample=50.0%,actions(],
> +    [psample(group=42,cookie=0x64000000c8),],
> +    [userspace(pid=0,],
> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> +    [))),],
> +    [2],
> +)])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: EXPECTED_ACT
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - with metered IPFIX])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> +         [0], [ignore])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats bands=type=drop rate=2'])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +    [icmp(type=8,code=0)])])
> +
> +m4_define([EXPECTED_ACT], [m4_join([],
> +    [sample(sample=50.0%,actions(],
> +    [psample(group=42,cookie=0x64000000c8),],
> +    [meter(0),],
> +    [userspace(pid=0,],
> +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> +    [))),],
> +    [2],
> +)])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: EXPECTED_ACT
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP

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 <amorenoz@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?

>
> > +        ovs_be32 *data;
> > +
> > +        ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
> > +
> > +        data = ofpbuf_put_uninit(&psample.cookie,
> > +                                 sizeof(os->obs_domain_id));
> > +        *data = htonl(os->obs_domain_id);
>
>         ofpbuf_put(&psample.cookie,
>                    htonl(os->obs_domain_id), sizeof os->obs_domain_id);
>
> > +
> > +        data = ofpbuf_put_uninit(&psample.cookie,
> > +                                 sizeof(os->obs_point_id));
> > +        *data = htonl(os->obs_point_id);
>
>         ofpbuf_put(&psample.cookie,
>                    htonl(os->obs_point_id), sizeof os->obs_point_id);
>
> But also, why this is an ofpbuf in the first place?
>
> We could define it as:
>
> ovs_32aligned_be64 psample_cookie;
>
> psample_cookie.hi = htonl(os->obs_domain_id);
> psample_cookie.lo = htonl(os->obs_point_id);
>
> Or something similar.  What do you think?
>

I used a ofpbuf as a convenient way of passing an array that might grow
in the future. I didn't even know about ovs_32aligned_be64 but it surely
just fits our purpose here. Thanks for the pointer, I'll use it.

> > +
> > +        compose_args.psample = &psample;
> > +    }
> > +
> > +    if (!compose_args.userspace && !compose_args.psample) {
> > +        return;
> > +    }
> > +
> > +    compose_sample_action(ctx, &compose_args);
> > +
> > +    if (compose_args.psample) {
> > +        ofpbuf_uninit(&compose_args.psample->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..08f9397d8 100644
> > --- a/ofproto/ofproto-dpif-xlate.h
> > +++ b/ofproto/ofproto-dpif-xlate.h
> > @@ -176,8 +176,9 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
> >                         const struct mac_learning *, struct stp *,
> >                         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_ipfix *, const struct dpif_lsample *,
> > +                       const struct netflow *, 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 56830c630..006f67b01 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->lsample, ofproto->netflow,
> >                                ofproto->up.forward_bpdu,
> >                                connmgr_has_in_band(ofproto->up.connmgr),
> >                                &ofproto->backer->rt_support);
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 0b23fd6c5..2a04977a7 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -12136,3 +12136,149 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
> >
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - not supported])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=10 \
> > +                    -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1 actions=sample(probability=32767,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [in_port(1)],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: 2
> > +])
> > +
> > +OVS_VSWITCHD_STOP(["/ignoring local sampling configuration: not supported by this datapath/d"])
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - sanity check])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2 3
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- create Flow_Sample_Collector_Set id=1 bridge=@br0 local_group_id=42],
> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),3
> > +in_port=2, actions=sample(probability=32767,collector_set_id=20,obs_domain_id=100,obs_point_id=200),3
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +dnl collector_set_id does not match.
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: 3
> > +])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: sample(sample=50.0%,actions(psample(group=42,cookie=0x64000000c8))),3
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - with IPFIX])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> > +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> > +         [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +m4_define([EXPECTED_ACT], [m4_join([],
> > +    [sample(sample=50.0%,actions(],
> > +    [psample(group=42,cookie=0x64000000c8),],
> > +    [userspace(pid=0,],
> > +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> > +    [))),],
> > +    [2],
> > +)])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: EXPECTED_ACT
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - Local sampling - with metered IPFIX])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2
> > +
> > +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0], [ignore])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +                    -- --id=@i create ipfix targets=\"127.0.0.1:4739\" \
> > +                    -- create Flow_Sample_Collector_Set ipfix=@i id=1 bridge=@br0 local_group_id=42],
> > +         [0], [ignore])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats bands=type=drop rate=2'])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=1, actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200),2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> > +
> > +m4_define([TRACE_PKT], [m4_join([,],
> > +    [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> > +    [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> > +    [icmp(type=8,code=0)])])
> > +
> > +m4_define([EXPECTED_ACT], [m4_join([],
> > +    [sample(sample=50.0%,actions(],
> > +    [psample(group=42,cookie=0x64000000c8),],
> > +    [meter(0),],
> > +    [userspace(pid=0,],
> > +    [flow_sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200,output_port=4294967295)],
> > +    [))),],
> > +    [2],
> > +)])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0], [dnl
> > +Datapath actions: EXPECTED_ACT
> > +])
> > +
> > +OVS_VSWITCHD_STOP()
> > +AT_CLEANUP
>

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 <amorenoz@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.

On Wed, Jul 10, 2024 at 06:52:30PM GMT, Ilya Maximets wrote:
> 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 <amorenoz@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.
>

That's right. When we add a different implementation we'd need to change
this function. We could check what is supported here or maybe we can
"cache" what implementation shall be used in the dpif_lsample object.
I'm not sure at this point in what will be cleaner.

For the time being, do you think we should refactor this to prepare for
the alternative implementation?

On 7/10/24 19:17, Adrián Moreno wrote:
> On Wed, Jul 10, 2024 at 06:52:30PM GMT, Ilya Maximets wrote:
>> 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 <amorenoz@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.
>>
> 
> That's right. When we add a different implementation we'd need to change
> this function. We could check what is supported here or maybe we can
> "cache" what implementation shall be used in the dpif_lsample object.
> I'm not sure at this point in what will be cleaner.
> 
> For the time being, do you think we should refactor this to prepare for
> the alternative implementation?

I'd say no need for refactor for now.  There is no indication so far
that there will be a viable alternative action in the near future.

Best regards, Ilya Maximets.