[ovs-dev] [PATCH v2 1/5] dpif: implement subtable lookup validation
Harry van Haaren
harry.van.haaren at intel.com
Wed May 6 13:06:05 UTC 2020
This commit refactors the existing dpif subtable function pointer
infrastructure, and implements an autovalidator component.
The refactoring of the existing dpcls subtable lookup function
handling, making it more generic, and cleaning up how to enable
more implementations in future.
In order to ensure all implementations provide identical results,
the autovalidator is added. The autovalidator itself implements
the subtable lookup function prototype, but internally iterates
over all other available implementations. The end result is that
testing of each implementation becomes automatic, when the auto-
validator implementation is selected.
Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
---
I'm looking for a clean way to be able to select the autovalidator
implementation for all unit tests, while not require a recompile of
the codebase. The only change is to set the "autovalidator" lookup
function to the highest priority - that would provide the required
change in dpcls testing behaviour.
---
lib/automake.mk | 3 +
lib/dpif-netdev-lookup-autovalidator.c | 106 +++++++++++++++++++++++++
lib/dpif-netdev-lookup-generic.c | 7 +-
lib/dpif-netdev-lookup.c | 80 +++++++++++++++++++
lib/dpif-netdev-lookup.h | 75 +++++++++++++++++
lib/dpif-netdev-private.h | 15 ----
lib/dpif-netdev.c | 13 ++-
7 files changed, 276 insertions(+), 23 deletions(-)
create mode 100644 lib/dpif-netdev-lookup-autovalidator.c
create mode 100644 lib/dpif-netdev-lookup.c
create mode 100644 lib/dpif-netdev-lookup.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 86940ccd2..9dbc2bbc5 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -81,7 +81,10 @@ lib_libopenvswitch_la_SOURCES = \
lib/dp-packet.h \
lib/dp-packet.c \
lib/dpdk.h \
+ lib/dpif-netdev-lookup.h \
+ lib/dpif-netdev-lookup.c \
lib/dpif-netdev-lookup-generic.c \
+ lib/dpif-netdev-lookup-autovalidator.c \
lib/dpif-netdev.c \
lib/dpif-netdev.h \
lib/dpif-netdev-private.h \
diff --git a/lib/dpif-netdev-lookup-autovalidator.c b/lib/dpif-netdev-lookup-autovalidator.c
new file mode 100644
index 000000000..dbcaee36d
--- /dev/null
+++ b/lib/dpif-netdev-lookup-autovalidator.c
@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2020 Intel Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#include "dpif-netdev.h"
+#include "dpif-netdev-private.h"
+#include "dpif-netdev-lookup.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(dpif_lookup_autovalidator);
+
+/* This file implements an automated validator for subtable search
+ * implementations. It compares the results of the generic scalar search result
+ * with ISA optimized implementations.
+ *
+ * Note the goal is *NOT* to test the *specialized* versions of subtables, as
+ * the compiler performs the specialization - and we rely on the correctness of
+ * the compiler to not break those specialized variantes.
+ *
+ * The goal is to ensure identical results of the different implementations,
+ * despite that the implementations may have different methods to get those
+ * results.
+ *
+ * Example: AVX-512 ISA uses different instructions and algorithm to the scalar
+ * implementation, however the results (rules[] output) must be the same.
+ */
+
+static uint32_t
+dpcls_subtable_autovalidator(struct dpcls_subtable *subtable,
+ uint32_t keys_map,
+ const struct netdev_flow_key *keys[],
+ struct dpcls_rule **rules_good)
+{
+ const uint32_t u0_bit_count = subtable->mf_bits_set_unit0;
+ const uint32_t u1_bit_count = subtable->mf_bits_set_unit1;
+
+ /* Scalar generic - the "known correct" version */
+ dpcls_subtable_lookup_func lookup_good;
+ lookup_good = dpcls_subtable_generic_probe(u0_bit_count, u1_bit_count);
+
+ /* Run actual scalar implemenation to get known good results */
+ uint32_t matches_good = lookup_good(subtable, keys_map, keys, rules_good);
+
+ /* Now compare all other implementations against known good results.
+ * Note we start iterating from array[2], as 0 is autotester, and 1 is
+ * the known-good scalar implementation.
+ */
+ /* TODO: use BUILD_BUG_ON to check for i = 2 being correct? */
+
+ struct dpcls_subtable_lookup_info_t *lookup_funcs;
+ int32_t lookup_func_count = dpcls_subtable_lookup_info_get(&lookup_funcs);
+ if (lookup_func_count < 0) {
+ VLOG_ERR("failed to get lookup subtable function implementations\n");
+ return 0;
+ }
+
+ for (int i = 1; i < lookup_func_count; i++) {
+ dpcls_subtable_lookup_func lookup_func;
+ lookup_func = lookup_funcs[i].probe(u0_bit_count,
+ u1_bit_count);
+
+ /* If its probe returns a function, then test it */
+ if (lookup_func) {
+ struct dpcls_rule *rules_test[NETDEV_MAX_BURST];
+ size_t rules_size = sizeof(struct dpcls_rule *) * NETDEV_MAX_BURST;
+ memset(rules_test, 0, rules_size);
+ uint32_t matches_test = lookup_func(subtable, keys_map, keys,
+ rules_test);
+
+ /* Ensure same packets matched against subtable */
+ if (matches_good != matches_test) {
+ VLOG_ERR("matches_good %d != matches_test %d for func %s\n",
+ matches_good, matches_test, lookup_funcs[i].name);
+ }
+
+ /* Ensure rules matched are the same for scalar / others */
+ int j;
+ ULLONG_FOR_EACH_1 (j, matches_test) {
+ ovs_assert(rules_good[j] == rules_test[j]);
+ }
+ }
+ }
+
+ return matches_good;
+}
+
+dpcls_subtable_lookup_func
+dpcls_subtable_autovalidator_probe(uint32_t u0 OVS_UNUSED,
+ uint32_t u1 OVS_UNUSED)
+{
+ /* Always return the same validator tester, it works for all subtables */
+ return dpcls_subtable_autovalidator;
+}
diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-generic.c
index 89c8be0fa..7848f026f 100644
--- a/lib/dpif-netdev-lookup-generic.c
+++ b/lib/dpif-netdev-lookup-generic.c
@@ -18,6 +18,7 @@
#include <config.h>
#include "dpif-netdev.h"
#include "dpif-netdev-private.h"
+#include "dpif-netdev-lookup.h"
#include "bitmap.h"
#include "cmap.h"
@@ -254,7 +255,7 @@ lookup_generic_impl(struct dpcls_subtable *subtable,
}
/* Generic lookup function that uses runtime provided mf bits for iterating. */
-uint32_t
+static uint32_t
dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
uint32_t keys_map,
const struct netdev_flow_key *keys[],
@@ -310,6 +311,10 @@ dpcls_subtable_generic_probe(uint32_t u0_bits, uint32_t u1_bits)
if (f) {
VLOG_DBG("Subtable using Generic Optimized for u0 %d, u1 %d\n",
u0_bits, u1_bits);
+ } else {
+ /* Always return the generic function */
+ f = dpcls_subtable_lookup_generic;
}
+
return f;
}
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
new file mode 100644
index 000000000..2e9fb0abd
--- /dev/null
+++ b/lib/dpif-netdev-lookup.c
@@ -0,0 +1,80 @@
+
+#include <config.h>
+#include "dpif-netdev-lookup.h"
+
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(dpif_netdev_lookup);
+
+/* Actual list of implementations goes here */
+static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
+ /* Lowest priority - the auto testing implementation will not be used
+ * by default, it must be enabled by user */
+ { .prio = 0,
+ .probe = dpcls_subtable_autovalidator_probe,
+ .name = "autovalidator", },
+
+ /* Second lowest priority - the default scalar implementation */
+ { .prio = 1,
+ .probe = dpcls_subtable_generic_probe,
+ .name = "generic", },
+};
+
+int32_t
+dpcls_subtable_lookup_info_get(struct dpcls_subtable_lookup_info_t **out_ptr)
+{
+ if (out_ptr == NULL) {
+ return -1;
+ }
+
+ *out_ptr = subtable_lookups;
+ return ARRAY_SIZE(subtable_lookups);
+}
+
+/* sets the priority of the lookup function with "name" */
+int32_t
+dpcls_subtable_set_prio(const char *name, uint8_t priority)
+{
+ for (int i = 0; i < ARRAY_SIZE(subtable_lookups); i++) {
+ if (strcmp(name, subtable_lookups[i].name) == 0) {
+ subtable_lookups[i].prio = priority;
+ VLOG_INFO("Subtable function '%s' set priority to %d\n",
+ name, priority);
+ return 0;
+ }
+ }
+ VLOG_WARN("Subtable function '%s' not found, failed to set priority\n",
+ name);
+ return -EINVAL;
+}
+
+dpcls_subtable_lookup_func
+dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count)
+{
+ /* Iter over each subtable impl, and get highest priority one. */
+ int32_t prio = -1;
+ const char *name = 0;
+ dpcls_subtable_lookup_func best_func = NULL;
+
+ for (int i = 0; i < ARRAY_SIZE(subtable_lookups); i++) {
+ int32_t probed_prio = subtable_lookups[i].prio;
+ if (probed_prio > prio) {
+ dpcls_subtable_lookup_func probed_func;
+ probed_func = subtable_lookups[i].probe(u0_bit_count,
+ u1_bit_count);
+ if (probed_func) {
+ best_func = probed_func;
+ prio = probed_prio;
+ name = subtable_lookups[i].name;
+ }
+ }
+ }
+
+ VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority %d\n",
+ name, u0_bit_count, u1_bit_count, prio);
+
+ /* Programming error - we must always return a valid func ptr */
+ ovs_assert(best_func != NULL);
+
+ return best_func;
+}
diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h
new file mode 100644
index 000000000..61f44b9e8
--- /dev/null
+++ b/lib/dpif-netdev-lookup.h
@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2020 Intel Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef DPIF_NETDEV_LOOKUP_H
+#define DPIF_NETDEV_LOOKUP_H 1
+
+#include <config.h>
+#include "dpif-netdev.h"
+#include "dpif-netdev-private.h"
+
+/* Function to perform a probe for the subtable bit fingerprint.
+ * Returns NULL if not valid, or a valid function pointer to call for this
+ * subtable on success.
+ */
+typedef
+dpcls_subtable_lookup_func (*dpcls_subtable_probe_func)(uint32_t u0_bit_count,
+ uint32_t u1_bit_count);
+
+/* Prototypes for subtable implementations */
+dpcls_subtable_lookup_func
+dpcls_subtable_autovalidator_probe(uint32_t u0_bit_count,
+ uint32_t u1_bit_count);
+
+/* Probe function to select a specialized version of the generic lookup
+ * implementation. This provides performance benefit due to compile-time
+ * optimizations such as loop-unrolling. These are enabled by the compile-time
+ * constants in the specific function implementations.
+ */
+dpcls_subtable_lookup_func
+dpcls_subtable_generic_probe(uint32_t u0_bit_count, uint32_t u1_bit_count);
+
+
+/* Subtable registration and iteration helpers */
+struct dpcls_subtable_lookup_info_t {
+ /* higher priority gets used over lower values. This allows deployments
+ * to select the best implementation for the use-case.
+ */
+ uint8_t prio;
+
+ /* Probe function: tests if the (u0,u1) combo is supported. If not
+ * supported, this function returns NULL. If supported, a function pointer
+ * is returned which when called will perform the lookup on the subtable.
+ */
+ dpcls_subtable_probe_func probe;
+
+ /* Human readable name, used in setting subtable priority commands */
+ const char *name;
+};
+
+int32_t dpcls_subtable_set_prio(const char *name, uint8_t priority);
+
+dpcls_subtable_lookup_func
+dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count);
+
+/* Retrieve the array of lookup implementations for iteration.
+ * On error, returns a negative number.
+ * On success, returns the size of the arrays pointed to by the out parameter.
+ */
+int32_t
+dpcls_subtable_lookup_info_get(struct dpcls_subtable_lookup_info_t **out_ptr);
+
+#endif /* dpif-netdev-lookup.h */
diff --git a/lib/dpif-netdev-private.h b/lib/dpif-netdev-private.h
index 68c33a0f9..bdc150d45 100644
--- a/lib/dpif-netdev-private.h
+++ b/lib/dpif-netdev-private.h
@@ -60,21 +60,6 @@ uint32_t (*dpcls_subtable_lookup_func)(struct dpcls_subtable *subtable,
const struct netdev_flow_key *keys[],
struct dpcls_rule **rules);
-/* Prototype for generic lookup func, using generic scalar code path. */
-uint32_t
-dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
- uint32_t keys_map,
- const struct netdev_flow_key *keys[],
- struct dpcls_rule **rules);
-
-/* Probe function to select a specialized version of the generic lookup
- * implementation. This provides performance benefit due to compile-time
- * optimizations such as loop-unrolling. These are enabled by the compile-time
- * constants in the specific function implementations.
- */
-dpcls_subtable_lookup_func
-dpcls_subtable_generic_probe(uint32_t u0_bit_count, uint32_t u1_bit_count);
-
/* A set of rules that all have the same fields wildcarded. */
struct dpcls_subtable {
/* The fields are only used by writers. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 51c888501..d98541151 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -43,6 +43,7 @@
#include "dp-packet.h"
#include "dpif.h"
#include "dpif-netdev-perf.h"
+#include "dpif-netdev-lookup.h"
#include "dpif-provider.h"
#include "dummy.h"
#include "fat-rwlock.h"
@@ -8058,13 +8059,11 @@ dpcls_create_subtable(struct dpcls *cls, const struct netdev_flow_key *mask)
subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1));
netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1);
- /* Probe for a specialized generic lookup function. */
- subtable->lookup_func = dpcls_subtable_generic_probe(unit0, unit1);
-
- /* If not set, assign generic lookup. Generic works for any miniflow. */
- if (!subtable->lookup_func) {
- subtable->lookup_func = dpcls_subtable_lookup_generic;
- }
+ /* Get the preferred subtable search function for this (u0,u1) subtable.
+ * The function is gauranteed to always return a valid implementation, and
+ * possibly an ISA optimized, and/or specialized implementation.
+ */
+ subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1);
cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
/* Add the new subtable at the end of the pvector (with no hits yet) */
--
2.17.1
More information about the dev
mailing list