Date: Fri, 18 Oct 2013 14:09:16 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Marko Zec <zec@fer.hr> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, hrs@freebsd.org, Mikolaj Golub <trociny@freebsd.org>, Bjoern Zeeb <bz@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r228969 - head/sys/netinet Message-ID: <20131018100915.GH52889@FreeBSD.org> In-Reply-To: <201310180007.36048.zec@fer.hr> References: <201112292041.pBTKfGkj071711@svn.freebsd.org> <201310161709.04489.jhb@freebsd.org> <20131017193545.GA3683@gmail.com> <201310180007.36048.zec@fer.hr>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Oct 18, 2013 at 12:07:35AM +0200, Marko Zec wrote: M> > Concerning this more general solution from Gleb, with storing the vnet M> > pointer in the task, I wander how it is safe when the vnet is being M> > removed? M> > M> > In vnet_destroy() the vnet interfaces are moved back to their parent M> > vnets, so setting the vnet context from the interface looks safer. M> M> +1 M> M> Gleb, M> M> what exactly was the justification for embedding a vnet context inside M> struct task, when it could have been easily embedded, directly or M> indirectly, inside the object pointed to by ta_context? M> M> What is / was your plan to garbage collect such stale pointers? M> M> At least in the early days of VIMAGE / VNET hacking, all the people involved M> strived very hard to store backpointers to vnets in as few structs as M> possible, and for quite a while we managed to constrain this to only ifnets M> and sockets, and both of those references are refcounted in struct vnet. And my change is in the same direction, isn't it? We will store vnet pointer only in the struct task, not in all structs that ta_context may point at. M> Now, more (unreferenced) backpointers to struct vnet seem to be popping up M> here and there, but in most cases they are embedded in objects which are M> guaranteed to persist only as long as the referenced vnet is alive. One M> notable exception to this rule is struct tcpcb which apparently can outlive M> vnet teardowns under certain conditions, but this is a known issue which M> needs to be fixed, not a precedent which calls for more timebombs to be M> scattered around the source tree. M> M> So pls. reconsider solving the issue at hand using a different approach, and M> back out r256587. I don't see problem here. When a vnet is destroyed, every facility is notified to tear down its vnet related data. In pf(4) that would mean, that the task is freed, and would never be put again on taskqueue. So I don't see any leaks or dereferences of dead vnet here. I have just looked into multicast code and see that it takes another approach. The task is single and global, and it sets vnet context per-option. Well, my patch won't hurt its operation. Just orthogonal approach. -- Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131018100915.GH52889>