[ovs-dev] [PATCH] ofproto-dpif: Delete system tunnel interface when remove ovs bridge

Ben Pfaff blp at ovn.org
Wed Jan 24 20:48:38 UTC 2018


On Wed, Jan 24, 2018 at 09:31:32AM -0500, Eric Garver wrote:
> On Tue, Jan 23, 2018 at 07:46:47PM -0800, Ben Pfaff wrote:
> > On Thu, Oct 26, 2017 at 10:24:46AM -0400, Eric Garver wrote:
> > > On Wed, Oct 25, 2017 at 11:41:27AM +0800, juyan at redhat.com wrote:
> > > > When there is only one bridge,create tunnel in the bridge,
> > > > then delete the bridge directly. the system tunnel interface
> > > > still in.
> > > > 
> > > > Cause of only one bridge, backer->refcount values 1, when
> > > > delete bridge, "close_dpif_backer" will delete the backer,
> > > > so type_run will return directly, doesn't delete the interface.
> > > > This patch delete the system interface before free the backer.
> > > 
> > > I'll add a bit more explanation..
> > > 
> > > This occurs when a tunnel is created with rtnetlink. With the compat API
> > > the tunnel is created via the vport tunnel interface, so it can be
> > > implicitly cleaned up by the kernel when the dp is closed or the module
> > > unloaded.  But with rtnetlink the kernel module is not involved with the
> > > tunnel device creation (it's added to OVS as a netdev vport), so
> > > userspace needs to explicitly clean up the tunnel backers - type_run
> > > can't garbage collect them if the dpif is already deleted.
> > 
> > I guess this has been due to be applied for a long time!  I am sorry
> > that I missed it.
> > 
> > The code looks OK but I don't yet understand the consequences.  What
> > problem does this patch solve?
> > 
> 
> Bugzilla reference:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1505776
> 
> IIRC, there is a race between close_dpif_backer() and tnl_backers
> garbage collection which is done in the "if (backer->need_revalidate)"
> block of type_run(). Non-tunnel port types are immediately cleaned up by
> port_del(), but tunnel ports are only cleaned up during dpif
> revalidation. My theory is that we never noticed it in the compat
> interface because the openvswitch kernel module implicitly cleans up all
> the interfaces when the dpif is closed. In the rtnetlink case, the
> tunnel interfaces are created by OVS process not the openvswitch kernel
> module, so it doesn't know about them.
> 
> I think this may also be closing a tunnel port memory leak that occurs
> for both compat and rtnetlink when deleting the dpif backer. This occurs
> since the tunnel ports weren't garbage collected before the dpif backer
> disappeared.
> 
> Sorry if my explanation isn't that good. I'm not all that familiar with
> this code.

OK, thanks.

Is the following a fair description for the commit message?

        When a user adds the first tunnel of a given type (e.g. the
        first VXLAN tunnel) to an OVS bridge, OVS adds a vport of the
        same type to the kernel datapath that backs the bridge.  There
        is the corresponding expectation that, when the last tunnel of
        that type is removed from the OVS bridges, OVS would remove the
        vport that represents it from the backing kernel datapath, but
        OVS was not doing that.  This commit fixes the problem.

What isn't clear to me is the higher-level consequence of failing to
delete the kernel datapath vport.  The bugzilla report doesn't say and I
don't see anything else that does.  Can anyone help me out?  I'm willing
to apply this patch without that information, but having it would allow
me to better understand the importance of the fix.

Thanks,

Ben.


More information about the dev mailing list