On 1 Jul 2024, at 14:22, Adrián Moreno wrote:

> On Mon, Jul 01, 2024 at 01:30:12PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 28 Jun 2024, at 18:40, Adrián Moreno wrote:
>>
>>> On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote:
>>>> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>>>>
>>>>> Use the newly added emit_sample to implement OpenFlow sample() actions
>>>>> with local sampling configuration.
>>>>
>>>> See some comments below.
>>>>
>>>> Cheers,
>>>>
>>>> Eelco
>>>>
>>>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>>>> ---
>>>>>  ofproto/ofproto-dpif-lsample.c |  17 ++++
>>>>>  ofproto/ofproto-dpif-lsample.h |   6 ++
>>>>>  ofproto/ofproto-dpif-xlate.c   | 163 ++++++++++++++++++++++++---------
>>>>>  ofproto/ofproto-dpif-xlate.h   |   3 +-
>>>>>  ofproto/ofproto-dpif.c         |   2 +-
>>>>>  5 files changed, 144 insertions(+), 47 deletions(-)
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif-lsample.c 
>>>>> b/ofproto/ofproto-dpif-lsample.c
>>>>> index 7bdafac19..2c0b5da89 100644
>>>>> --- a/ofproto/ofproto-dpif-lsample.c
>>>>> +++ b/ofproto/ofproto-dpif-lsample.c
>>>>> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample 
>>>>> *lsample,
>>>>>      return changed;
>>>>>  }
>>>>>
>>>>> +/* Returns the group_id of the exporter with the given collector_set_id, 
>>>>> if it
>>>>> + * exists. */
>>>>
>>>> nit: The below will fit on one line
>>>>
>>>> /* Returns the exporter group_id for a given collector_set_id, if it 
>>>> exists. */
>>>>
>>>
>>> Ack.
>>>
>>>>> +bool
>>>>> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t 
>>>>> collector_set_id,
>>>>> +                          uint32_t *group_id)
>>>>> +{
>>>>> +    struct lsample_exporter_node *node;
>>>>> +    bool found = false;
>>>>> +
>>>>> +    node = dpif_lsample_find_exporter_node(ps, collector_set_id);
>>>>> +    if (node) {
>>>>> +        found = true;
>>>>> +        *group_id = node->exporter.options.group_id;
>>>>> +    }
>>>>> +    return found;
>>>>> +}
>>>>> +
>>>>>  struct dpif_lsample *
>>>>>  dpif_lsample_create(void)
>>>>>  {
>>>>> diff --git a/ofproto/ofproto-dpif-lsample.h 
>>>>> b/ofproto/ofproto-dpif-lsample.h
>>>>> index c23ea8372..f13425575 100644
>>>>> --- a/ofproto/ofproto-dpif-lsample.h
>>>>> +++ b/ofproto/ofproto-dpif-lsample.h
>>>>> @@ -19,6 +19,7 @@
>>>>>
>>>>>  #include <stdbool.h>
>>>>>  #include <stdlib.h>
>>>>> +#include <stdint.h>
>>>>
>>>> Maybe add in alphabetical order.
>>>>
>>>
>>> Ack.
>>>
>>>>>  struct dpif_lsample;
>>>>>  struct ofproto_lsample_options;
>>>>> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>>>>>                                const struct ofproto_lsample_options *,
>>>>>                                size_t n_opts);
>>>>>
>>>>> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
>>>>> +                               uint32_t collector_set_id,
>>>>> +                               uint32_t *group_id);
>>>>> +
>>>>>  #endif /* OFPROTO_DPIF_LSAMPLE_H */
>>>>> +
>>>>
>>>> Accedantely added a newline?
>>>>
>>>
>>> Ack.
>>>
>>>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>>>> index 7c4950895..5bd215d31 100644
>>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>>> @@ -47,6 +47,7 @@
>>>>>  #include "ofproto/ofproto-dpif-ipfix.h"
>>>>>  #include "ofproto/ofproto-dpif-mirror.h"
>>>>>  #include "ofproto/ofproto-dpif-monitor.h"
>>>>> +#include "ofproto/ofproto-dpif-lsample.h"
>>>>>  #include "ofproto/ofproto-dpif-sflow.h"
>>>>>  #include "ofproto/ofproto-dpif-trace.h"
>>>>>  #include "ofproto/ofproto-dpif-xlate-cache.h"
>>>>> @@ -117,6 +118,7 @@ struct xbridge {
>>>>>      struct dpif_sflow *sflow;     /* SFlow handle, or null. */
>>>>>      struct dpif_ipfix *ipfix;     /* Ipfix handle, or null. */
>>>>>      struct netflow *netflow;      /* Netflow handle, or null. */
>>>>> +    struct dpif_lsample *lsample; /* Local sample handle, or null. */
>>>>>      struct stp *stp;              /* STP or null if disabled. */
>>>>>      struct rstp *rstp;            /* RSTP or null if disabled. */
>>>>>
>>>>> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, 
>>>>> struct dpif *,
>>>>>                                const struct dpif_sflow *,
>>>>>                                const struct dpif_ipfix *,
>>>>>                                const struct netflow *,
>>>>> +                              const struct dpif_lsample *,
>>>>>                                bool forward_bpdu, bool has_in_band,
>>>>>                                const struct dpif_backer_support *,
>>>>>                                const struct xbridge_addr *);
>>>>> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>>>>                    const struct dpif_sflow *sflow,
>>>>>                    const struct dpif_ipfix *ipfix,
>>>>>                    const struct netflow *netflow,
>>>>> +                  const struct dpif_lsample *lsample,
>>>>
>>>> nit: I would move above netflow, as you also do the actual init/unref 
>>>> before
>>>>      netflow.
>>>
>>> Ack.
>>>
>>>>>                    bool forward_bpdu, bool has_in_band,
>>>>>                    const struct dpif_backer_support *support,
>>>>>                    const struct xbridge_addr *addr)
>>>>> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>>>>          xbridge->ipfix = dpif_ipfix_ref(ipfix);
>>>>>      }
>>>>>
>>>>> +    if (xbridge->lsample != lsample) {
>>>>> +        dpif_lsample_unref(xbridge->lsample);
>>>>> +        xbridge->lsample = dpif_lsample_ref(lsample);
>>>>> +    }
>>>>> +
>>>>>      if (xbridge->stp != stp) {
>>>>>          stp_unref(xbridge->stp);
>>>>>          xbridge->stp = stp_ref(stp);
>>>>> @@ -1214,8 +1223,9 @@ xlate_xbridge_copy(struct xbridge *xbridge)
>>>>>                        xbridge->dpif, xbridge->ml, xbridge->stp,
>>>>>                        xbridge->rstp, xbridge->ms, xbridge->mbridge,
>>>>>                        xbridge->sflow, xbridge->ipfix, xbridge->netflow,
>>>>> -                      xbridge->forward_bpdu, xbridge->has_in_band,
>>>>> -                      &xbridge->support, xbridge->addr);
>>>>> +                      xbridge->lsample, xbridge->forward_bpdu,
>>>>> +                      xbridge->has_in_band, &xbridge->support,
>>>>> +                      xbridge->addr);
>>>>>      LIST_FOR_EACH (xbundle, list_node, &xbridge->xbundles) {
>>>>>          xlate_xbundle_copy(new_xbridge, xbundle);
>>>>>      }
>>>>> @@ -1373,6 +1383,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, 
>>>>> const char *name,
>>>>>                    const struct dpif_sflow *sflow,
>>>>>                    const struct dpif_ipfix *ipfix,
>>>>>                    const struct netflow *netflow,
>>>>> +                  const struct dpif_lsample *lsample,
>>>>
>>>> nit: move before netflow.
>>>>
>>>
>>> Ack.
>>>
>>>>>                    bool forward_bpdu, bool has_in_band,
>>>>>                    const struct dpif_backer_support *support)
>>>>>  {
>>>>> @@ -1396,7 +1407,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, 
>>>>> const char *name,
>>>>>      old_addr = xbridge->addr;
>>>>>
>>>>>      xlate_xbridge_set(xbridge, dpif, ml, stp, rstp, ms, mbridge, sflow, 
>>>>> ipfix,
>>>>> -                      netflow, forward_bpdu, has_in_band, support,
>>>>> +                      netflow, lsample, forward_bpdu, has_in_band, 
>>>>> support,
>>>>>                        xbridge_addr);
>>>>>
>>>>>      if (xbridge_addr != old_addr) {
>>>>> @@ -1428,6 +1439,7 @@ xlate_xbridge_remove(struct xlate_cfg *xcfg, struct 
>>>>> xbridge *xbridge)
>>>>>      mbridge_unref(xbridge->mbridge);
>>>>>      dpif_sflow_unref(xbridge->sflow);
>>>>>      dpif_ipfix_unref(xbridge->ipfix);
>>>>> +    dpif_lsample_unref(xbridge->lsample);
>>>>>      netflow_unref(xbridge->netflow);
>>>>>      stp_unref(xbridge->stp);
>>>>>      rstp_unref(xbridge->rstp);
>>>>> @@ -3357,6 +3369,11 @@ xlate_normal(struct xlate_ctx *ctx)
>>>>>      }
>>>>>  }
>>>>>
>>>>> +struct emit_sample_args {
>>>>> +    uint32_t group_id;
>>>>> +    struct ofpbuf cookie;
>>>>> +};
>>>>
>>>> nit: I like the structs to be defined at the top of the file. Not sure what
>>>> the preferred place is.
>>>>
>>>
>>> Given this struct is only used to pass parameters to the function
>>> inmediately below, it seems kind of self-contained. But I agree at the
>>> top might also be OK.
>>>
>>>>>  /* Appends a "sample" action for sFlow or IPFIX to 'ctx->odp_actions'.  
>>>>> The
>>>>>   * 'probability' is the number of packets out of UINT32_MAX to sample.  
>>>>> The
>>>>>   * 'cookie' is passed back in the callback for each sampled packet.
>>>>> @@ -3370,7 +3387,8 @@ xlate_normal(struct xlate_ctx *ctx)
>>>>>  static size_t
>>>>>  compose_sample_action(struct xlate_ctx *ctx,
>>>>>                        const uint32_t probability,
>>>>> -                      const struct user_action_cookie *cookie,
>>>>> +                      const struct user_action_cookie *upcall,
>>>>
>>>> Guess the parameter is still a cookie, so it makes not sense to rename it 
>>>> to
>>>> upcall. If you want to add more meaning you could call it user_cookie. If 
>>>> you
>>>> do you need to update the help text above.
>>>>
>>>
>>> I've refactored this part in the next version to reduce the amount of
>>> arguments and have a more readable argument generation.
>>>
>>>>> +                      const struct emit_sample_args *emit,
>>>>>                        const odp_port_t tunnel_out_port,
>>>>>                        bool include_actions)
>>>>>  {
>>>>> @@ -3379,14 +3397,20 @@ compose_sample_action(struct xlate_ctx *ctx,
>>>>>          return 0;
>>>>>      }
>>>>>
>>>>> +    /* At least one of upcall or emit_sample config must be provided. */
>>>>> +    ovs_assert(upcall || emit);
>>>>> +
>>>>>      /* If the slow path meter is configured by the controller,
>>>>>       * insert a meter action before the user space action.  */
>>>>>      struct ofproto *ofproto = &ctx->xin->ofproto->up;
>>>>>      uint32_t meter_id = ofproto->slowpath_meter_id;
>>>>> +    size_t cookie_offset = 0;
>>>>>
>>>>>      /* When meter action is not required, avoid generate sample action
>>>>>       * for 100% sampling rate.  */
>>>>> -    bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX;
>>>>> +    bool is_sample = (probability < UINT32_MAX ||
>>>>> +                      (upcall && meter_id != UINT32_MAX));
>>>>
>>>> Maybe update the help message a bit to be clear the meter is only for the
>>>> slow patch (userspace) action, and we also omit if sample for 100%
>>>> probability.
>>>>
>>>
>>> Ack.
>>>
>>>>>      size_t sample_offset = 0, actions_offset = 0;
>>>>>      if (is_sample) {
>>>>>          sample_offset = nl_msg_start_nested(ctx->odp_actions,
>>>>> @@ -3397,19 +3421,26 @@ compose_sample_action(struct xlate_ctx *ctx,
>>>>>                                               OVS_SAMPLE_ATTR_ACTIONS);
>>>>>      }
>>>>>
>>>>> -    if (meter_id != UINT32_MAX) {
>>>>> -        nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, 
>>>>> meter_id);
>>>>> +    if (emit) {
>>>>> +        odp_put_emit_sample_action(ctx->odp_actions, emit->group_id,
>>>>> +                                   emit->cookie.data, emit->cookie.size);
>>>>>      }
>>>>>
>>>>> -    odp_port_t odp_port = ofp_port_to_odp_port(
>>>>> -        ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
>>>>> -    uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
>>>>> -    size_t cookie_offset;
>>>>> -    int res = odp_put_userspace_action(pid, cookie, sizeof *cookie,
>>>>> -                                       tunnel_out_port, include_actions,
>>>>> -                                       ctx->odp_actions, &cookie_offset);
>>>>> -    ovs_assert(res == 0);
>>>>> -    if (is_sample) {
>>>>> +    if (upcall) {
>>>>> +        if (meter_id != UINT32_MAX) {
>>>>> +            nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, 
>>>>> meter_id);
>>>>> +        }
>>>>> +
>>>>> +        odp_port_t odp_port = ofp_port_to_odp_port(
>>>>> +            ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
>>>>> +        uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
>>>>> +        int res = odp_put_userspace_action(pid, upcall, sizeof *upcall,
>>>>> +                                           tunnel_out_port, 
>>>>> include_actions,
>>>>> +                                           ctx->odp_actions, 
>>>>> &cookie_offset);
>>>>> +        ovs_assert(res == 0);
>>>>> +    }
>>>>> +
>>>>> +    if (actions_offset) {
>>>>
>>>> Why is if (sample) no longer used, it makes more sense.
>>>>
>>>
>>> Sure, we can change this for  "if (is_sample) {"
>>>
>>>>>          nl_msg_end_nested(ctx->odp_actions, actions_offset);
>>>>>          nl_msg_end_nested(ctx->odp_actions, sample_offset);
>>>>>      }
>>>>> @@ -3440,7 +3471,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>>>>>      cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
>>>>>
>>>>>      return compose_sample_action(ctx, dpif_sflow_get_probability(sflow),
>>>>> -                                 &cookie, ODPP_NONE, true);
>>>>> +                                 &cookie, NULL, ODPP_NONE, true);
>>>>>  }
>>>>>
>>>>>  /* If flow IPFIX is enabled, make sure IPFIX flow sample action
>>>>> @@ -3490,7 +3521,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, 
>>>>> odp_port_t output_odp_port)
>>>>>
>>>>>      compose_sample_action(ctx,
>>>>>                            
>>>>> dpif_ipfix_get_bridge_exporter_probability(ipfix),
>>>>> -                          &cookie, tunnel_out_port, false);
>>>>> +                          &cookie, NULL, tunnel_out_port, false);
>>>>>  }
>>>>>
>>>>>  /* Fix "sample" action according to data collected while composing ODP 
>>>>> actions,
>>>>> @@ -5847,23 +5878,15 @@ xlate_fin_timeout(struct xlate_ctx *ctx,
>>>>>  }
>>>>>
>>>>>  static void
>>>>> -xlate_sample_action(struct xlate_ctx *ctx,
>>>>> -                    const struct ofpact_sample *os)
>>>>> +xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>>>>> +                        const struct ofpact_sample *os,
>>>>> +                        const struct dpif_ipfix *ipfix,
>>>>> +                        struct user_action_cookie *cookie,
>>>>> +                        odp_port_t *tunnel_out_port)
>>>>>  {
>>>>>      odp_port_t output_odp_port = ODPP_NONE;
>>>>> -    odp_port_t tunnel_out_port = ODPP_NONE;
>>>>> -    struct dpif_ipfix *ipfix = ctx->xbridge->ipfix;
>>>>>      bool emit_set_tunnel = false;
>>>>>
>>>>> -    if (!ipfix) {
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>> -    /* Scale the probability from 16-bit to 32-bit while representing
>>>>> -     * the same percentage. */
>>>>> -    uint32_t probability =
>>>>> -        ((uint32_t) os->probability << 16) | os->probability;
>>>>> -
>>>>>      /* If ofp_port in flow sample action is equel to ofp_port,
>>>>>       * this sample action is a input port action. */
>>>>>      if (os->sampling_port != OFPP_NONE &&
>>>>> @@ -5879,7 +5902,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
>>>>>          if (dpif_ipfix_get_flow_exporter_tunnel_sampling(ipfix,
>>>>>                                                           
>>>>> os->collector_set_id)
>>>>>              && dpif_ipfix_is_tunnel_port(ipfix, output_odp_port)) {
>>>>> -            tunnel_out_port = output_odp_port;
>>>>> +            *tunnel_out_port = output_odp_port;
>>>>>              emit_set_tunnel = true;
>>>>>          }
>>>>>      }
>>>>> @@ -5913,20 +5936,70 @@ xlate_sample_action(struct xlate_ctx *ctx,
>>>>>          }
>>>>>      }
>>>>>
>>>>> -    struct user_action_cookie cookie;
>>>>> +    cookie->type = USER_ACTION_COOKIE_FLOW_SAMPLE;
>>>>> +    cookie->ofp_in_port = ctx->xin->flow.in_port.ofp_port;
>>>>> +    cookie->ofproto_uuid = ctx->xbridge->ofproto->uuid;
>>>>> +    cookie->flow_sample.probability = os->probability;
>>>>> +    cookie->flow_sample.collector_set_id = os->collector_set_id;
>>>>> +    cookie->flow_sample.obs_domain_id = os->obs_domain_id;
>>>>> +    cookie->flow_sample.obs_point_id = os->obs_point_id;
>>>>> +    cookie->flow_sample.output_odp_port = output_odp_port;
>>>>> +    cookie->flow_sample.direction = os->direction;
>>>>> +}
>>>>>
>>>>> -    memset(&cookie, 0, sizeof cookie);
>>>>
>>>> Why does is the memset() removed from the new code? Are we sure we are
>>>> setting all the fields?
>>>>
>>>>> -    cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE;
>>>>> -    cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port;
>>>>> -    cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
>>>>> -    cookie.flow_sample.probability = os->probability;
>>>>> -    cookie.flow_sample.collector_set_id = os->collector_set_id;
>>>>> -    cookie.flow_sample.obs_domain_id = os->obs_domain_id;
>>>>> -    cookie.flow_sample.obs_point_id = os->obs_point_id;
>>>>> -    cookie.flow_sample.output_odp_port = output_odp_port;
>>>>> -    cookie.flow_sample.direction = os->direction;
>>>>> -
>>>>> -    compose_sample_action(ctx, probability, &cookie, tunnel_out_port, 
>>>>> false);
>>>>> +static void
>>>>> +xlate_sample_action(struct xlate_ctx *ctx,
>>>>> +                    const struct ofpact_sample *os)
>>>>> +{
>>>>> +    uint8_t cookie_buf[sizeof(os->obs_domain_id) + 
>>>>> sizeof(os->obs_point_id)];
>>>>> +    struct dpif_lsample *lsample = ctx->xbridge->lsample;
>>>>> +    struct dpif_ipfix *ipfix = ctx->xbridge->ipfix;
>>>>> +    struct emit_sample_args emit_sample_args;
>>>>> +    struct user_action_cookie upcall_cookie;
>>>>
>>>> As the action is called userspace we might just want to call the cookie as
>>>> such for consistency, i.e. user_cookie/userspace_cookie.
>>>>
>>>
>>> This will change in next version and I think it will have better
>>> names... I said "I think" :-).
>>>
>>>>> +    odp_port_t tunnel_out_port = ODPP_NONE;
>>>>> +    bool emit_sample = false;
>>>>> +    uint32_t group_id;
>>>>> +
>>>>> +    if (!ipfix && !lsample) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /* Scale the probability from 16-bit to 32-bit while representing
>>>>> +     * the same percentage. */
>>>>> +    uint32_t probability =
>>>>> +        ((uint32_t) os->probability << 16) | os->probability;
>>>>> +
>>>>> +    if (ipfix) {
>>>>> +        xlate_fill_ipfix_sample(ctx, os, ipfix, &upcall_cookie,
>>>>> +                                &tunnel_out_port);
>>>>> +    }
>>>>
>>>> New line here.
>>>>
>>>
>>> Ack.
>>>
>>>>> +    if (lsample) {
>>>>> +        if (dpif_lsample_get_group_id(lsample,
>>>>> +                                      os->collector_set_id,
>>>>> +                                      &group_id)) {
>>>>
>>>> Maybe just do &emit_sample_args.group_id, it saves the variable and 
>>>> setting it
>>>> below. Also all the ofbuf_xx() function are in order ;)
>>>>
>>>>> +            emit_sample = true;
>>>>> +            ofpbuf_use_stub(&emit_sample_args.cookie, cookie_buf,
>>>>> +                            sizeof cookie_buf);
>>>>> +
>>>>> +            emit_sample_args.group_id = group_id;
>>>>> +            ofpbuf_put(&emit_sample_args.cookie, &os->obs_domain_id,
>>>>> +                       sizeof(os->obs_domain_id));
>>>>> +            ofpbuf_put(&emit_sample_args.cookie, &os->obs_point_id,
>>>>> +                       sizeof(os->obs_point_id));
>>>>
>>>> Guess the endianness discussion needs to be fixed here.
>>>>
>>>
>>> Yes.
>>>
>>>>> +        }
>>>>
>>>> Is the dpif_lsample_get_group_id() a common case? Do we need a log message 
>>>> or
>>>> coverage counter?
>>>>
>>>
>>> You mean for samples for which we cannot find a group_id?
>>
>> Yes, the corner case.
>
> dpif_lsample_get_group_id() will check if there is as Local Sampling
> entry associated with the current SAMPLE action.
>
> OTOH, Local Sampling and IPFIX can be configured simmultaneously with or
> without sharing the collector id. For instance:
>
> ovs-vsctl -- --id=@br0 get Bridge br0 \
>     -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \
>     -- create Flow_Sample_Collector_Set id=1 bridge=@br0 ipfix=@ipfix \
>     -- create Flow_Sample_Collector_Set id=2 bridge=@br0 local_group_id=12],
>
> With this config, all actions such as the following ...
>     actions:...,sample(sample=123,collector_set_id=1)
>
> ... will trigger this: lsample will be non-NULL (it is configured with
> local_group_id=12 for collector_set_id=2), but we will fail to find a
> valid group_id for collector_set_id 1.
>
> Not sure how rare this configuration might be (for someone starting to
> use local sampling on an already IPFIX-enabled system it doesn't look
> bad to me). But when configured this way, the lookup failure can happen
> quite often (on every sample action translation) and it doesn't seem to
> be relevant enough for a coverage counter.

