On Thu, Jan 25, 2018 at 09:22:56AM -0500, Eric Garver wrote: > On Wed, Jan 24, 2018 at 12:48:38PM -0800, Ben Pfaff wrote: > > 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, ju...@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 > > short answer: I don't think there is any major concern about the > lingering tunnel interface. > > Longer answer follows: > > The rtnetlink create will attempt to reuse a tunnel interface if it > already exists. If that interface has the wrong parameters we attempt to > delete and recreate it. So the lingering kernel interface is a non-issue > for OVS. However, OVS shouldn't leave the interface hanging around if > it's not using it anymore (this is what this patch fixes). > > Tunnels created via rtnetlink are attached to the openvswitch kernel > module by NETDEV vports. When the backer is closed those NETDEV vports > are cleaned up by the kernel module. As such, they are disconnected from > OVS so we won't get any wild packets appearing. > > > to apply this patch without that information, but having it would allow > > me to better understand the importance of the fix. > > Thanks Ben.
Thanks for the extra info. I applied this to master. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev