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 <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?
>
> 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.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
