Message ID | 20180223191538.18399-1-mmichels@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] ofproto: remove redundant ofproto class lookup. | expand |
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 <mmichels@redhat.com> > Tested-by: Aliasgar Ginwala <aginwala@ebay.com> > --- > 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 10000 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.
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