Modified the dplcs info-get command output to include
the count for different dpcls implementations.
$ovs-appctl dpif-netdev/subtable-lookup-prio-get
Available dpcls implementations:
autovalidator (Use count: 1, Priority: 5)
generic (Use count: 0, Priority: 1)
avx512_gather (Use count: 0, Priority: 3)
Test case to verify changes:
1021: PMD - dpcls configuration ok
Signed-off-by: Kumar Amber <kumar.am...@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
Signed-off-by: Eelco Chaudron <echau...@redhat.com>
Co-authored-by: Harry van Haaren <harry.van.haa...@intel.com>
Co-authored-by: Eelco Chaudron <echau...@redhat.com>
Acked-by: Eelco Chaudron <echau...@redhat.com>
---
v6:
- Rebase to master
v5:
- change the info-incr and decr APIs.
- Reduce the complexity of dpcls stats APIs.
v4:
- Fix comments on the patch.
- Change API from an overloaded method of counting, to returning the
old and new subtable structs. This allows the caller to identify the
modified subtable implementations, and update the statistics accordingly.
v3:
- Fix comments on the patch.
- Function API remains same, see discussion on OVS ML here:
"https://mail.openvswitch.org/pipermail/ovs-dev/2021-October/388737.html";
v2:
- Dependency merged rebased to master.
---
---
Documentation/topics/dpdk/bridge.rst | 16 ++---
lib/dpif-netdev-lookup.c | 98 +++++++++++++++++++++-------
lib/dpif-netdev-lookup.h | 18 ++++-
lib/dpif-netdev-private-dpcls.h | 1 +
lib/dpif-netdev.c | 38 ++++++-----
tests/pmd.at | 16 ++---
6 files changed, 127 insertions(+), 60 deletions(-)
diff --git a/Documentation/topics/dpdk/bridge.rst
b/Documentation/topics/dpdk/bridge.rst
index ceee91015..314c31a47 100644
--- a/Documentation/topics/dpdk/bridge.rst
+++ b/Documentation/topics/dpdk/bridge.rst
@@ -180,10 +180,10 @@ OVS provides multiple implementations of dpcls. The
following command enables
the user to check what implementations are available in a running instance::
$ ovs-appctl dpif-netdev/subtable-lookup-prio-get
- Available lookup functions (priority : name)
- 0 : autovalidator
- 1 : generic
- 0 : avx512_gather
+ Available dpcls implementations:
+ autovalidator (Use count: 1, Priority: 5)
+ generic (Use count: 0, Priority: 1)
+ avx512_gather (Use count: 0, Priority: 3)
To set the priority of a lookup function, run the ``prio-set`` command::
@@ -196,10 +196,10 @@ function due to the command being run. To verify the
prioritization, re-run the
get command, note the updated priority of the ``avx512_gather`` function::
$ ovs-appctl dpif-netdev/subtable-lookup-prio-get
- Available lookup functions (priority : name)
- 0 : autovalidator
- 1 : generic
- 5 : avx512_gather
+ Available dpcls implementations:
+ autovalidator (Use count: 1, Priority: 5)
+ generic (Use count: 0, Priority: 1)
+ avx512_gather (Use count: 0, Priority: 3)
If two lookup functions have the same priority, the first one in the list is
chosen, and the 2nd occurance of that priority is not used. Put in logical
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
index bd0a99abe..e641e4028 100644
--- a/lib/dpif-netdev-lookup.c
+++ b/lib/dpif-netdev-lookup.c
@@ -36,18 +36,21 @@ static struct dpcls_subtable_lookup_info_t
subtable_lookups[] = {
{ .prio = 0,
#endif
.probe = dpcls_subtable_autovalidator_probe,
- .name = "autovalidator", },
+ .name = "autovalidator",
+ .usage_cnt = ATOMIC_COUNT_INIT(0), },
/* The default scalar C code implementation. */
{ .prio = 1,
.probe = dpcls_subtable_generic_probe,
- .name = "generic", },
+ .name = "generic",
+ .usage_cnt = ATOMIC_COUNT_INIT(0), },
#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
/* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
{ .prio = 0,
.probe = dpcls_subtable_avx512_gather_probe,
- .name = "avx512_gather", },
+ .name = "avx512_gather",
+ .usage_cnt = ATOMIC_COUNT_INIT(0), },
#else
/* Disabling AVX512 at compile time, as compile time requirements not met.
* This could be due to a number of reasons:
@@ -64,7 +67,7 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[]
= {
#endif
};
-int32_t
+int
dpcls_subtable_lookup_info_get(struct dpcls_subtable_lookup_info_t **out_ptr)
{
if (out_ptr == NULL) {
@@ -76,7 +79,7 @@ dpcls_subtable_lookup_info_get(struct
dpcls_subtable_lookup_info_t **out_ptr)
}
/* sets the priority of the lookup function with "name". */
-int32_t
+int
dpcls_subtable_set_prio(const char *name, uint8_t priority)
{
for (int i = 0; i < ARRAY_SIZE(subtable_lookups); i++) {
@@ -93,32 +96,81 @@ 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)
+dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count,
+ struct dpcls_subtable_lookup_info_t **info)
{
- /* Iter over each subtable impl, and get highest priority one. */
- int32_t prio = -1;
- const char *name = NULL;
+ struct dpcls_subtable_lookup_info_t *best_info = NULL;
dpcls_subtable_lookup_func best_func = NULL;
+ int prio = -1;
+ /* Iter over each subtable impl, and get highest priority one. */
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;
- }
+ struct dpcls_subtable_lookup_info_t *impl_info = &subtable_lookups[i];
+ dpcls_subtable_lookup_func probed_func;
+
+ if (impl_info->prio <= prio) {
+ continue;
}
- }
- VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority %d\n",
- name, u0_bit_count, u1_bit_count, prio);
+ probed_func = subtable_lookups[i].probe(u0_bit_count,
+ u1_bit_count);
+ if (!probed_func) {
+ continue;
+ }
+
+ best_func = probed_func;
+ best_info = impl_info;
+ prio = impl_info->prio;
+ }
/* Programming error - we must always return a valid func ptr. */
- ovs_assert(best_func != NULL);
+ ovs_assert(best_func != NULL && best_info != NULL);
+ VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority %d\n",
+ best_info->name, u0_bit_count, u1_bit_count, prio);
+
+ if (info) {
+ *info = best_info;
+ }
return best_func;
}
+
+void
+dpcls_info_inc_usage(struct dpcls_subtable_lookup_info_t *info)
+{
+ if (info) {
+ atomic_count_inc(&info->usage_cnt);
+ }
+}
+
+void
+dpcls_info_dec_usage(struct dpcls_subtable_lookup_info_t *info)
+{
+ if (info) {
+ atomic_count_dec(&info->usage_cnt);
+ }
+}
+
+void
+dpcls_impl_print_stats(struct ds *reply)
+{
+ struct dpcls_subtable_lookup_info_t *lookup_funcs = NULL;
+ int count = dpcls_subtable_lookup_info_get(&lookup_funcs);
+
+ /* Add all DPCLS functions to reply string. */
+ ds_put_cstr(reply, "Available dpcls implementations:\n");
+
+ for (int i = 0; i < count; i++) {
+ ds_put_format(reply, " %s (Use count: %d, Priority: %d",
+ lookup_funcs[i].name,
+ atomic_count_get(&lookup_funcs[i].usage_cnt),
+ lookup_funcs[i].prio);
+
+ if (ds_last(reply) == ' ') {
+ ds_put_cstr(reply, "none");
+ }
+
+ ds_put_cstr(reply, ")\n");
+ }
+
+}
diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h
index 59f51faa0..7f124a46e 100644
--- a/lib/dpif-netdev-lookup.h
+++ b/lib/dpif-netdev-lookup.h
@@ -20,6 +20,7 @@
#include <config.h>
#include "dpif-netdev.h"
#include "dpif-netdev-private-dpcls.h"
+#include "dpif-netdev-private-thread.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
@@ -62,18 +63,29 @@ struct dpcls_subtable_lookup_info_t {
/* Human readable name, used in setting subtable priority commands */
const char *name;
+
+ /* Counter which holds the usage count of each implementations. */
+ atomic_count usage_cnt;
};
-int32_t dpcls_subtable_set_prio(const char *name, uint8_t priority);
+int dpcls_subtable_set_prio(const char *name, uint8_t priority);
+void dpcls_info_inc_usage(struct dpcls_subtable_lookup_info_t *info);
+void dpcls_info_dec_usage(struct dpcls_subtable_lookup_info_t *info);
+/* Lookup the best subtable lookup implementation for the given u0,u1 count. */
dpcls_subtable_lookup_func
-dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count);
+dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t u1_bit_count,
+ struct dpcls_subtable_lookup_info_t **info);
/* 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
+int
dpcls_subtable_lookup_info_get(struct dpcls_subtable_lookup_info_t **out_ptr);
+/* Prints dpcls subtables in use for different implementations. */
+void
+dpcls_impl_print_stats(struct ds *reply);
+
#endif /* dpif-netdev-lookup.h */
diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h
index 0d5da73c7..2a9279437 100644
--- a/lib/dpif-netdev-private-dpcls.h
+++ b/lib/dpif-netdev-private-dpcls.h
@@ -87,6 +87,7 @@ struct dpcls_subtable {
* can be used at any time by a PMD thread, so it's declared as an atomic
* here to prevent garbage from being read. */
ATOMIC(dpcls_subtable_lookup_func) lookup_func;
+ struct dpcls_subtable_lookup_info_t *lookup_func_info;
/* Caches the masks to match a packet to, reducing runtime calculations. */
uint64_t *mf_masks;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 88a5459cc..66336520b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1000,21 +1000,9 @@ dpif_netdev_subtable_lookup_get(struct unixctl_conn
*conn, int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED,
void *aux OVS_UNUSED)
{
- /* Get a list of all lookup functions. */
- struct dpcls_subtable_lookup_info_t *lookup_funcs = NULL;
- int32_t count = dpcls_subtable_lookup_info_get(&lookup_funcs);
- if (count < 0) {
- unixctl_command_reply_error(conn, "error getting lookup names");
- return;
- }
-
- /* Add all lookup functions to reply string. */
struct ds reply = DS_EMPTY_INITIALIZER;
- ds_put_cstr(&reply, "Available lookup functions (priority : name)\n");
- for (int i = 0; i < count; i++) {
- ds_put_format(&reply, " %d : %s\n", lookup_funcs[i].prio,
- lookup_funcs[i].name);
- }
+
+ dpcls_impl_print_stats(&reply);
unixctl_command_reply(conn, ds_cstr(&reply));
ds_destroy(&reply);
}
@@ -9699,6 +9687,7 @@ dpcls_destroy_subtable(struct dpcls *cls, struct
dpcls_subtable *subtable)
pvector_remove(&cls->subtables, subtable);
cmap_remove(&cls->subtables_map, &subtable->cmap_node,
subtable->mask.hash);
+ dpcls_info_dec_usage(subtable->lookup_func_info);
ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable);
}
@@ -9749,7 +9738,9 @@ dpcls_create_subtable(struct dpcls *cls, const struct
netdev_flow_key *mask)
* by the PMD thread.
*/
atomic_init(&subtable->lookup_func,
- dpcls_subtable_get_best_impl(unit0, unit1));
+ dpcls_subtable_get_best_impl(unit0, unit1,
+ &subtable->lookup_func_info));
+ dpcls_info_inc_usage(subtable->lookup_func_info);
cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
/* Add the new subtable at the end of the pvector (with no hits yet) */
@@ -9794,12 +9785,23 @@ dpcls_subtable_lookup_reprobe(struct dpcls *cls)
uint32_t u0_bits = subtable->mf_bits_set_unit0;
uint32_t u1_bits = subtable->mf_bits_set_unit1;
void *old_func = subtable->lookup_func;
-
+ struct dpcls_subtable_lookup_info_t *old_info;
+ old_info = subtable->lookup_func_info;
/* Set the subtable lookup function atomically to avoid garbage data
* being read by the PMD thread. */
atomic_store_relaxed(&subtable->lookup_func,
- dpcls_subtable_get_best_impl(u0_bits, u1_bits));
- subtables_changed += (old_func != subtable->lookup_func);
+ dpcls_subtable_get_best_impl(u0_bits, u1_bits,
+ &subtable->lookup_func_info));
+ if (old_func != subtable->lookup_func) {
+ subtables_changed += 1;
+ }
+
+ if (old_info != subtable->lookup_func_info) {
+ /* In theory, functions can be shared between implementations, so
+ * do an explicit check on the function info structures. */
+ dpcls_info_dec_usage(old_info);
+ dpcls_info_inc_usage(subtable->lookup_func_info);
+ }
}
return subtables_changed;
diff --git a/tests/pmd.at b/tests/pmd.at
index a2f9d34a2..968802249 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1093,11 +1093,11 @@ AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1
type=dummy-pmd])
AT_CHECK([ovs-vsctl show], [], [stdout])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [],
[dnl
- 1 : generic
+ generic (Use count: 0, Priority: 1)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep
autovalidator], [], [dnl
- 0 : autovalidator
+ autovalidator (Use count: 0, Priority: 0)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 3],
[0], [dnl
@@ -1105,7 +1105,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep
autovalidator], [], [dnl
- 3 : autovalidator
+ autovalidator (Use count: 0, Priority: 3)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 4], [0], [dnl
@@ -1113,7 +1113,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [],
[dnl
- 4 : generic
+ generic (Use count: 0, Priority: 4)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 8], [0], [dnl
@@ -1121,7 +1121,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [],
[dnl
- 8 : generic
+ generic (Use count: 0, Priority: 8)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 8],
[0], [dnl
@@ -1129,7 +1129,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep
autovalidator], [], [dnl
- 8 : autovalidator
+ autovalidator (Use count: 0, Priority: 8)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 0], [0], [dnl
@@ -1137,7 +1137,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [],
[dnl
- 0 : generic
+ generic (Use count: 0, Priority: 0)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 255], [0],
[dnl
@@ -1145,7 +1145,7 @@ Lookup priority change affected 0 dpcls ports and 0
subtables.
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [],
[dnl
- 255 : generic
+ generic (Use count: 0, Priority: 255)
])
AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic -1], [2],
--
2.25.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
