Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function
On 25 Mar 2019, at 17:30, Van Haaren, Harry wrote: -Original Message- From: Ilya Maximets [mailto:i.maxim...@samsung.com] Sent: Friday, March 22, 2019 1:19 PM To: Van Haaren, Harry ; Eelco Chaudron Cc: ovs-dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function On 22.03.2019 16:08, Van Haaren, Harry wrote: Hey Eelco, -Original Message- I'll work on addressing your comments early next week - thanks again! -H Hi Harry, I didn't look at this series closely yet. But if you're going to prepare new version, it'll be good to fix checkpatch issues reported by 0-day robot too: https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html BTW, commented out counters looks suspicious. Best regards, Ilya Maximets. Thanks Ilya, fixing checkpatch issues now, no problems. Correct the lookups_match counter isn't implemented yet - will add it in v6 implementation. The name of the variable is a bit misleading - it really counts the number of subtables searched for a match, per packet that it found a hit for. Eg: a packet that matched the 3rd subtable searched, will += counter by 3. So really, it counts "subtable-effort per packet match". Does this align with you understanding of it? Thanks, -Harry Ran the PVP tests over the weekend and the results look good. The Zero loss ones have quite some deviation, so I would ignore them. Here is a link to the results: https://docs.google.com/spreadsheets/u/0/d/e/2PACX-1vSrHgy18vTWFWk_SCF4vggtr1qpimMUkhZuHCSQtmEQi77yCjPrxlAr4hvNvtuIO3Vzzsy31bls2_wP/pubhtml?skip_itp2_check=true# Will continue the review on v6. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function
> -Original Message- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Friday, March 22, 2019 1:19 PM > To: Van Haaren, Harry ; Eelco Chaudron > > Cc: ovs-dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup > function > > On 22.03.2019 16:08, Van Haaren, Harry wrote: > > Hey Eelco, > > > >> -Original Message- > > I'll work on addressing your comments early next week - thanks again! -H > > Hi Harry, > > I didn't look at this series closely yet. But if you're going to prepare new > version, it'll be good to fix checkpatch issues reported by 0-day robot too: > > https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html > https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html > > BTW, commented out counters looks suspicious. > > Best regards, Ilya Maximets. Thanks Ilya, fixing checkpatch issues now, no problems. Correct the lookups_match counter isn't implemented yet - will add it in v6 implementation. The name of the variable is a bit misleading - it really counts the number of subtables searched for a match, per packet that it found a hit for. Eg: a packet that matched the 3rd subtable searched, will += counter by 3. So really, it counts "subtable-effort per packet match". Does this align with you understanding of it? Thanks, -Harry ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function
On 22.03.2019 16:08, Van Haaren, Harry wrote: > Hey Eelco, > >> -Original Message- >> From: Eelco Chaudron [mailto:echau...@redhat.com] >> Sent: Friday, March 22, 2019 12:29 PM >> To: Van Haaren, Harry >> Cc: ovs-dev@openvswitch.org; i.maxim...@samsung.com >> Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup >> function >> >> On 25 Feb 2019, at 17:18, Harry van Haaren wrote: >> >>> This commit splits the generic hash-lookup-verify >>> function to its own file. In doing so, we must move >>> some MACRO definitions to dpif-netdev.h >> >> Changes look good, but want to wait for some performance numbers before >> ack’ing the series. >> >> Did you do any performance tests on the change? It does not seem to >> impact much, but I’ve been surprised before. > > Thanks for the review - I'll take the comments into account for the V+1. > > Regarding performance, I expect there to be no performance delta with > this patchset - it is just enabling flexibility in the dpcls infrastructure. > > As presented at OvS conf last November, with the flexibility mentioned above > we can select optimized implementations at runtime, and that is where the > performance gains will be seen. > > > I'll work on addressing your comments early next week - thanks again! -H Hi Harry, I didn't look at this series closely yet. But if you're going to prepare new version, it'll be good to fix checkpatch issues reported by 0-day robot too: https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html BTW, commented out counters looks suspicious. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function
Hey Eelco, > -Original Message- > From: Eelco Chaudron [mailto:echau...@redhat.com] > Sent: Friday, March 22, 2019 12:29 PM > To: Van Haaren, Harry > Cc: ovs-dev@openvswitch.org; i.maxim...@samsung.com > Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup > function > > On 25 Feb 2019, at 17:18, Harry van Haaren wrote: > > > This commit splits the generic hash-lookup-verify > > function to its own file. In doing so, we must move > > some MACRO definitions to dpif-netdev.h > > Changes look good, but want to wait for some performance numbers before > ack’ing the series. > > Did you do any performance tests on the change? It does not seem to > impact much, but I’ve been surprised before. Thanks for the review - I'll take the comments into account for the V+1. Regarding performance, I expect there to be no performance delta with this patchset - it is just enabling flexibility in the dpcls infrastructure. As presented at OvS conf last November, with the flexibility mentioned above we can select optimized implementations at runtime, and that is where the performance gains will be seen. I'll work on addressing your comments early next week - thanks again! -H ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup function
On 25 Feb 2019, at 17:18, Harry van Haaren wrote:
This commit splits the generic hash-lookup-verify
function to its own file. In doing so, we must move
some MACRO definitions to dpif-netdev.h
Changes look good, but want to wait for some performance numbers before
ack’ing the series.
Did you do any performance tests on the change? It does not seem to
impact much, but I’ve been surprised before.
Signed-off-by: Harry van Haaren
---
lib/automake.mk | 1 +
lib/dpif-netdev-lookup-generic.c | 95
lib/dpif-netdev.c| 81 +--
lib/dpif-netdev.h| 16 ++
4 files changed, 113 insertions(+), 80 deletions(-)
create mode 100644 lib/dpif-netdev-lookup-generic.c
diff --git a/lib/automake.mk b/lib/automake.mk
index bae032bd8..3a5baf2b8 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -78,6 +78,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/dp-packet.h \
lib/dp-packet.c \
lib/dpdk.h \
+ lib/dpif-netdev-lookup-generic.c \
lib/dpif-netdev.c \
lib/dpif-netdev.h \
lib/dpif-netdev-perf.c \
diff --git a/lib/dpif-netdev-lookup-generic.c
b/lib/dpif-netdev-lookup-generic.c
new file mode 100644
index 0..8ae8ff59d
--- /dev/null
+++ b/lib/dpif-netdev-lookup-generic.c
@@ -0,0 +1,95 @@
+/*
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016, 2017
Nicira, Inc.
+ *
+ * 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
+#include "dpif-netdev.h"
+
+#include "bitmap.h"
+#include "cmap.h"
+
+#include "dp-packet.h"
+#include "dpif.h"
+#include "dpif-netdev-perf.h"
+#include "dpif-provider.h"
+#include "flow.h"
+#include "packets.h"
+#include "pvector.h"
+
+/* Returns a hash value for the bits of 'key' where there are 1-bits
in
+ * 'mask'. */
+static inline uint32_t
+netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
+ const struct netdev_flow_key *mask)
+{
+const uint64_t *p = miniflow_get_values(>mf);
+uint32_t hash = 0;
+uint64_t value;
+
+NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
+hash = hash_add64(hash, value & *p);
+p++;
+}
+
+return hash_finish(hash, (p - miniflow_get_values(>mf)) *
8);
+}
+
+uint32_t
+dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
+ uint32_t keys_map,
+ const struct netdev_flow_key *keys[],
+ struct dpcls_rule **rules)
+{
+int i;
+/* Compute hashes for the remaining keys. Each search-key is
+ * masked with the subtable's mask to avoid hashing the
wildcarded
+ * bits. */
+uint32_t hashes[NETDEV_MAX_BURST];
+ULLONG_FOR_EACH_1(i, keys_map) {
+hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
+
>mask);
+}
+
+/* Lookup. */
+const struct cmap_node *nodes[NETDEV_MAX_BURST];
+uint32_t found_map =
+cmap_find_batch(>rules, keys_map, hashes,
nodes);
+/* Check results. When the i-th bit of found_map is set, it
means
+ * that a set of nodes with a matching hash value was found
for the
+ * i-th search-key. Due to possible hash collisions we need
to check
+ * which of the found rules, if any, really matches our
masked
+ * search-key. */
+ULLONG_FOR_EACH_1(i, found_map) {
+struct dpcls_rule *rule;
+
+CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
+if (OVS_LIKELY(dpcls_rule_matches_key(rule,
keys[i]))) {
+rules[i] = rule;
+/* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
+ * within one second optimization interval. */
+subtable->hit_cnt++;
+//lookups_match += subtable_pos;
+goto next;
+}
+}
+/* None of the found rules was a match. Reset the i-th
bit to
+ * keep searching this key in the next subtable. */
+ULLONG_SET0(found_map, i); /* Did not match. */
+next:
+; /* Keep Sparse happy. */
+}
+
+return found_map;
+}
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 143f1c288..bc0339a25 100644
---
