[ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
aginwala
aginwala at asu.edu
Mon Feb 26 20:15:33 UTC 2018
Sweet!
On Mon, Feb 26, 2018 at 11:42 AM, Ben Pfaff <blp at ovn.org> wrote:
> Thanks a lot for the testing! I added your test results to the commit
> message and applied this to master.
>
> On Sat, Feb 24, 2018 at 04:07:23PM +0000, aginwala wrote:
> > 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 <aginwala at 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 <zhouhan at gmail.com> wrote:
> > >
> > >> On Fri, Feb 23, 2018 at 2:03 PM, Ben Pfaff <blp at 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 <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"
> > >> > --
> > >> > 2.16.1
> > >> >
> > >> > _______________________________________________
> > >> > dev mailing list
> > >> > dev at 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
> > >> dev at openvswitch.org
> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >>
> > >
> > >
>
More information about the dev
mailing list