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 <amore...@redhat.com>
>>> ---
>>> ofproto/ofproto-dpif-lsample.c | 16 +++
>>> ofproto/ofproto-dpif-lsample.h | 5 +
>>> ofproto/ofproto-dpif-xlate.c | 251 +++++++++++++++++++++++----------
>>> ofproto/ofproto-dpif-xlate.h | 5 +-
>>> ofproto/ofproto-dpif.c | 2 +-
>>> tests/ofproto-dpif.at | 146 +++++++++++++++++++
>>> 6 files changed, 345 insertions(+), 80 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
>>> index d675a116f..534ad96f0 100644
>>> --- a/ofproto/ofproto-dpif-lsample.c
>>> +++ b/ofproto/ofproto-dpif-lsample.c
>>> @@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
>>> return changed;
>>> }
>>>
>>> +/* Returns the group_id for a given collector_set_id, if it exists. */
>>> +bool
>>> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
>>> collector_set_id,
>>> + uint32_t *group_id)
>>> +{
>>> + struct lsample_exporter_node *node;
>>> + bool found = false;
>>> +
>>> + node = dpif_lsample_find_exporter_node(ps, collector_set_id);
>>> + if (node) {
>>> + found = true;
>>> + *group_id = node->exporter.options.group_id;
>>> + }
>>> + return found;
>>> +}
>>> +
>>> struct dpif_lsample *
>>> dpif_lsample_create(void)
>>> {
>>> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
>>> index bee36c9c5..9c1026551 100644
>>> --- a/ofproto/ofproto-dpif-lsample.h
>>> +++ b/ofproto/ofproto-dpif-lsample.h
>>> @@ -18,6 +18,7 @@
>>> #define OFPROTO_DPIF_LSAMPLE_H 1
>>>
>>> #include <stdbool.h>
>>> +#include <stdint.h>
>>> #include <stdlib.h>
>>>
>>> struct dpif_lsample;
>>> @@ -33,4 +34,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>>> const struct ofproto_lsample_options *,
>>> size_t n_opts);
>>>
>>> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
>>> + uint32_t collector_set_id,
>>> + uint32_t *group_id);
>>> +
>>> #endif /* OFPROTO_DPIF_LSAMPLE_H */
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 7c4950895..5e8113d5e 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -47,6 +47,7 @@
>>> #include "ofproto/ofproto-dpif-ipfix.h"
>>> #include "ofproto/ofproto-dpif-mirror.h"
>>> #include "ofproto/ofproto-dpif-monitor.h"
>>> +#include "ofproto/ofproto-dpif-lsample.h"
>>
>> 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
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
