[ovs-dev] [v6 02/11] dpif-netdev: Add auto validation function for miniflow extract
Eelco Chaudron
echaudro at redhat.com
Wed Jul 7 10:42:52 UTC 2021
On 6 Jul 2021, at 15:11, Cian Ferriter wrote:
> From: Kumar Amber <kumar.amber at intel.com>
>
> 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>
>
> ---
>
> v5:
> - fix review comments(Ian, Flavio, Eelco)
> - remove ovs assert and switch to default after a batch of packets
> is processed
> - Atomic set and get introduced
> - fix raw_ctz for windows build
> ---
> ---
> NEWS | 2 +
> lib/dpif-netdev-private-extract.c | 149
> ++++++++++++++++++++++++++++++
> lib/dpif-netdev-private-extract.h | 13 +++
> lib/dpif-netdev.c | 2 +-
> 4 files changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 60db823c4..ccf9a0f1e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,6 +23,8 @@ Post-v2.15.0
> CPU supports it. This enhances performance by using the native
> vpopcount
> instructions, instead of the emulated version of vpopcount.
> * Add command line option to switch between mfex function
> pointers.
> + * Add miniflow extract auto-validator function to compare
> different
> + miniflow extract implementations against default
> implementation.
> - ovs-ctl:
> * New option '--no-record-hostname' to disable hostname
> configuration
> in ovsdb on startup.
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> index f7ad2d5b5..62170ff6c 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -38,6 +38,11 @@ static miniflow_extract_func default_mfex_func =
> NULL;
> */
> static struct dpif_miniflow_extract_impl mfex_impls[] = {
>
> + [MFEX_IMPL_AUTOVALIDATOR] = {
> + .probe = NULL,
> + .extract_func = dpif_miniflow_extract_autovalidator,
> + .name = "autovalidator", },
> +
> [MFEX_IMPL_SCALAR] = {
> .probe = NULL,
> .extract_func = NULL,
> @@ -157,3 +162,147 @@ dp_mfex_impl_get_by_name(const char *name,
> miniflow_extract_func *out_func)
>
> return -EINVAL;
> }
> +
> +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,
> + struct dp_netdev_pmd_thread
> *pmd_handle)
> +{
> + const uint32_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 netdev_flow_key test_keys[NETDEV_MAX_BURST];
> +
> + if (keys_size < cnt) {
> + miniflow_extract_func default_func = NULL;
> + atomic_uintptr_t *pmd_func = (void
> *)&pmd->miniflow_extract_opt;
> + atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> + VLOG_ERR("Invalid key size supplied, Key_size: %d less than"
> + "batch_size: %d", keys_size, cnt);
> + return 0;
> + }
> +
> + /* 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;
> + }
> +
> + uint32_t batch_failed = 0;
> + /* Iterate through each version of miniflow implementations. */
> + for (int j = MFEX_IMPL_MAX; j < MFEX_IMPL_MAX; j++) {
This is really confusing ;) Maybe we could define an enum for the first
implementation, as you had in your previous patch, MFEX_IMPL_START_IDX?
So that even if someone changes the enum order it’s clear? Maybe with
a separate #define or doing something like:
enum dpif_miniflow_extract_impl_idx {
MFEX_IMPL_AUTOVALIDATOR,
MFEX_IMPL_SCALAR,
MFEX_IMPL_START_IDX,
MFEX_IMPL_MAX = MFEX_IMPL_START_IDX
};
> + if ((j < MFEX_IMPL_MAX) || (!mfex_impls[j].available)) {
Why the (j < MFEX_IMPL_MAX) so the below code is always skipped? So the
validator is not executed ever?
> + 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 = raw_ctz(hit_mask);
> + /* Set the index in hitmask to Zero. */
> + hit_mask &= (hit_mask - 1);
> +
> + uint32_t failed = 0;
> +
> + struct ds log_msg = DS_EMPTY_INITIALIZER;
> + ds_put_format(&log_msg, "mfex autovalidator pkt %d\n",
> i);
Capital MFEX?
> + /* 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])) {
In this code we assume FLOWMAP_UNITS == 2, can we add a static assert to
make sure this is not changing.
There is also flowmap_equal() which might be better, but the assert
might still be needed for the below logging.
> + ds_put_format(&log_msg, "Good 0x%llx 0x%llx\tTest
> 0x%llx"
Think here we need some more details like for the ones below:
dd_put_format(&log_msg, "Autovalidation map failed\n”
“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);
> + ds_put_format(&log_msg, "Autovalidation blocks failed
> for %s"
> + "pkt %d\nGood hex:\n",
> mfex_impls[j].name, i);
To save on log file content, we can remove the packet and name, as
it’s already logged on error in the if(failed) condition below.
So it will become:
ds_put_format(&log_msg, "Autovalidation blocks failed\n”
"nGood hex:\n", mfex_impls[j].name, i);
> + ds_put_hex_dump(&log_msg, &keys[i].buf, block_cnt *
> 8, 0,
> + false);
> + ds_put_format(&log_msg, "Test hex:\n");
> + ds_put_hex_dump(&log_msg, &test_keys[i].buf,
> block_cnt * 8, 0,
> + false);
> + failed = 1;
> + }
> +
> + packet = packets->packets[i];
> + if ((packet->l2_pad_size != good_l2_pad_size[i]) ||
> + (packet->l2_5_ofs != good_l2_5_ofs[i]) ||
> + (packet->l3_ofs != good_l3_ofs[i]) ||
> + (packet->l4_ofs != good_l4_ofs[i])) {
> + ds_put_format(&log_msg, "Autovalidation packet
> offsets failed"
> + " for %s pkt %d\n", mfex_impls[j].name,
> i);
To save on log file content, we can remove the packet and name, as
it’s already logged on error in the if(failed) condition below.
> + ds_put_format(&log_msg, "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]);
> + ds_put_format(&log_msg, " Test offsets: l2_pad_size
> %u,"
> + " l2_5_ofs : %u l3_ofs %u, l4_ofs
> %u\n",
> + packet->l2_pad_size, packet->l2_5_ofs,
> + packet->l3_ofs, packet->l4_ofs);
> + failed = 1;
> + }
> +
> + if (failed) {
> + VLOG_ERR("Autovalidation for %s failed in pkt %d,"
> + " disabling.", mfex_impls[j].name, i);
> + VLOG_ERR("Autovalidation failure details:\n%s",
> + ds_cstr(&log_msg));
> + batch_failed = 1;
> + }
> + ds_destroy(&log_msg);
> + }
> + }
> +
> + /* Having dumped the debug info for the batch, disable
> autovalidator. */
> + if (batch_failed) {
> + miniflow_extract_func default_func = NULL;
> + atomic_uintptr_t *pmd_func = (void
> *)&pmd->miniflow_extract_opt;
> + atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> + }
> +
> + /* 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. The auto-validator
> is only
> + * meant to test different implementaions against a batch of
> packets
> + * without incrementing hit counters.
> + */
> + return 0;
> +}
> diff --git a/lib/dpif-netdev-private-extract.h
> b/lib/dpif-netdev-private-extract.h
> index 074b3ee16..10525c378 100644
> --- a/lib/dpif-netdev-private-extract.h
> +++ b/lib/dpif-netdev-private-extract.h
> @@ -69,6 +69,7 @@ struct dpif_miniflow_extract_impl {
> * should always come before the generic AVX512-F version.
> */
> enum dpif_miniflow_extract_impl_idx {
> + MFEX_IMPL_AUTOVALIDATOR,
> MFEX_IMPL_SCALAR,
> MFEX_IMPL_MAX
> };
> @@ -102,4 +103,16 @@ int32_t dp_mfex_impl_set_default_by_name(const
> char *name);
> void
> dpif_miniflow_extract_init(void);
>
> +/* Retrieve the hitmask of the batch of pakcets which is obtained by
> comparing
> + * different miniflow implementations with linear miniflow extract.
> + * Key_size need to be at least the size of the batch.
> + * 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,
> + struct dp_netdev_pmd_thread
> *pmd_handle);
> +
> #endif /* MFEX_AVX512_EXTRACT */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 2043c9ba2..175d8699f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1156,8 +1156,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.32.0
More information about the dev
mailing list