Thanks that clears it up, so we can leave it as is.

>>>>> +    }
>>>>> +
>>>>> +    if (!ipfix && !emit_sample) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    compose_sample_action(ctx, probability, ipfix ? &upcall_cookie : 
>>>>> NULL,
>>>>> +                          emit_sample ? &emit_sample_args : NULL,
>>>>> +                          tunnel_out_port, false);
>>>>> +
>>>>> +    if (emit_sample) {
>>>>> +        ofpbuf_uninit(&emit_sample_args.cookie);
>>>>> +    }
>>>>>  }
>>>>>
>>>>>  /* Determine if an datapath action translated from the openflow action
>>>>> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
>>>>> index 05b46fb26..164ff3713 100644
>>>>> --- a/ofproto/ofproto-dpif-xlate.h
>>>>> +++ b/ofproto/ofproto-dpif-xlate.h
>>>>> @@ -177,7 +177,8 @@ void xlate_ofproto_set(struct ofproto_dpif *, const 
>>>>> char *name, struct dpif *,
>>>>>                         struct rstp *, const struct mcast_snooping *,
>>>>>                         const struct mbridge *, const struct dpif_sflow *,
>>>>>                         const struct dpif_ipfix *, const struct netflow *,
>>>>> -                       bool forward_bpdu, bool has_in_band,
>>>>> +                       const struct dpif_lsample *, bool forward_bpdu,
>>>>
>>>> Maybe move it after dpif_sflow to be consistent.
>>>>
>>>
>>> Ack.
>>>
>>>>> +                       bool has_in_band,
>>>>>                         const struct dpif_backer_support *support);
>>>>>  void xlate_remove_ofproto(struct ofproto_dpif *);
>>>>>  struct ofproto_dpif *xlate_ofproto_lookup(const struct uuid *uuid);
>>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>>> index 067f60df3..48167751f 100644
>>>>> --- a/ofproto/ofproto-dpif.c
>>>>> +++ b/ofproto/ofproto-dpif.c
>>>>> @@ -485,7 +485,7 @@ type_run(const char *type)
>>>>>                                ofproto->backer->dpif, ofproto->ml,
>>>>>                                ofproto->stp, ofproto->rstp, ofproto->ms,
>>>>>                                ofproto->mbridge, ofproto->sflow, 
>>>>> ofproto->ipfix,
>>>>> -                              ofproto->netflow,
>>>>> +                              ofproto->netflow, ofproto->lsample,
>>>>>                                ofproto->up.forward_bpdu,
>>>>>                                connmgr_has_in_band(ofproto->up.connmgr),
>>>>>                                &ofproto->backer->rt_support);
>>>>> --
>>>>> 2.45.1
>>>>
>>

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to