[ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract

Eelco Chaudron echaudro at redhat.com
Tue Jun 29 11:43:43 UTC 2021



On 17 Jun 2021, at 18:27, Kumar Amber wrote:

> This patch introduced the auto-validation function which
> allows users to compare the batch of packets obtained from
> different miniflow implementations against the linear
> miniflow extract and return a hitmask.
>
> The autovaidator function can be triggered at runtime using the
> following command:
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator
>
> Signed-off-by: Kumar Amber <kumar.amber at intel.com>
> Co-authored-by: Harry van Haaren <harry.van.haaren at intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> ---
>  lib/dpif-netdev-private-extract.c | 141 
> ++++++++++++++++++++++++++++++
>  lib/dpif-netdev-private-extract.h |  15 ++++
>  lib/dpif-netdev.c                 |   2 +-
>  3 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev-private-extract.c 
> b/lib/dpif-netdev-private-extract.c
> index fcc56ef26..0741c19f9 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -32,6 +32,11 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev_extract);
>
>  /* Implementations of available extract options. */
>  static struct dpif_miniflow_extract_impl mfex_impls[] = {
> +   {
> +        .probe = NULL,
> +        .extract_func = dpif_miniflow_extract_autovalidator,
> +        .name = "autovalidator",
> +    },
>      {
>          .probe = NULL,
>          .extract_func = NULL,
> @@ -84,3 +89,139 @@ dpif_miniflow_extract_info_get(struct 
> dpif_miniflow_extract_impl **out_ptr)
>      *out_ptr = mfex_impls;
>      return ARRAY_SIZE(mfex_impls);
>  }
> +
> +uint32_t
> +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets,
> +                                    struct netdev_flow_key *keys,
> +                                    uint32_t keys_size, odp_port_t 
> in_port,
> +                                    void *pmd_handle)
> +{
> +    const size_t cnt = dp_packet_batch_size(packets);
> +    uint16_t good_l2_5_ofs[NETDEV_MAX_BURST];
> +    uint16_t good_l3_ofs[NETDEV_MAX_BURST];
> +    uint16_t good_l4_ofs[NETDEV_MAX_BURST];
> +    uint16_t good_l2_pad_size[NETDEV_MAX_BURST];
> +    struct dp_packet *packet;
> +    struct dp_netdev_pmd_thread *pmd = pmd_handle;
> +    struct dpif_miniflow_extract_impl *miniflow_funcs;
> +
> +    int32_t mfunc_count = 
> dpif_miniflow_extract_info_get(&miniflow_funcs);
> +    if (mfunc_count < 0) {

In theory 0 could not be returned, but just to cover the corner case can 
we change this to include zero.

> +        pmd->miniflow_extract_opt = NULL;

Guess the above needs to be atomic.

> +        VLOG_ERR("failed to get miniflow extract function 
> implementations\n");

Capital F to be in sync with your other error messages?

> +        return 0;
> +    }
> +    ovs_assert(keys_size >= cnt);

I don’t think we should assert here. Just return an error like above, 
so in production, we get notified, and this implementation gets 
disabled.

> +    struct netdev_flow_key test_keys[NETDEV_MAX_BURST];
> +
> +    /* Run scalar miniflow_extract to get default result. */
> +    DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +        pkt_metadata_init(&packet->md, in_port);
> +        miniflow_extract(packet, &keys[i].mf);
> +
> +        /* Store known good metadata to compare with optimized 
> metadata. */
> +        good_l2_5_ofs[i] = packet->l2_5_ofs;
> +        good_l3_ofs[i] = packet->l3_ofs;
> +        good_l4_ofs[i] = packet->l4_ofs;
> +        good_l2_pad_size[i] = packet->l2_pad_size;
> +    }
> +
> +    /* Iterate through each version of miniflow implementations. */
> +    for (int j = MFEX_IMPL_START_IDX; j < ARRAY_SIZE(mfex_impls); 
> j++) {
> +        if (!mfex_impls[j].available) {
> +            continue;
> +        }
> +
> +        /* Reset keys and offsets before each implementation. */
> +        memset(test_keys, 0, keys_size * sizeof(struct 
> netdev_flow_key));
> +        DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +            dp_packet_reset_offsets(packet);
> +        }
> +        /* Call optimized miniflow for each batch of packet. */
> +        uint32_t hit_mask = mfex_impls[j].extract_func(packets, 
> test_keys,
> +                                            keys_size, in_port, 
> pmd_handle);
> +
> +        /* Do a miniflow compare for bits, blocks and offsets for all 
> the
> +         * classified packets in the hitmask marked by set bits. */
> +        while (hit_mask) {
> +            /* Index for the set bit. */
> +            uint32_t i = __builtin_ctz(hit_mask);
> +            /* Set the index in hitmask to Zero. */
> +            hit_mask &= (hit_mask - 1);
> +
> +            uint32_t failed = 0;
> +
> +            /* Check miniflow bits are equal. */
> +            if ((keys[i].mf.map.bits[0] != 
> test_keys[i].mf.map.bits[0]) ||
> +                (keys[i].mf.map.bits[1] != 
> test_keys[i].mf.map.bits[1])) {
> +                VLOG_ERR("Good 0x%llx 0x%llx\tTest 0x%llx 0x%llx\n",
> +                         keys[i].mf.map.bits[0], 
> keys[i].mf.map.bits[1],
> +                         test_keys[i].mf.map.bits[0],
> +                         test_keys[i].mf.map.bits[1]);
> +                failed = 1;
> +            }
> +
> +            if (!miniflow_equal(&keys[i].mf, &test_keys[i].mf)) {
> +                uint32_t block_cnt = miniflow_n_values(&keys[i].mf);
> +                VLOG_ERR("Autovalidation blocks failed for %s pkt 
> %d",
> +                         mfex_impls[j].name, i);
> +                VLOG_ERR("  Good hexdump:\n");
> +                uint64_t *good_block_ptr = (uint64_t *)&keys[i].buf;
> +                uint64_t *test_block_ptr = (uint64_t 
> *)&test_keys[i].buf;
> +                for (uint32_t b = 0; b < block_cnt; b++) {
> +                    VLOG_ERR("    %"PRIx64"\n", good_block_ptr[b]);
> +                }
> +                VLOG_ERR("  Test hexdump:\n");
> +                for (uint32_t b = 0; b < block_cnt; b++) {
> +                    VLOG_ERR("    %"PRIx64"\n", test_block_ptr[b]);
> +                }
> +                failed = 1;
> +            }
> +
> +            if ((packets->packets[i]->l2_pad_size != 
> good_l2_pad_size[i]) ||
> +                    (packets->packets[i]->l2_5_ofs != 
> good_l2_5_ofs[i]) ||
> +                    (packets->packets[i]->l3_ofs != good_l3_ofs[i]) 
> ||
> +                    (packets->packets[i]->l4_ofs != good_l4_ofs[i])) 
> {
> +                VLOG_ERR("Autovalidation packet offsets failed for %s 
> pkt %d",
> +                         mfex_impls[j].name, i);
> +                VLOG_ERR("  Good offsets: l2_pad_size %u, l2_5_ofs : 
> %u"
> +                         " l3_ofs %u, l4_ofs %u\n",
> +                         good_l2_pad_size[i], good_l2_5_ofs[i],
> +                         good_l3_ofs[i], good_l4_ofs[i]);
> +                VLOG_ERR("  Test offsets: l2_pad_size %u, l2_5_ofs : 
> %u"
> +                         " l3_ofs %u, l4_ofs %u\n",
> +                         packets->packets[i]->l2_pad_size,
> +                         packets->packets[i]->l2_5_ofs,
> +                         packets->packets[i]->l3_ofs,
> +                         packets->packets[i]->l4_ofs);
> +                failed = 1;
> +            }
> +
> +            if (failed) {

Why stop now!? I think we should run all implementations, as others 
might need fixing too!

> +                /* Having dumped the debug info, disable 
> autovalidator. */
> +                VLOG_ERR("Autovalidation failed in %s pkt %d, 
> disabling.\n",
> +                         mfex_impls[j].name, i);
> +                /* Halt OVS here on debug builds. */
> +                ovs_assert(0);
> +                pmd->miniflow_extract_opt = NULL;
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* preserve packet correctness by storing back the good offsets 
> in
> +     * packets back. */
> +    DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +        packet->l2_5_ofs = good_l2_5_ofs[i];
> +        packet->l3_ofs = good_l3_ofs[i];
> +        packet->l4_ofs = good_l4_ofs[i];
> +        packet->l2_pad_size = good_l2_pad_size[i];
> +    }
> +
> +    /* Returning zero implies no packets were hit by autovalidation. 
> This
> +     * simplifies unit-tests as changing 
> --enable-mfex-default-autovalidator
> +     * would pass/fail. By always returning zero, autovalidator is a 
> little
> +     * slower, but we gain consistency in testing.
> +     */
> +    return 0;
> +}
> diff --git a/lib/dpif-netdev-private-extract.h 
> b/lib/dpif-netdev-private-extract.h
> index b7b0b2be4..455a7b590 100644
> --- a/lib/dpif-netdev-private-extract.h
> +++ b/lib/dpif-netdev-private-extract.h
> @@ -24,6 +24,11 @@
>  /* Max size of dpif_miniflow_extract_impl array. */
>  #define MFEX_IMPLS_MAX_SIZE (16)
>
> +/* Skip the autovalidator study and null when iterating all available
> + * miniflow implementations.
> + */
> +#define MFEX_IMPL_START_IDX (1)
> +
>  /* Forward declarations. */
>  struct dp_packet;
>  struct miniflow;
> @@ -90,5 +95,15 @@ dpif_miniflow_extract_init(void);
>  int32_t
>  dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl 
> **out_ptr);


