From owner-svn-src-all@FreeBSD.ORG Fri Oct 18 10:55:57 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 561BD559; Fri, 18 Oct 2013 10:55:57 +0000 (UTC) (envelope-from zec@fer.hr) Received: from mail.fer.hr (mail.fer.hr [161.53.72.233]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id AF2F6254B; Fri, 18 Oct 2013 10:55:56 +0000 (UTC) Received: from dyn10.nxlab.fer.hr (161.53.63.210) by MAIL.fer.hr (161.53.72.233) with Microsoft SMTP Server (TLS) id 14.2.342.3; Fri, 18 Oct 2013 12:55:54 +0200 From: Marko Zec To: Gleb Smirnoff Subject: Re: svn commit: r228969 - head/sys/netinet Date: Fri, 18 Oct 2013 12:56:11 +0200 User-Agent: KMail/1.9.10 References: <201112292041.pBTKfGkj071711@svn.freebsd.org> <201310180007.36048.zec@fer.hr> <20131018100915.GH52889@FreeBSD.org> In-Reply-To: <20131018100915.GH52889@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-ID: <201310181256.12135.zec@fer.hr> X-Originating-IP: [161.53.63.210] Cc: src-committers@freebsd.org, John Baldwin , svn-src-all@freebsd.org, hrs@freebsd.org, Mikolaj Golub , Bjoern Zeeb , svn-src-head@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Oct 2013 10:55:57 -0000 On Friday 18 October 2013 12:09:16 Gleb Smirnoff wrote: > 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? No, it is not. 1) you're not refcounting tasks in struct vnet, so when a vnet is freed you can't be sure (in a generic way) that there are no pending tasks referencig that vnet; 2) taskqueue is a general-purpose facility, not network-stack specialized, so you can't afford a luxury to keep non-refcounted references to vnets in struct task, since there's no guarantee a task will go away before the referenced vnet is destroyed. In your particular example you're probably doing the right thing in pf cleanup code, but then the reference to a vnet should be kept in a pf-related object, not struct task. > 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. Just as you described here, it is the pf(4) code which gets notified on vnet teardowns, not some generic taskqueue subroutine, so the right place to hold a vnet pointer is in a pf-related struct, not struct task. I'm also surprised noone else chimed in to object to this obviously unnecessary layering violation, since up to this point there were no inherent dependencies between taskqueues and vnets, and now you are introducing one for no good reason. Again, please remove vnet references from struct task. Thanks, Marko > 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.