[ovs-dev,v7,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                      |  3 ++
 lib/dp-packet.c           | 23 +++++++++
 lib/dp-packet.h           |  4 ++
 lib/odp-execute-private.c | 98 +++++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.h |  6 +++
 5 files changed, 134 insertions(+)

On 14 Jun 2022, at 13:57, 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                      |  3 ++
>  lib/dp-packet.c           | 23 +++++++++
>  lib/dp-packet.h           |  4 ++
>  lib/odp-execute-private.c | 98 +++++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.h |  6 +++
>  5 files changed, 134 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 9fe3f44f4..3a25f3035 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -32,6 +32,9 @@ Post-v2.17.0
>     - DPDK:
>       * OVS validated with DPDK 21.11.1.  It is recommended to use this version
>         until further releases.
> +   - Userspace datapath:
> +     * Add actions auto-validator function to compare different actions
> +       implementations against default implementation.
>
>
>  v2.17.0 - 17 Feb 2022
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 35c72542a..237dcf19e 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_offsets(struct dp_packet *b1, struct dp_packet *b2,
> +                          struct ds *err_str)
> +{
> +    if ((b1->l2_pad_size != b2->l2_pad_size) ||
> +        (b1->l2_5_ofs != b2->l2_5_ofs) ||
> +        (b1->l3_ofs != b2->l3_ofs) ||
> +        (b1->l4_ofs != b2->l4_ofs)) {

Please wrap error reporting in if (err_str) so this function is more general.
Guess you removed it, as the dp_put_format's are indented one to far.

> +            ds_put_format(err_str, "Packet offset comparison failed"
> +                          "\n");

For this one split the line differently so the string is on one line:

            ds_put_format(err_str,
                          "Packet offset comparison failed\n");

We are not as strict as Linux that all strings should be a single line, to make grep easier, but I like to comply as much as possible.

> +            ds_put_format(err_str, "Buffer 1 offsets: l2_pad_size %u,"
> +                          " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> +                          b1->l2_pad_size, b1->l2_5_ofs,
> +                          b1->l3_ofs, b1->l4_ofs);
> +            ds_put_format(err_str, "Buffer 2 offsets: l2_pad_size %u,"
> +                          " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> +                          b2->l2_pad_size, b2->l2_5_ofs,
> +                          b2->l3_ofs, b2->l4_ofs);
> +        return false;
> +    }
> +    return true;
> +}
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index bddaa2b5d..1776c3bfe 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_offsets(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 25dbbfefc..267f32c3e 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -28,8 +28,15 @@
>
>  VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
>  static int active_action_impl_index;
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);

From the v6 discussion:

EC> Don't think we need this, see below.
EC> Autovalidator should be used in test cases, so we should not rate limit it.

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

This is for CI only, and we should get every log message helping up point to the root cause.
So I still think this needs to be removed.

>  static struct odp_execute_action_impl action_impls[] = {
> +    [ACTION_IMPL_AUTOVALIDATOR] = {
> +        .available = false,
> +        .name = "autovalidator",
> +        .init_func = action_autoval_init,
> +    },
> +
>      [ACTION_IMPL_SCALAR] = {
>          .available = false,
>          .name = "scalar",
> @@ -94,3 +101,94 @@ odp_execute_action_init(void)
>                    action_impls[i].name, avail ? "available" : "not available");
>      }
>  }
> +
> +/* 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(struct dp_packet_batch *batch, const struct nlattr *a)
> +{
> +    bool failed = false;
> +    int type = nl_attr_type(a);
> +    enum ovs_action_attr attr_type = (enum ovs_action_attr) type;
> +    struct odp_execute_action_impl *scalar = &action_impls[ACTION_IMPL_SCALAR];
> +    struct dp_packet_batch good_batch;

Can we use reverse Christmas tree and remove type as suggested before?

    struct odp_execute_action_impl *scalar = &action_impls[ACTION_IMPL_SCALAR];
    enum ovs_action_attr attr_type = nl_attr_type(a);
    struct dp_packet_batch good_batch;
    bool failed = false;

> +
> +    dp_packet_batch_clone(&good_batch, batch);
> +
> +    scalar->funcs[attr_type](&good_batch, a);
> +
> +    for (int impl = ACTION_IMPL_BEGIN; impl < ACTION_IMPL_MAX; impl++) {
> +        /* Clone original batch and execute implementation under test. */
> +        struct dp_packet_batch test_batch;
> +
> +        dp_packet_batch_clone(&test_batch, batch);
> +        action_impls[impl].funcs[attr_type](&test_batch, a);
> +
> +        /* Loop over implementations, checking each one. */
> +        for (int pidx = 0; pidx < batch->count; 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 = true;
> +            }
> +
> +            /* Compare offsets and RSS */
> +            if (!dp_packet_compare_offsets(good_pkt, test_pkt, &log_msg)) {
> +                failed = true;
> +            }
> +
> +            uint32_t good_hash = dp_packet_get_rss_hash(good_pkt);
> +            uint32_t test_hash = dp_packet_get_rss_hash(test_pkt);
> +

