[ovs-dev] [PATCH 6/6] vxlan: Create and delete tnl_backers in type_run()

Ben Pfaff blp at nicira.com
Thu Feb 14 20:20:14 UTC 2013


On Thu, Feb 14, 2013 at 08:19:29PM +0000, Kyle Mestery (kmestery) wrote:
> On Feb 14, 2013, at 12:49 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Thu, Feb 14, 2013 at 06:47:56PM +0000, Kyle Mestery (kmestery) wrote:
> >> On Feb 14, 2013, at 12:33 PM, Ben Pfaff <blp at nicira.com> wrote:
> >>> On Thu, Feb 14, 2013 at 09:37:30AM -0500, Kyle Mestery wrote:
> >>>> Garbage collect tnl_backers during type_run(). Add new
> >>>> tnl_backers if a VXLAN ports UDP port changes.
> >>>> 
> >>>> Signed-off-by: Kyle Mestery <kmestery at cisco.com>
> >>> 
> >>> The error handling here is bad.  If it fails to add or remove a port,
> >>> then ovs_assert() aborts the whole ovs-vswitchd process.
> >>> 
> >> I can change the asserts into something more sane.
> >> 
> >>> But I don't fully understand the code.  It looks to me like the first
> >>> time we encounter a particular dp_port_name in the inner HMAP_FOR_EACH,
> >>> we transfer that dp_port_name from tmp_simap to backer->tnl_backers.
> >>> Straightforward enough.  However, I believe that the second time we
> >>> encounter that same dp_port_name, we will not find it in tmp_simap
> >>> (because we deleted it) and will therefore try to add a new port for
> >>> it.   I believe that will fail, because there is already a port with
> >>> that dst_port, and then we'll assert-fail the process.
> >>> 
> >>> Am I reading the code correctly?
> >>> 
> >> Actually, the second time the same dp_port_name is encountered,
> >> we first look and see if we need to reconfigure it, and then we check if
> >> it's already in backer->tnl_backers, which it will be since we added it
> >> there the first time we found it. I don't see it adding the port twice, unless
> >> I'm misreading it.
> > 
> > Looking again, you're right, thanks.
> > 
> >>> I've now pushed patch 1-4 to master, waiting for an ack from Jesse on
> >>> #5, waiting for your reply on this one.
> >>> 
> >> Thanks Ben! Let me know if you want me to modify the error handling
> >> behavior.
> > 
> > Please do, I'm not sure of the correct behavior.
> 
> I sent a revised version with error handling in lieu of ovs_asserts() for
> port creation and deletion. For the netdev_get_tunnel_config() assert,
> Ethan indicated that should never happen, so I'd rather leave that as an
> ovs_assert().

Sure, I'm happy with that.

Ethan told me out-of-band he wants to look this over, so we can await
his review.

Thanks a lot.

Ben.



More information about the dev mailing list