Forgot to comment on this in my previous patch:

/* Function pointer prototype to be implemented in the optimized 
miniflow
  * extract code.
  * returns the hitmask of the processed packets on success.
  * returns zero on failure.
  */
typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch *batch,
                                           struct netdev_flow_key *keys,
                                           uint32_t keys_size,
                                           odp_port_t in_port,
                                           void *pmd_handle);


Maybe we could add some description of what the input parameters mean, 
are expected? For example that key_size need to be at least the size of 
the batch? If this is the case, do we need it, and can we just assume 
keys should be at least batch size long?

> +/* Retrieve the hitmask of the batch of pakcets which is obtained by 
> comparing
> + * different miniflow implementations with linear miniflow extract.
> + * On error, returns a zero.
> + * On success, returns the number of packets in the batch compared.
> + */
> +uint32_t
> +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *batch,
> +                                    struct netdev_flow_key *keys,
> +                                    uint32_t keys_size, odp_port_t 
> in_port,
> +                                    void *pmd_handle);
>
>  #endif /* DPIF_NETDEV_AVX512_EXTRACT */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 567ebd952..4f4ab2790 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1181,8 +1181,8 @@ dpif_miniflow_extract_impl_set(struct 
> unixctl_conn *conn, int argc,
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      ds_put_format(&reply, "Miniflow implementation set to %s.\n", 
> mfex_name);
>      const char *reply_str = ds_cstr(&reply);
> -    unixctl_command_reply(conn, reply_str);
>      VLOG_INFO("%s", reply_str);
> +    unixctl_command_reply(conn, reply_str);
>      ds_destroy(&reply);
>  }
>
> -- 
> 2.25.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list