Date: Thu, 15 Aug 2013 00:25:34 +0200 From: Marko Zec <zec@fer.hr> To: Mikolaj Golub <trociny@freebsd.org> Cc: freebsd-virtualization@freebsd.org Subject: Re: RFC: ipfw nat VIMAGE improvements Message-ID: <201308150025.34517.zec@fer.hr> In-Reply-To: <20130814204303.GA13541@gmail.com> References: <20130811200111.GA49895@gmail.com> <201308141728.31361.zec@fer.hr> <20130814204303.GA13541@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 14 August 2013 22:43:05 Mikolaj Golub wrote:
> On Wed, Aug 14, 2013 at 05:28:31PM +0200, Marko Zec wrote:
> > On Sunday 11 August 2013 22:01:12 Mikolaj Golub wrote:
> > > Hi,
> > >
> > > I would like to commit this patch that fixes some issues related to
> > > ipfw nat module load/unload on VIMAGE featured system.
> > >
> > > Any comments, objections?
> >
> > Far from being an expert in ipfw, I'm worried that the proposed
> > approach of simultaneously acquiring locks on _all_ ipfw instances
> > might be calling for trouble:
> >
> > + VNET_LIST_RLOCK();
> > + VNET_FOREACH(vnet_iter) {
> > + CURVNET_SET(vnet_iter);
> > + IPFW_WLOCK(&V_layer3_chain);
> > + CURVNET_RESTORE();
> > + }
> > ipfw_nat_ptr = ipfw_nat;
> > lookup_nat_ptr = lookup_nat;
> > ipfw_nat_cfg_ptr = ipfw_nat_cfg;
> > ipfw_nat_del_ptr = ipfw_nat_del;
> > ipfw_nat_get_cfg_ptr = ipfw_nat_get_cfg;
> > ipfw_nat_get_log_ptr = ipfw_nat_get_log;
> > - IPFW_WUNLOCK(&V_layer3_chain);
> > - V_ifaddr_event_tag = EVENTHANDLER_REGISTER(
> > + VNET_FOREACH(vnet_iter) {
> > + CURVNET_SET(vnet_iter);
> > + IPFW_WUNLOCK(&V_layer3_chain);
> > + CURVNET_RESTORE();
> > + }
> > + VNET_LIST_RUNLOCK();
> >
> > Why couldn't we introduce a per-vnet flag, say V_ipfw_nat_ready, and
> > use it as
> >
> > #define IPFW_NAT_LOADED (V_ipfw_nat_ready)
> >
> > instead of current version of that macro:
> >
> > #define IPFW_NAT_LOADED (ipfw_nat_ptr != NULL)
> >
> > I.e., perhaps in ipfw_nat_init() we could first set all the function
> > pointers, and then iterate over all vnets and set V_ipfw_nat ready
> > there. In ipfw_nat_destroy() we would first iterate over all vnets to
> > clear the flag, before clearing function pointers?
>
> I like you approach. Though insted of iterating vnets in
> ipfw_nat_init/destroy I think it is safe just to set/unset
> V_ipfw_nat_ready in vnet_ipfw_nat_init/uninit.
Yup that should be safe, provided you're 100% sure that
vnet_ipfw_nat_uninit() fires before ipfw_nat_destroy() - admittedly my
sysinit ordering insight got a bit rusty so I can't tell at the first
glance...
Anyhow, this looks fine to me...
Marko
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201308150025.34517.zec>