Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
Sweet! On Mon, Feb 26, 2018 at 11:42 AM, Ben Pfaff 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 +, 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 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 wrote: > > > > > >> On Fri, Feb 23, 2018 at 2: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 1 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 > > >> > Signed-off-by: Ben Pfaff > > >> > --- > > >> > 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, whe
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
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
>
> 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 1 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
> >Signed-off-by: Ben Pfaff
> >---
> > 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 =
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
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 +, 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 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 wrote: > > > >> On Fri, Feb 23, 2018 at 2: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 1 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 > >> > Signed-off-by: Ben Pfaff > >> > --- > >> > 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 > >> *p
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
On Fri, Feb 23, 2018 at 03:25:40PM -0800, Han Zhou wrote:
> On Fri, Feb 23, 2018 at 2: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 1 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
> > Signed-off-by: Ben Pfaff
> > ---
> > 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_typ
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
Yes this is much better than the patch I submitted. Thank you very much Ben.
Acked-by: Mark Michelson
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 1 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
Signed-off-by: Ben Pfaff
---
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 ofpr
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
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 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 wrote:
>
>> On Fri, Feb 23, 2018 at 2: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 1 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
>> > Signed-off-by: Ben Pfaff
>> > ---
>> > 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
>>
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
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 wrote:
> On Fri, Feb 23, 2018 at 2: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 1 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
> > Signed-off-by: Ben Pfaff
> > ---
> > 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 = n
Re: [ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
On Fri, Feb 23, 2018 at 2: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 1 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
> Signed-off-by: Ben Pfaff
> ---
> 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/ofpro
[ovs-dev] [PATCH] ofproto: Make ofproto_port_open_type() faster.
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 1 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
Signed-off-by: Ben Pfaff
---
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 20
#define OFPROTO_MAX_IDLE_DEFAULT 1 /* ms */
-const char *ofproto_port_open_type(const char *da
