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

Reply via email to