By bumb luck, I noticed that 'ofproto->backer->tnl_count' will affect
how ovs_native_tunneling_is_on() gives its answer. The following
incremental diff seems to help 'tunnel_push_pop" test to pass. BTW, I
am not suggesting this is the right fix, since it looks ugly.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 431f739..44b5749 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1670,13 +1670,14 @@ port_construct(struct ofport *port_)
port->odp_port = dpif_port.port_no;
if (netdev_get_tunnel_config(netdev)) {
+ atomic_count_inc(&ofproto->backer->tnl_count);
error = tnl_port_add(port, port->up.netdev, port->odp_port,
ovs_native_tunneling_is_on(ofproto), namebuf);
if (error) {
+ atomic_count_dec(&ofproto->backer->tnl_count);
return error;
}
- atomic_count_inc(&ofproto->backer->tnl_count);
port->is_tunnel = true;
if (ofproto->ipfix) {
dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_, port->odp_port);
In addition, there may be a memory leak in case we bail out early.
proto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 44b5749..bd45305 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1675,6 +1675,7 @@ port_construct(struct ofport *port_)
ovs_native_tunneling_is_on(ofproto), namebuf);
if (error) {
atomic_count_dec(&ofproto->backer->tnl_count);
+ dpif_port_destroy(&dpif_port);
return error;
}
The unit tests are still happy with both patches applied.
On Wed, May 27, 2015 at 9:36 AM, Ben Pfaff <b...@nicira.com> wrote:
> Oh, unfortunately it breaks test 629 "tunnel_push_pop - action". I
> haven't had a chance to fully investigate yet.
>
> On Wed, May 27, 2015 at 09:33:07AM -0700, Ben Pfaff wrote:
>> Wow, thanks!
>>
>> I added a Tested-by for you also, and I'll apply this to master in a
>> minute.
>>
>> On Wed, May 27, 2015 at 09:17:23AM -0700, Alex Wang wrote:
>> > Tested,
>> >
>> >
>> > Acked-by: Alex Wang <al...@nicira.com>
>> >
>> > On Tue, May 26, 2015 at 6:48 PM, Ben Pfaff <b...@nicira.com> wrote:
>> >
>> > > Until now, when two tunnels had an identical configuration, both of them
>> > > were assigned OpenFlow ports, but only one of those OpenFlow ports was
>> > > functional. With this commit, only one of the two (or more) identically
>> > > configured tunnels will be assigned an OpenFlow port number.
>> > >
>> > > Reported-by: Keith Holleman <hollemani...@gmail.com>
>> > > Signed-off-by: Ben Pfaff <b...@nicira.com>
>> > > ---
>> > > AUTHORS | 1 +
>> > > ofproto/ofproto-dpif.c | 8 ++++++--
>> > > ofproto/tunnel.c | 14 ++++++++++----
>> > > ofproto/tunnel.h | 6 +++---
>> > > 4 files changed, 20 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/AUTHORS b/AUTHORS
>> > > index 5178b43..60b8cfa 100644
>> > > --- a/AUTHORS
>> > > +++ b/AUTHORS
>> > > @@ -276,6 +276,7 @@ Joan Cirer j...@ev0.net
>> > > John Darrington j...@darrington.wattle.id.au
>> > > John Galgay j...@galgay.net
>> > > John Hurley john.hur...@netronome.com
>> > > +Keith Holleman hollemani...@gmail.com
>> > > K 華 k940...@hotmail.com
>> > > Kevin Mancuso kevin.manc...@rackspace.com
>> > > Kiran Shanbhog ki...@vmware.com
>> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> > > index d151bb7..532fc4a 100644
>> > > --- a/ofproto/ofproto-dpif.c
>> > > +++ b/ofproto/ofproto-dpif.c
>> > > @@ -1686,9 +1686,13 @@ port_construct(struct ofport *port_)
>> > > port->odp_port = dpif_port.port_no;
>> > >
>> > > if (netdev_get_tunnel_config(netdev)) {
>> > > + error = tnl_port_add(port, port->up.netdev, port->odp_port,
>> > > + ovs_native_tunneling_is_on(ofproto),
>> > > namebuf);
>> > > + if (error) {
>> > > + return error;
>> > > + }
>> > > +
>> > > atomic_count_inc(&ofproto->backer->tnl_count);
>> > > - tnl_port_add(port, port->up.netdev, port->odp_port,
>> > > - ovs_native_tunneling_is_on(ofproto), namebuf);
>> > > port->is_tunnel = true;
>> > > if (ofproto->ipfix) {
>> > > dpif_ipfix_add_tunnel_port(ofproto->ipfix, port_,
>> > > port->odp_port);
>> > > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
>> > > index 3ea0eb4..d2ac7c6 100644
>> > > --- a/ofproto/tunnel.c
>> > > +++ b/ofproto/tunnel.c
>> > > @@ -1,4 +1,4 @@
>> > > -/* Copyright (c) 2013, 2014 Nicira, Inc.
>> > > +/* Copyright (c) 2013, 2014, 2015 Nicira, Inc.
>> > > *
>> > > * Licensed under the Apache License, Version 2.0 (the "License");
>> > > * you may not use this file except in compliance with the License.
>> > > @@ -203,14 +203,20 @@ tnl_port_add__(const struct ofport_dpif *ofport,
>> > > const struct netdev *netdev,
>> > >
>> > > /* Adds 'ofport' to the module with datapath port number 'odp_port'.
>> > > 'ofport's
>> > > * must be added before they can be used by the module. 'ofport' must
>> > > be a
>> > > - * tunnel. */
>> > > -void
>> > > + * tunnel.
>> > > + *
>> > > + * Returns 0 if successful, otherwise a positive errno value. */
>> > > +int
>> > > tnl_port_add(const struct ofport_dpif *ofport, const struct netdev
>> > > *netdev,
>> > > odp_port_t odp_port, bool native_tnl, const char name[])
>> > > OVS_EXCLUDED(rwlock)
>> > > {
>> > > + bool ok;
>> > > +
>> > > fat_rwlock_wrlock(&rwlock);
>> > > - tnl_port_add__(ofport, netdev, odp_port, true, native_tnl, name);
>> > > + ok = tnl_port_add__(ofport, netdev, odp_port, true, native_tnl,
>> > > name);
>> > > fat_rwlock_unlock(&rwlock);
>> > > +
>> > > + return ok ? 0 : EEXIST;
>> > > }
>> > >
>> > > /* Checks if the tunnel represented by 'ofport' reconfiguration due to
>> > > changes
>> > > diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
>> > > index 6181762..b8415ab 100644
>> > > --- a/ofproto/tunnel.h
>> > > +++ b/ofproto/tunnel.h
>> > > @@ -1,4 +1,4 @@
>> > > -/* Copyright (c) 2013 Nicira, Inc.
>> > > +/* Copyright (c) 2013, 2015 Nicira, Inc.
>> > > *
>> > > * Licensed under the Apache License, Version 2.0 (the "License");
>> > > * you may not use this file except in compliance with the License.
>> > > @@ -33,8 +33,8 @@ void ofproto_tunnel_init(void);
>> > > bool tnl_port_reconfigure(const struct ofport_dpif *, const struct
>> > > netdev
>> > > *,
>> > > odp_port_t, bool native_tnl, const char
>> > > name[]);
>> > >
>> > > -void tnl_port_add(const struct ofport_dpif *, const struct netdev *,
>> > > - odp_port_t odp_port, bool native_tnl, const char
>> > > name[]);
>> > > +int tnl_port_add(const struct ofport_dpif *, const struct netdev *,
>> > > + odp_port_t odp_port, bool native_tnl, const char
>> > > name[]);
>> > > void tnl_port_del(const struct ofport_dpif *);
>> > >
>> > > const struct ofport_dpif *tnl_port_receive(const struct flow *);
>> > > --
>> > > 2.1.3
>> > >
>> > > _______________________________________________
>> > > dev mailing list
>> > > dev@openvswitch.org
>> > > http://openvswitch.org/mailman/listinfo/dev
>> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
