[ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
Mark Michelson
mmichels at redhat.com
Mon Feb 26 15:22:59 UTC 2018
Yes this is much better than the patch I submitted. Thank you very much Ben.
Acked-by: Mark Michelson <mmichels at 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 <mmichels at redhat.com>
> Signed-off-by: Ben Pfaff <blp at 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"
>
More information about the dev
mailing list