Thanks for the review, I applied this to master.
On Mon, Feb 26, 2018 at 09:22:59AM -0600, Mark Michelson wrote: > Yes this is much better than the patch I submitted. Thank you very much Ben. > > Acked-by: Mark Michelson <mmich...@redhat.com> > > On 02/23/2018 04:03 PM, Ben Pfaff 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" > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev