[ovs-dev,v6,04/11] odp-execute: Add auto validation function for actions.

This commit introduced the auto-validation function which
allows users to compare the batch of packets obtained from
different action implementations against the linear
action implementation.

The autovalidator function can be triggered at runtime using the
following command:

$ ovs-appctl dpif-netdev/action-impl-set autovalidator

Signed-off-by: Emma Finn <emma.finn@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 NEWS                      |  2 +
 lib/dp-packet.c           | 23 +++++++++
 lib/dp-packet.h           |  4 ++
 lib/odp-execute-private.c | 99 +++++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.h |  3 ++
 5 files changed, 131 insertions(+)

On 10 May 2022, at 16:21, Emma Finn wrote:

> This commit introduced the auto-validation function which
> allows users to compare the batch of packets obtained from
> different action implementations against the linear
> action implementation.
>
> The autovalidator function can be triggered at runtime using the
> following command:
>
> $ ovs-appctl dpif-netdev/action-impl-set autovalidator
>
> Signed-off-by: Emma Finn <emma.finn@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  NEWS                      |  2 +
>  lib/dp-packet.c           | 23 +++++++++
>  lib/dp-packet.h           |  4 ++
>  lib/odp-execute-private.c | 99 +++++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.h |  3 ++
>  5 files changed, 131 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index eece0d0b2..8539a03b6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -58,6 +58,8 @@ v2.17.0 - 17 Feb 2022
>       * Add support for DPDK 21.11.
>       * Forbid use of DPDK multiprocess feature.
>       * Add support for running threads on cores >= RTE_MAX_LCORE.
> +     * Add actions auto-validator function to compare different actions
> +       implementations against default implementation.
>     - Python:
>       * For SSL support, the use of the pyOpenSSL library has been replaced
>         with the native 'ssl' module.
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 35c72542a..b71c68ed0 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -506,3 +506,26 @@ dp_packet_resize_l2(struct dp_packet *b, int increment)
>      dp_packet_adjust_layer_offset(&b->l2_5_ofs, increment);
>      return dp_packet_data(b);
>  }
> +
> +bool
> +dp_packet_compare_and_log(struct dp_packet *good, struct dp_packet *test,
> +                          struct ds *err_str)

This function does not live up to its name, i.e. it's not comparing any packet data, and it's not logging either.

Maybe just call it dp_packet_compare(), and make the *err_str optional. For good, test, I would rename it to b1 and b2.

Rather than (only) comparing offsets, it should also do a full packet compare, and dump hex values. But this might be too much variations for logging in this function, so maybe just do a memcpy and return true/false, then in action_autoval_generic(), you can figure out what to log.

> +{
> +    if ((good->l2_pad_size != test->l2_pad_size) ||
> +        (good->l2_5_ofs != test->l2_5_ofs) ||
> +        (good->l3_ofs != test->l3_ofs) ||
> +        (good->l4_ofs != test->l4_ofs)) {

See above wrap logging in if (err_str) {

> +            ds_put_format(err_str, "Autovalidation packet offsets failed"
> +                          "\n");

This text should not be part of this function, as it's not general.

> +            ds_put_format(err_str, "Good offsets: l2_pad_size %u,"

Buffer 1 offsets:

> +                          " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> +                          good->l2_pad_size, good->l2_5_ofs,
> +                          good->l3_ofs, good->l4_ofs);
> +            ds_put_format(err_str, "Test offsets: l2_pad_size %u,"

Buffer 2 offsets:

> +                          " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> +                          test->l2_pad_size, test->l2_5_ofs,
> +                          test->l3_ofs, test->l4_ofs);
> +        return false;
> +    }
> +    return true;
> +}
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index bddaa2b5d..bf7ee61a5 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -236,6 +236,10 @@ void *dp_packet_steal_data(struct dp_packet *);
>  static inline bool dp_packet_equal(const struct dp_packet *,
>                                     const struct dp_packet *);
>
> +bool dp_packet_compare_and_log(struct dp_packet *good,
> +                               struct dp_packet *test,
> +                               struct ds *err_str);
> +
>  
>  /* Frees memory that 'b' points to, as well as 'b' itself. */
>  static inline void
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 996de0bf6..e85aed636 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -30,8 +30,16 @@
>  int32_t action_autoval_init(struct odp_execute_action_impl *self);
>  VLOG_DEFINE_THIS_MODULE(odp_execute_private);
>  static uint32_t active_action_impl_index;
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);

Don't think we need this, see below.

>
>  static struct odp_execute_action_impl action_impls[] = {
> +    [ACTION_IMPL_AUTOVALIDATOR] = {
> +        .available = 1,

Use true/false on bools, please apply everywhere.

> +        .name = "autovalidator",
> +        .probe = NULL,
> +        .init_func = action_autoval_init,
> +    },
> +
>      [ACTION_IMPL_SCALAR] = {
>          .available = 1,
>          .name = "scalar",
> @@ -109,3 +117,94 @@ odp_execute_action_init(void)
>          }
>      }
>  }
> +
> +/* Init sequence required to be scalar first to pick up the default scalar
> +* implementations, allowing over-riding of the optimized functions later.
> +*/
> +BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
> +BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
> +
> +/* Loop over packets, and validate each one for the given action. */
> +static void
> +action_autoval_generic(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
> +                       const struct nlattr *a, bool should_steal)
> +{
> +    uint32_t failed = 0;

Why not bool, bool failed = false; ?

> +
> +    int type = nl_attr_type(a);
> +    enum ovs_action_attr attr_type = (enum ovs_action_attr) type;

This can be done in one line:

  enum ovs_action_attr attr_type = (enum ovs_action_attr) nl_attr_type(a);

We should also check the attr_type is in bound of __OVS_ACTION_ATTR_MAX.

> +
> +    struct odp_execute_action_impl *scalar = &action_impls[ACTION_IMPL_SCALAR];

Remove this cr/lf.

> +    struct dp_packet_batch good_batch;

cr should be here.

> +    dp_packet_batch_clone(&good_batch, batch);
> +
> +    scalar->funcs[attr_type](NULL, &good_batch, a, should_steal);

Can we test this on packets that have the should_steal flag? Is the result available?

> +
> +    for (uint32_t impl = ACTION_IMPL_BEGIN; impl < ACTION_IMPL_MAX; impl++) {

int impl?

> +        /* Clone original batch and execute implementation under test. */
> +        struct dp_packet_batch test_batch;

cr between definitions and code.

> +        dp_packet_batch_clone(&test_batch, batch);
> +        action_impls[impl].funcs[attr_type](NULL, &test_batch, a,
> +                                            should_steal);
> +
> +        /* Loop over implementations, checking each one. */
> +        for (uint32_t pidx = 0; pidx < batch->count; pidx++) {

int pidx?

> +            struct dp_packet *good_pkt = good_batch.packets[pidx];
> +            struct dp_packet *test_pkt = test_batch.packets[pidx];
> +
> +            struct ds log_msg = DS_EMPTY_INITIALIZER;
> +
> +            /* Compare packet length and payload contents. */
> +            bool eq = dp_packet_equal(good_pkt, test_pkt);
> +
> +            if (!eq) {
> +                ds_put_format(&log_msg, "Packet: %d\nAction : ", pidx);
> +                format_odp_actions(&log_msg, a, a->nla_len, NULL);
> +                ds_put_format(&log_msg, "\nGood hex:\n");
> +                ds_put_hex_dump(&log_msg, dp_packet_data(good_pkt),
> +                                dp_packet_size(good_pkt), 0, false);
> +                ds_put_format(&log_msg, "Test hex:\n");
> +                ds_put_hex_dump(&log_msg, dp_packet_data(test_pkt),
> +                                dp_packet_size(test_pkt), 0, false);
> +
> +                failed = 1;

You should use true/false here (and below)

> +            }
> +
> +            /* Compare offsets and RSS */

My previous comments on dp_packet_compare_and_log() seem to be taken care of above.
So I would either change dp_packet_compare_and_log() to dp_packet_compare_offsets(), or just do the compare here.

> +            if (!dp_packet_compare_and_log(good_pkt, test_pkt, &log_msg)) {
> +                failed = 1;
> +            }
> +
> +            uint32_t good_hash = dp_packet_get_rss_hash(good_pkt);
> +            uint32_t test_hash = dp_packet_get_rss_hash(test_pkt);
> +
> +            if (good_hash != test_hash) {
> +                ds_put_format(&log_msg, "Autovalidation rss hash failed"
> +                              "\n");

What about:

 +                ds_put_format(&log_msg,
 +                              "Autovalidation rss hash failed!\n");

> +                ds_put_format(&log_msg, "Good RSS hash : %u\n", good_hash);
> +                ds_put_format(&log_msg, "Test RSS hash : %u\n", test_hash);
> +
> +                failed = 1;
> +            }
> +
> +            if (failed) {
> +                VLOG_ERR_RL(&rl, "\nAutovalidation failed details:\n%s",

Autovalidator should be used in test cases, so we should not rate limit it.
Don’t thinks we need the leading “\n”, also we do not know which implementation failed!

> +                            ds_cstr(&log_msg));

Do we need to free/re-init log_msg?

We also need to reset failed, failed = false.

> +            }
> +        }
> +        dp_packet_delete_batch(&test_batch, 1);
> +    }
> +    dp_packet_delete_batch(&good_batch, 1);
> +
> +    /* Apply the action to the original batch for continued processing. */

Now we execute the scaler twice is there another way?

> +    scalar->funcs[attr_type](NULL, batch, a, should_steal);
> +}
> +
> +int32_t
> +action_autoval_init(struct odp_execute_action_impl *self)
> +{
> +    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_autoval_generic;
> +

We should not initialize only the known function(s), we should assign the action_autoval_generic function to all possible actions (__OVS_ACTION_ATTR_MAX), and only skip the ones that would forward the packets ;)

I think in general we need to be more clear that the function pointer implementation is only for packet transform operations, and we should identify which ones those are (add this to the odp_execute_action_impl definition).

You can have the action_autoval_generic() skip the test if the two function pointers are the same?