Only compare RSS if it's valid, or else it will cause failures:

 if (dp_packet_rss_valid(good_pkt)) {

> +            if (good_hash != test_hash) {
> +                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 = true;
> +            }
> +
> +            if (failed) {
> +                VLOG_ERR_RL(&rl, "Autovalidation of %s failed. Details:\n%s",
> +                            action_impls[impl].name, ds_cstr(&log_msg));
> +                ds_destroy(&log_msg);
> +                failed = false;
> +            }
> +        }
> +        dp_packet_delete_batch(&test_batch, 1);

Use true here.

> +    }
> +    dp_packet_delete_batch(&good_batch, 1);

Use true here.

> +
> +    /* Apply the action to the original batch for continued processing. */
> +    scalar->funcs[attr_type](batch, a);

v6 discussion:

EC> Now we execute the scaler twice is there another way?
EF> Not a clean way that I can think of.

How about the following:

@@ -124,26 +123,25 @@ BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
 static void
 action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a)
 {
-    bool failed = false;
-    int type = nl_attr_type(a);
-    enum ovs_action_attr attr_type = (enum ovs_action_attr) type;
     struct odp_execute_action_impl *scalar = &action_impls[ACTION_IMPL_SCALAR];
-    struct dp_packet_batch good_batch;
+    enum ovs_action_attr attr_type = nl_attr_type(a);
+    struct dp_packet_batch original_batch;
+    bool failed = false;

-    dp_packet_batch_clone(&good_batch, batch);
+    dp_packet_batch_clone(&original_batch, batch);

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

     for (int impl = ACTION_IMPL_BEGIN; impl < ACTION_IMPL_MAX; impl++) {
         /* Clone original batch and execute implementation under test. */
         struct dp_packet_batch test_batch;

-        dp_packet_batch_clone(&test_batch, batch);
+        dp_packet_batch_clone(&test_batch, &original_batch);
         action_impls[impl].funcs[attr_type](&test_batch, a);

         /* Loop over implementations, checking each one. */
-        for (int pidx = 0; pidx < batch->count; pidx++) {
-            struct dp_packet *good_pkt = good_batch.packets[pidx];
+        for (int pidx = 0; pidx < original_batch.count; pidx++) {
+            struct dp_packet *good_pkt = batch->packets[pidx];
             struct dp_packet *test_pkt = test_batch.packets[pidx];

             struct ds log_msg = DS_EMPTY_INITIALIZER;
@@ -181,18 +179,15 @@ action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a)
             }

             if (failed) {
-                VLOG_ERR_RL(&rl, "Autovalidation of %s failed. Details:\n%s",
-                            action_impls[impl].name, ds_cstr(&log_msg));
+                VLOG_ERR("Autovalidation of %s failed. Details:\n%s",
+                         action_impls[impl].name, ds_cstr(&log_msg));
                 ds_destroy(&log_msg);
                 failed = false;
             }
         }
-        dp_packet_delete_batch(&test_batch, 1);
+        dp_packet_delete_batch(&test_batch, true);
     }
-    dp_packet_delete_batch(&good_batch, 1);
-
-    /* Apply the action to the original batch for continued processing. */
-    scalar->funcs[attr_type](batch, a);
+    dp_packet_delete_batch(&original_batch, true);
 }


From v6 review:

EC> We should not initialize only the known function(s), we should assign the
EC> action_autoval_generic function to all possible actions
EC> (__OVS_ACTION_ATTR_MAX), and only skip the ones that would forward
EC> the packets ;)
EC>
EC> I think in general we need to be more clear that the function pointer
EC> implementation is only for packet transform operations, and we should
EC> identify which ones those are (add this to the odp_execute_action_impl
EC> definition).
EC>
EC> You can have the action_autoval_generic() skip the test if the two function
EC> pointers are the same?
EC>

EF> What is the core problem to solve here?
EF> I like the simplicity of setting the function pointer for the optimized routines. Essentially, this manually selects autovalidation for the optimized action variants.
EF> A blanket "autovalidate all" approach, and then selectively disabling specific actions feels "backwards" in finding out which actions do transmit/have-side-effects.
EF> 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.

What I'm trying to solve here, is that during a new addition, this is something that is easily forgotten. In line with the previous change to make sure a scalar implementation existst for every ISA specific one, I think we should populate the actions that have a scaler configured. Something like:

diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 0333261d5..54c7bc638 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -195,7 +195,10 @@ action_autoval_init(struct odp_execute_action_impl *self)
 {
     /* Set function pointers for actions that can be applied directly, these
      * are identified by OVS_ACTION_ATTR_*. */
-    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_autoval_generic;
-
+    for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) {
+        if (action_impls[ACTION_IMPL_SCALAR].funcs[i]) {
+            self->funcs[i] = action_autoval_generic;
+        }
+    }
     return 0;
 }

