On 7/2/2019 4:02 PM, Van Haaren, Harry wrote:
-----Original Message-----
From: Stokes, Ian
Sent: Tuesday, July 2, 2019 3:40 PM
To: Van Haaren, Harry <harry.van.haa...@intel.com>; ovs-dev@openvswitch.org
Cc: i.maxim...@samsung.com
Subject: Re: [ovs-dev] [PATCH v9 1/5] dpif-netdev: implement function
pointers/subtable
On 5/15/2019 6:02 PM, Ian Stokes wrote:
On 5/8/2019 4:13 PM, Harry van Haaren wrote:
This allows plugging-in of different subtable hash-lookup-verify
routines, and allows special casing of those functions based on
known context (eg: # of bits set) of the specific subtable.
Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
---
v9:
- Use count_1bits in favour of __builtin_popcount (Ilya)
v6:
- Implement subtable effort per packet "lookups_match" counter (Ilya)
- Remove double newline (Eelco)
- Remove doubel * before comments (Eelco)
- Reword comments in dpcls_lookup() for clarity (Harry)
---
lib/dpif-netdev.c | 135 +++++++++++++++++++++++++++++++---------------
1 file changed, 93 insertions(+), 42 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5a6f2abac..e2e2c14e7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7572,6 +7572,27 @@ dpif_dummy_register(enum dummy_level level)
/* Datapath Classifier. */
+/* forward declaration for lookup_func typedef */
Minor nit above, general rule of thumb for comment style, start with
capital letter and finish with period.
+struct dpcls_subtable;
+
+/* Lookup function for a subtable in the dpcls. This function is called
+ * by each subtable with an array of packets, and a bitmask of
packets to
+ * perform the lookup on. Using a function pointer gives flexibility to
+ * optimize the lookup function based on subtable properties and the
+ * CPU instruction set available at runtime.
+ */
+typedef uint32_t (*dpcls_subtable_lookup_func)(struct dpcls_subtable
*subtable,
+ uint32_t keys_map, const struct netdev_flow_key *keys[],
+ struct dpcls_rule **rules);
Alignment for the arguments above looks odd, typically arguments are
kept vertically in line with one another *similar to
dpcls_subtable_lookup_generic below).
+
+/* Prototype for generic lookup func, using same code path as before */
Period at end of comment above.
+uint32_t
+dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
+ uint32_t keys_map,
+ const struct netdev_flow_key *keys[],
+ struct dpcls_rule **rules);
+
+
One minor follow up, extra whitespace seems to have been added here, cab
be reduced to 1 new line.
Will fix.
<snip>
+ uint32_t pkts_matched = count_1bits(found_map);
Just to clarify, at this point found_map has been returned and it only
contains matches where packets were found? i.e. it doesn't contain
matches from possible hash collisions?
+ lookups_match += pkts_matched * subtable_pos;
Hi Harry, can you clarify above why '* subtable_pos' is used? Is that
right? I'm just thinking that you already have the number of packets
matched from the count_1bits(found_map).
The "lookups match" counter variable name is a bit misleading, but I'd
change it in this patchset as its orthogonal to the DPCLS function pointer
concept.
The counter counts "number of subtables searched per hit packet", so to
speak. See Ilya's input and my response here:
https://www.mail-archive.com/ovs-dev@openvswitch.org/msg31442.html
This is a pseudo code of expected behavior:
1st subtable packet hits are += 1,
2nd subtable packet hits are += 2,
3rd subtable packet hits are += 3 etc.
At the end of a burst, we have a bitmask of packets hit, and a counter for
"subtable effort per packet matched".
Given two experienced OVS folks asked ~ the same question, hints that its
time for a cleanup & refactor, perhaps even a /* descriptive comment */ :)
Ya that makes more sense, confirms what i suspected :).
Sure a description of the purpose would help here in the next revision.
Thanks
Ian
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