> +    return 0;
> +}
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index 869478ce9..fed20930d 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -66,6 +66,7 @@ struct odp_execute_action_impl {
>  /* Order of Actions implementations. */
>  enum odp_execute_action_impl_idx {
>      ACTION_IMPL_SCALAR,
> +    ACTION_IMPL_AUTOVALIDATOR,
>      /* See ACTION_IMPL_BEGIN below, for "first to-be-validated" impl.
>       * Do not change the autovalidator position in this list without updating
>       * the define below.
> @@ -76,6 +77,8 @@ enum odp_execute_action_impl_idx {
>
>  /* Index to start verifying implementations from. */
>  BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
> +BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);

Add cr/lf

> +#define ACTION_IMPL_BEGIN (ACTION_IMPL_AUTOVALIDATOR + 1)
>
>  /* Odp execute init handles setting up the state of the actions functions at
>   * initialization time. It cannot return errors, as it must always succeed in
> -- 
> 2.25.1

> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Thursday 2 June 2022 15:15
> To: Finn, Emma <emma.finn@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, Kumar
> <kumar.amber@intel.com>; Stokes, Ian <ian.stokes@intel.com>;
> dev@openvswitch.org
> Subject: Re: [v6 04/11] odp-execute: Add auto validation function for
> actions.
> 
> On 10 May 2022, at 16:21, Emma Finn wrote:
> 
> > This commit introduced the auto-validation function which allows users
> > to compare the batch of packets obtained from different action
> > implementations against the linear action implementation.
> >
> > The autovalidator function can be triggered at runtime using the
> > following command:
> >
> > $ ovs-appctl dpif-netdev/action-impl-set autovalidator
> >
> > Signed-off-by: Emma Finn <emma.finn@intel.com>
> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  NEWS                      |  2 +
> >  lib/dp-packet.c           | 23 +++++++++
> >  lib/dp-packet.h           |  4 ++
> >  lib/odp-execute-private.c | 99
> > +++++++++++++++++++++++++++++++++++++++
> >  lib/odp-execute-private.h |  3 ++
> >  5 files changed, 131 insertions(+)
> >
<snip>

> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> 
> Don't think we need this, see below.
> 
> Autovalidator should be used in test cases, so we should not rate limit it.

I think it is best to avoid spamming CI log files to huge sizes.

> Don’t thinks we need the leading “\n”, also we do not know which
> implementation failed!
> 
> > +                            ds_cstr(&log_msg));
> 
> Do we need to free/re-init log_msg?
> 
> We also need to reset failed, failed = false.
> 
> > +            }
> > +        }
> > +        dp_packet_delete_batch(&test_batch, 1);
> > +    }
> > +    dp_packet_delete_batch(&good_batch, 1);
> > +
> > +    /* Apply the action to the original batch for continued
> > + processing. */
> 
> Now we execute the scaler twice is there another way?
> 

Not a clean way that I can think of. 

> > +    scalar->funcs[attr_type](NULL, batch, a, should_steal); }
> > +
> > +int32_t
> > +action_autoval_init(struct odp_execute_action_impl *self) {
> > +    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_autoval_generic;
> > +
> 
> We should not initialize only the known function(s), we should assign the
> action_autoval_generic function to all possible actions
> (__OVS_ACTION_ATTR_MAX), and only skip the ones that would forward
> the packets ;)
> 
> I think in general we need to be more clear that the function pointer
> implementation is only for packet transform operations, and we should
> identify which ones those are (add this to the odp_execute_action_impl
> definition).
> 
> You can have the action_autoval_generic() skip the test if the two function
> pointers are the same?
> 

What is the core problem to solve here?
I like the simplicity of setting the function pointer for the optimized routines. Essentially, this manually selects autovalidation for the optimized action variants.
A blanket "autovalidate all" approach, and then selectively disabling specific actions feels "backwards" in finding out which actions do transmit/have-side-effects.
I prefer the current approach of enabling func-ptr validation where it is required, and limiting change to only the area where it is known to be required.

> > +    return 0;
> > +}
> > diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> > index 869478ce9..fed20930d 100644
> > --- a/lib/odp-execute-private.h
> > +++ b/lib/odp-execute-private.h
> > @@ -66,6 +66,7 @@ struct odp_execute_action_impl {
> >  /* Order of Actions implementations. */  enum
> > odp_execute_action_impl_idx {
> >      ACTION_IMPL_SCALAR,
> > +    ACTION_IMPL_AUTOVALIDATOR,
> >      /* See ACTION_IMPL_BEGIN below, for "first to-be-validated" impl.
> >       * Do not change the autovalidator position in this list without
> updating
> >       * the define below.
> > @@ -76,6 +77,8 @@ enum odp_execute_action_impl_idx {
> >
> >  /* Index to start verifying implementations from. */
> > BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
> > +BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
> 
> Add cr/lf
> 
> > +#define ACTION_IMPL_BEGIN (ACTION_IMPL_AUTOVALIDATOR + 1)
> >
> >  /* Odp execute init handles setting up the state of the actions functions
> at
> >   * initialization time. It cannot return errors, as it must always
> > succeed in
> > --
> > 2.25.1