> +    return 0;
> +}
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index c5ab00b07..d3dc669d1 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -54,6 +54,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.
> @@ -64,6 +65,9 @@ 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);
> +
> +#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
> @@ -76,6 +80,8 @@ void odp_execute_action_init(void);
>   */
>  int odp_action_scalar_init(struct odp_execute_action_impl *self);
>
> +int action_autoval_init(struct odp_execute_action_impl *self);
> +
>  int odp_execute_action_set(const char *name,
>                                 struct odp_execute_action_impl *active);
>
> -- 
> 2.32.0

Here is the full diff for the recommended changes:

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 237dcf19e..877281bcd 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -515,8 +515,9 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct dp_packet *b2,
         (b1->l2_5_ofs != b2->l2_5_ofs) ||
         (b1->l3_ofs != b2->l3_ofs) ||
         (b1->l4_ofs != b2->l4_ofs)) {
-            ds_put_format(err_str, "Packet offset comparison failed"
-                          "\n");
+        if (err_str) {
+            ds_put_format(err_str,
+                          "Packet offset comparison failed\n");
             ds_put_format(err_str, "Buffer 1 offsets: l2_pad_size %u,"
                           " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
                           b1->l2_pad_size, b1->l2_5_ofs,
@@ -525,6 +526,7 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct dp_packet *b2,
                           " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
                           b2->l2_pad_size, b2->l2_5_ofs,
                           b2->l3_ofs, b2->l4_ofs);
+        }
         return false;
     }
     return true;
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 666292037..6facb02a5 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -28,7 +28,6 @@

 VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
 static int active_action_impl_index;
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);

 static struct odp_execute_action_impl action_impls[] = {
     [ACTION_IMPL_AUTOVALIDATOR] = {
@@ -124,26 +123,25 @@ BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
 static void
 action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a)
 {
-    bool failed = false;
-    int type = nl_attr_type(a);
-    enum ovs_action_attr attr_type = (enum ovs_action_attr) type;
     struct odp_execute_action_impl *scalar = &action_impls[ACTION_IMPL_SCALAR];
-    struct dp_packet_batch good_batch;
+    enum ovs_action_attr attr_type = nl_attr_type(a);
+    struct dp_packet_batch original_batch;
+    bool failed = false;

-    dp_packet_batch_clone(&good_batch, batch);
+    dp_packet_batch_clone(&original_batch, batch);

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

     for (int impl = ACTION_IMPL_BEGIN; impl < ACTION_IMPL_MAX; impl++) {
         /* Clone original batch and execute implementation under test. */
         struct dp_packet_batch test_batch;

-        dp_packet_batch_clone(&test_batch, batch);
+        dp_packet_batch_clone(&test_batch, &original_batch);
         action_impls[impl].funcs[attr_type](&test_batch, a);

         /* Loop over implementations, checking each one. */
-        for (int pidx = 0; pidx < batch->count; pidx++) {
-            struct dp_packet *good_pkt = good_batch.packets[pidx];
+        for (int pidx = 0; pidx < original_batch.count; pidx++) {
+            struct dp_packet *good_pkt = batch->packets[pidx];
             struct dp_packet *test_pkt = test_batch.packets[pidx];

             struct ds log_msg = DS_EMPTY_INITIALIZER;
@@ -169,30 +167,30 @@ action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a)
                 failed = true;
             }

-            uint32_t good_hash = dp_packet_get_rss_hash(good_pkt);
-            uint32_t test_hash = dp_packet_get_rss_hash(test_pkt);
+            if (dp_packet_rss_valid(good_pkt)) {
+                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");
-                ds_put_format(&log_msg, "Good RSS hash : %u\n", good_hash);
-                ds_put_format(&log_msg, "Test RSS hash : %u\n", test_hash);
+                if (good_hash != test_hash) {
+                    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 = true;
+                    failed = true;
+                }
             }

             if (failed) {
-                VLOG_ERR_RL(&rl, "Autovalidation of %s failed. Details:\n%s",
-                            action_impls[impl].name, ds_cstr(&log_msg));
+                VLOG_ERR("Autovalidation of %s failed. Details:\n%s",
+                         action_impls[impl].name, ds_cstr(&log_msg));
                 ds_destroy(&log_msg);
                 failed = false;
             }
         }
-        dp_packet_delete_batch(&test_batch, 1);
+        dp_packet_delete_batch(&test_batch, true);
     }
-    dp_packet_delete_batch(&good_batch, 1);
-
-    /* Apply the action to the original batch for continued processing. */
-    scalar->funcs[attr_type](batch, a);
+    dp_packet_delete_batch(&original_batch, true);
 }

 int
@@ -200,7 +198,10 @@ action_autoval_init(struct odp_execute_action_impl *self)
 {
     /* Set function pointers for actions that can be applied directly, these
      * are identified by OVS_ACTION_ATTR_*. */
-    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_autoval_generic;
-
+    for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) {
+        if (action_impls[ACTION_IMPL_SCALAR].funcs[i]) {
+            self->funcs[i] = action_autoval_generic;
+        }
+    }
     return 0;
 }