Hi : The results are super awesome! Performance have drastically improved by ~60%. The test for 10k lports, 40 LSs and 8 LRs and 1k HVs just got completed in 3 hours 39 min vs 8+ hours for branch-2.9. Cpu utilization graph of a farm comparing Ben's ofproto patch vs branch-2.9 is available @ https://raw.githubusercontent.com/noah8713/ovn-scale-test/scale_results/results/ovs_2.9_vs_ben_ofproto.png
Thanks a ton Ben for the new patch. I give it a +1 for merge untill someone is suggesting any other improvements/comments. On Fri, Feb 23, 2018 at 3:32 PM, aginwala <aginw...@asu.edu> wrote: > This is great! I will re-run the test with this patch and send over the > results soon! > > On Fri, Feb 23, 2018 at 3:25 PM, Han Zhou <zhou...@gmail.com> wrote: > >> On Fri, Feb 23, 2018 at 2:03 PM, Ben Pfaff <b...@ovn.org> wrote: >> > >> > ofproto_port_open_type() was surprisingly slow because it called the >> > function ofproto_class_find__(), which itself was surprisingly slow >> because >> > it actually creates a set of strings and enumerates all of the available >> > classes. >> > >> > This patch improves performance by eliminating the call to >> > ofproto_class_find__() from ofproto_port_open_type(). In turn that >> > required changing a parameter type and updating all the callers. >> > >> > Possibly it would be worth making ofproto_class_find__() itself faster, >> > but it doesn't look like any of its other callers would be used in inner >> > loops. >> > >> > A different patch that was also meant to speed this up reported the >> > following performance improvements. That patch just eliminated half of >> the >> > calls to ofproto_class_find__() in inner loops, whereas this one >> eliminates >> > all of them and should therefore perform even better. >> > >> > 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 >> > >> > Reported-by: Mark Michelson <mmich...@redhat.com> >> > Signed-off-by: Ben Pfaff <b...@ovn.org> >> > --- >> > ofproto/in-band.c | 2 +- >> > ofproto/ofproto.c | 30 ++++++++++-------------------- >> > ofproto/ofproto.h | 2 +- >> > vswitchd/bridge.c | 12 ++++-------- >> > 4 files changed, 16 insertions(+), 30 deletions(-) >> > >> > diff --git a/ofproto/in-band.c b/ofproto/in-band.c >> > index 849b1cedaff0..82d8dfa14774 100644 >> > --- a/ofproto/in-band.c >> > +++ b/ofproto/in-band.c >> > @@ -428,7 +428,7 @@ in_band_create(struct ofproto *ofproto, const char >> *local_name, >> > struct in_band *in_band; >> > struct netdev *local_netdev; >> > int error; >> > - const char *type = ofproto_port_open_type(ofproto->type, >> "internal"); >> > + const char *type = ofproto_port_open_type(ofproto, "internal"); >> > >> > *in_bandp = NULL; >> > error = netdev_open(local_name, type, &local_netdev); >> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> > index f28bb896eee9..a982de9d8db4 100644 >> > --- a/ofproto/ofproto.c >> > +++ b/ofproto/ofproto.c >> > @@ -1966,27 +1966,18 @@ ofproto_port_dump_done(struct ofproto_port_dump >> *dump) >> > return dump->error == EOF ? 0 : dump->error; >> > } >> > >> > -/* Returns the type to pass to netdev_open() when a datapath of type >> > - * 'datapath_type' has a port of type 'port_type', for a few special >> > - * cases when a netdev type differs from a port type. For example, >> when >> > - * using the userspace datapath, a port of type "internal" needs to be >> > - * opened as "tap". >> > +/* Returns the type to pass to netdev_open() when 'ofproto' has a port >> of type >> > + * 'port_type', for a few special cases when a netdev type differs from >> a port >> > + * type. For example, when using the userspace datapath, a port of >> type >> > + * "internal" needs to be opened as "tap". >> > * >> > * Returns either 'type' itself or a string literal, which must not be >> > * freed. */ >> > const char * >> > -ofproto_port_open_type(const char *datapath_type, const char >> *port_type) >> > +ofproto_port_open_type(const struct ofproto *ofproto, const char >> *port_type) >> > { >> > - const struct ofproto_class *class; >> > - >> > - datapath_type = ofproto_normalize_type(datapath_type); >> > - class = ofproto_class_find__(datapath_type); >> > - if (!class) { >> > - return port_type; >> > - } >> > - >> > - return (class->port_open_type >> > - ? class->port_open_type(datapath_type, port_type) >> > + return (ofproto->ofproto_class->port_open_type >> > + ? ofproto->ofproto_class->port_open_type(ofproto->type, >> port_type) >> > : port_type); >> > } >> > >> > @@ -2728,10 +2719,9 @@ 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 char *netdev_type = netdev_get_type(port->netdev); >> > + return !strcmp(netdev_type, ofproto_port_open_type(p, "internal")) >> || >> > + !strcmp(netdev_type, ofproto_port_open_type(p, "patch")); >> > } >> > >> > /* If 'port' is internal or patch and if the user didn't explicitly >> specify an >> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h >> > index 1e48e1952aa2..8c85bbf7fb26 100644 >> > --- a/ofproto/ofproto.h >> > +++ b/ofproto/ofproto.h >> > @@ -290,7 +290,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump >> *); >> > #define OFPROTO_FLOW_LIMIT_DEFAULT 200000 >> > #define OFPROTO_MAX_IDLE_DEFAULT 10000 /* ms */ >> > >> > -const char *ofproto_port_open_type(const char *datapath_type, >> > +const char *ofproto_port_open_type(const struct ofproto *, >> > const char *port_type); >> > int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t >> *ofp_portp); >> > int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port); >> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> > index cf9c79fd77cf..f03fb567f0be 100644 >> > --- a/vswitchd/bridge.c >> > +++ b/vswitchd/bridge.c >> > @@ -88,7 +88,6 @@ struct iface { >> > >> > /* These members are valid only within bridge_reconfigure(). */ >> > const char *type; /* Usually same as cfg->type. */ >> > - const char *netdev_type; /* type that should be used for >> netdev_open. */ >> > const struct ovsrec_interface *cfg; >> > }; >> > >> > @@ -822,7 +821,9 @@ bridge_delete_or_reconfigure_ports(struct bridge >> *br) >> > goto delete; >> > } >> > >> > - if (strcmp(ofproto_port.type, iface->netdev_type) >> > + const char *netdev_type = ofproto_port_open_type(br->ofproto, >> > + iface->type); >> > + if (strcmp(ofproto_port.type, netdev_type) >> > || netdev_set_config(iface->netdev, &iface->cfg->options, >> NULL)) { >> > /* The interface is the wrong type or can't be configured. >> > * Delete it. */ >> > @@ -1778,7 +1779,7 @@ iface_do_create(const struct bridge *br, >> > goto error; >> > } >> > >> > - type = ofproto_port_open_type(br->cfg->datapath_type, >> > + type = ofproto_port_open_type(br->ofproto, >> > iface_get_type(iface_cfg, br->cfg)); >> > error = netdev_open(iface_cfg->name, type, &netdev); >> > if (error) { >> > @@ -1856,8 +1857,6 @@ iface_create(struct bridge *br, const struct >> ovsrec_interface *iface_cfg, >> > iface->ofp_port = ofp_port; >> > iface->netdev = netdev; >> > iface->type = iface_get_type(iface_cfg, br->cfg); >> > - iface->netdev_type = ofproto_port_open_type(br->cfg->datapath_type, >> > - iface->type); >> > iface->cfg = iface_cfg; >> > hmap_insert(&br->ifaces, &iface->ofp_port_node, >> > hash_ofp_port(ofp_port)); >> > @@ -3445,13 +3444,10 @@ bridge_del_ports(struct bridge *br, const struct >> shash *wanted_ports) >> > const struct ovsrec_interface *cfg = >> port_rec->interfaces[i]; >> > struct iface *iface = iface_lookup(br, cfg->name); >> > const char *type = iface_get_type(cfg, br->cfg); >> > - const char *dp_type = br->cfg->datapath_type; >> > - const char *netdev_type = ofproto_port_open_type(dp_type, >> type); >> > >> > if (iface) { >> > iface->cfg = cfg; >> > iface->type = type; >> > - iface->netdev_type = netdev_type; >> > } else if (!strcmp(type, "null")) { >> > VLOG_WARN_ONCE("%s: The null interface type is >> deprecated and" >> > " may be removed in February 2013. >> Please >> email" >> > -- >> > 2.16.1 >> > >> > _______________________________________________ >> > dev mailing list >> > d...@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> I was debugging why ofproto_class_find__() was so slow in the scale >> testing >> env, but not obvious slow down in a standalone setup when I just do repeat >> port creation/deletion. It seems to be caused by acquiring the dpif_mutex >> in dp_enumerate_types, but not sure what causes contention of that lock, >> and why it is not a problem in the standalone environment. >> >> This patch makes a lot more sense to me. And it should be even better than >> reverting the patch "ofproto: Include patch ports in mtu overriden check", >> because there was one call to ofproto_port_open_type() before that patch, >> and with this patch there is none. >> >> I suggest to update the commit message with new numbers from Ali's test. >> Also, I suggest to include the original discussion in commit message for >> more context to understand the impact: >> >> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-February/046140.html >> >> Thanks, >> Han >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev