Wow, thanks for figuring that out!
I think that these changes are OK.
I'll fold them into my patch and repost.
On Thu, Jun 04, 2015 at 02:13:51AM -0700, Andy Zhou wrote:
> 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 <[email protected]> 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 <[email protected]>
> >> >
> >> > On Tue, May 26, 2015 at 6:48 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
> >> > > Signed-off-by: Ben Pfaff <[email protected]>
> >> > > ---
> >> > > 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 [email protected]
> >> > > John Darrington [email protected]
> >> > > John Galgay [email protected]
> >> > > John Hurley [email protected]
> >> > > +Keith Holleman [email protected]
> >> > > K 華 [email protected]
> >> > > Kevin Mancuso [email protected]
> >> > > Kiran Shanbhog [email protected]
> >> > > 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
> >> > > [email protected]
> >> > > http://openvswitch.org/mailman/listinfo/dev
> >> > >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
