Date: Thu, 25 Jul 2013 18:42:53 +0200 From: Marko Zec <zec@fer.hr> To: Marius Strobl <marius@alchemy.franken.de> Cc: Craig Rodrigues <rodrigc@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r253346 - in head/sys: kern net netgraph netgraph/bluetooth/socket Message-ID: <201307251842.53577.zec@fer.hr> In-Reply-To: <20130725142743.GS56034@alchemy.franken.de> References: <201307150132.r6F1WttU081255@svn.freebsd.org> <201307251224.53211.zec@fer.hr> <20130725142743.GS56034@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 25 July 2013 16:27:43 Marius Strobl wrote: > On Thu, Jul 25, 2013 at 12:24:53PM +0200, Marko Zec wrote: > > On Thursday 25 July 2013 11:36:46 Craig Rodrigues wrote: > > > On Thu, Jul 25, 2013 at 1:07 AM, Marius Strobl > > > > <marius@alchemy.franken.de>wrote: > > > > Uhm - do we really need to have that layering violation in > > > > subr_bus.c? Wouldn't it be sufficient to set curthread->td_vnet to > > > > vnet0 in if_alloc(9) or if_attach(9) at least instead? > > > > > > There was some discussion about this involving Marko Zec, Adrian > > > Chadd, and myself > > > starting in this thread: > > > > > > http://lists.freebsd.org/pipermail/svn-src-all/2013-July/071798.html > > > > > > Adrian and Marko converged on similar patches: > > > > > > http://lists.freebsd.org/pipermail/freebsd-hackers/2012-November/0411 > > >20.h tml > > > http://people.freebsd.org/~adrian/ath/20130712-vimage-default-attach- > > >deta ch.diff > > > > > > > > > As Marko mentioned in another e-mail on this thread, the patch as it > > > is necessary in > > > order to fix VIMAGE related kernel panics in many different > > > scenarios, including > > > ones involving Netgraph nodes. > > > > Moreover, unconditionally setting curvnet to vnet0 in if_alloc(), > > if_attach() or similar places as suggested my Marius simply couldn't > > work, because that would break creation of pseudo-interfaces inside > > non-vnet0 contexts (such as vlan, ng_ether, ng_eiface etc.). > > Well, I didn't say that it shall be unconditional; in a previous > version of the patch Adrian also set it conditionally only in case > vnet isn't vnet0 in device_probe_and_attach() so it seems viable to > check whether that's needed in a particular context. When a function which is expected to be called with curvnet properly set is called with curvnet being NULL, it should panic (or crash), not guess which vnet the caller should have set, but failed to do. Otherwise, we would have tons of subtle inter-vnet leaks (towards vnet0) which could be very difficult to track and nail down. Hell, why not set curvnet to vnet0 at boot time and NEVER revert it back to NULL, while only occasionally switching to non-default vnets - how would we swim in that kind of mud? > As for Netgraph nodes I don't know how these are attached to devices > but a quick look at the code suggests that f. e. ng_make_node_common() > would be a good candidate for setting vnet to vnet0 if necessary. > Moving this network specific stuff out of the generic device layer > would also make things consistent and symmetric given that r253346 > added setting vnet to if_detach(), if_free() and ng_unref_node(). In all the functions you're refering to here, vnet context is unambiguously defined by the object passed as function's argument, so it is perfectly safe to do CURVNET_SET(ifp->if_vnet) or CURVNET_SET(ng_node->nd_vnet), which is very different from the proposal for pure guessing in if_attach() etc. which I am strongly opposed to. Marko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201307251842.53577.zec>