On 7/17/2019 11:29 AM, Van Haaren, Harry wrote:
-----Original Message-----
From: Stokes, Ian
Sent: Tuesday, July 16, 2019 10:07 PM
To: Van Haaren, Harry <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v11 5/5] dpif-netdev: Add specialized generic scalar
functions
On 7/15/2019 5:36 PM, Harry van Haaren wrote:
This commit adds a number of specialized functions, that handle
<snip>
Thanks for the v11 Harry, some minor comments inline below.
Thanks for review!
v11:
- Use MACROs to declare and check optimized functions (Ilya)
- Use captial letter for commit message (Ilya)
- Rebase onto latest patchset changes
- Added NEWS entry for data-path subtable specialization (Ian/Harry)
- Checkpatch notes an "incorrect bracketing" in the MACROs, however I
didn't find a solution that it does like.
v10:
- Rebase changes from previous patches.
- Remove "restrict" keyword as windows CI failed, see here for details:
https://ci.appveyor.com/project/istokes/ovs-q8fvv/builds/24398228
v8:
- Rework to use blocks_cache from the dpcls instance, to avoid variable
lenght arrays in the data-path.
---
NEWS | 4 +++
lib/dpif-netdev-lookup-generic.c | 51 ++++++++++++++++++++++++++++++++
lib/dpif-netdev-private.h | 8 +++++
lib/dpif-netdev.c | 9 ++++--
4 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/NEWS b/NEWS
index 81130e667..4cfffb1bc 100644
--- a/NEWS
+++ b/NEWS
@@ -34,6 +34,10 @@ Post-v2.11.0
* 'ovs-appctl exit' now implies cleanup of non-internal ports in
userspace
datapath regardless of '--cleanup' option. Use '--cleanup' to
remove
internal ports too.
+ * Datapath classifer code refactored to enable function pointers to
select
+ the lookup implementation at runtime. This enables specialization of
+ specific subtables based on the miniflow attributes, enhancing the
+ performance of the subtable search.
- OVSDB:
* OVSDB clients can now resynchronize with clustered servers much
more
quickly after a brief disconnection, saving bandwidth and CPU time.
diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-
generic.c
index abd166fc3..259c36645 100644
--- a/lib/dpif-netdev-lookup-generic.c
+++ b/lib/dpif-netdev-lookup-generic.c
@@ -233,7 +233,58 @@ dpcls_subtable_lookup_generic(struct dpcls_subtable
*subtable,
const struct netdev_flow_key *keys[],
struct dpcls_rule **rules)
{
+ /* Here the runtime subtable->mf_bits counts are used, which forces the
+ * compiler to iterate normal for() loops. Due to this limitation in
the
+ * compilers available optimizations, this function has lower
performance
+ * than the below specialized functions.
+ */
return lookup_generic_impl(subtable, blocks_scratch, keys_map, keys,
rules,
subtable->mf_bits_set_unit0,
subtable->mf_bits_set_unit1);
}
+
+/* Expand out specialized functions with U0 and U1 bit attributes */
Minor, missing period at end of comment (can fix this on commit if there
are no other revisions).
Fixed.
<snip>
+DECLARE_OPTIMIZED_LOOKUP_FUNCTION(5,1)
+DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,1)
+DECLARE_OPTIMIZED_LOOKUP_FUNCTION(4,0)
+
+/* Check if a speicalized function is valid for the required subtable. */
Minor, speicalized -> specialized, again can be fixed on commit otherwise.
Fixed.
+#define CHECK_LOOKUP_FUNCTION(U0,U1)
\
+ if (!f && u0_bits == U0 && u1_bits == U1) {
\
+ f = dpcls_subtable_lookup_mf_u0w##U0##_u1w##U1;
\
+ }
+
+/* Probe function to lookup an available specialized function.
+ * If capable to run the requested miniflow fingerprint, this function
returns
+ * the most optimal implementation for that miniflow fingerprint.
+ * @retval FunctionAddress A valid function to handle the miniflow bit
pattern
+ * @retval 0 The requested miniflow is not supported here, NULL is returned
+ */
+dpcls_subtable_lookup_func
+dpcls_subtable_generic_probe(uint32_t u0_bits, uint32_t u1_bits)
+{
+ dpcls_subtable_lookup_func f = NULL;
In the comments you return FunctionAddress but you return f below,
should this not be FunctionAddress or maybe another variable name if
'FunctionAddress' is a bit unwieldy?
Updated return value descriptions as Non-NULL and NULL, and updated comments to
read well. This better describes the code than "FunctionAddress".
<snip>
- /* Assign the generic lookup - this works with any miniflow
fingerprint. */
- subtable->lookup_func = dpcls_subtable_lookup_generic;
+ /* 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;
+ }
cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
/* Add the new subtable at the end of the pvector (with no hits yet)
*/
Missing period above.
I'd prefer not fix this one - that code is patch context and isn't currently
being modified.
Ah, good catch, sounds good to me.
Ian
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
