Re: [ovs-dev] [PATCH] ofproto: remove redundant ofproto class lookup.

2018-02-23 Thread Ben Pfaff
On Fri, Feb 23, 2018 at 01:15:38PM -0600, Mark Michelson wrote:
> The ofport_is_internal_or_patch() function called
> ofproto_port_open_type() twice. This resulted in the ofproto_class being
> looked up twice, when we only needed to look it up once.
> 
> This patch alters ofport_is_internal_or_patch() to look up the
> ofproto_class in-line, and then use the looked up class's port_open_type
> callback twice.
> 
> Signed-off-by: Mark Michelson 
> Tested-by: Aliasgar Ginwala 
> ---
> This patch arises as a result of testing done by Ali Ginwala and Han
> Zhou. Their test showed that commit 2d4beba resulted in slower
> performance of ovs-vswitchd than was seen in previous versions of OVS.
> 
> With this patch, Ali retested and reported that this patch had nearly
> the same effect on performance as reverting 2d4beba.
> 
> The test was to create 1 logical switch ports using 20 farm nodes,
> each running 50 HV sandboxes. "Base" in the tests below is OVS master
> with Han Zhou's ovn-controller incremental processing patch applied.
> 
> base: Test completed in 8 hours
> base with 2d4beba reverted: Test completed in 5 hours 28 minutes
> base with this patch: Test completed in 5 hours 30 minutes

Thanks for finding and fixing the performance problem.

Taking a look at what ofproto_port_open_type() is doing, it's badly
written for the job it has to do in context.  Its caller has a
particular ofproto, from which one can find the ofproto_class and
therefore the port_open_type function by just following two pointers.
But it was actually throwing away that information and going to some
trouble to find it again via ofproto_class_find__(), which is super slow
and not really meant for inner loops.

We can solve the problem better by changing ofproto_port_open_type()'s
parameter from a datapath type name to an ofproto and then passing that
in.  It requires a little effort in a couple of places in the tree to
find the ofproto, but it's not exactly difficult.

So I think that we can do even better than this patch and probably
obtain an additional performance boost.  I posted a patch that does
that:
https://patchwork.ozlabs.org/patch/877318/

What do you think?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH] ofproto: remove redundant ofproto class lookup.

2018-02-23 Thread Mark Michelson
The ofport_is_internal_or_patch() function called
ofproto_port_open_type() twice. This resulted in the ofproto_class being
looked up twice, when we only needed to look it up once.

This patch alters ofport_is_internal_or_patch() to look up the
ofproto_class in-line, and then use the looked up class's port_open_type
callback twice.

Signed-off-by: Mark Michelson 
Tested-by: Aliasgar Ginwala 
---
This patch arises as a result of testing done by Ali Ginwala and Han
Zhou. Their test showed that commit 2d4beba resulted in slower
performance of ovs-vswitchd than was seen in previous versions of OVS.

With this patch, Ali retested and reported that this patch had nearly
the same effect on performance as reverting 2d4beba.

The test was to create 1 logical switch ports using 20 farm nodes,
each running 50 HV sandboxes. "Base" in the tests below is OVS master
with Han Zhou's ovn-controller incremental processing patch applied.

base: Test completed in 8 hours
base with 2d4beba reverted: Test completed in 5 hours 28 minutes
base with this patch: Test completed in 5 hours 30 minutes
---
testing showed that commit 
 ofproto/ofproto.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f28bb896e..1cc2d34e6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2728,10 +2728,26 @@ init_ports(struct ofproto *p)
 static bool
 ofport_is_internal_or_patch(const struct ofproto *p, const struct ofport *port)
 {
-return !strcmp(netdev_get_type(port->netdev),
-   ofproto_port_open_type(p->type, "internal")) ||
-   !strcmp(netdev_get_type(port->netdev),
-   ofproto_port_open_type(p->type, "patch"));
+const struct ofproto_class *class;
+const char *datapath_type = ofproto_normalize_type(p->type);
+
+class = ofproto_class_find__(datapath_type);
+if (!class) {
+return true;
+}
+
+const char *internal_type;
+const char *patch_type;
+if (class->port_open_type) {
+internal_type = class->port_open_type(datapath_type, "internal");
+patch_type = class->port_open_type(datapath_type, "patch");
+} else {
+internal_type = "internal";
+patch_type = "patch";
+}
+
+return !strcmp(netdev_get_type(port->netdev), internal_type) ||
+   !strcmp(netdev_get_type(port->netdev), patch_type);
 }
 
 /* If 'port' is internal or patch and if the user didn't explicitly specify an
-- 
2.13.6

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev