From owner-freebsd-virtualization@FreeBSD.ORG Wed Aug 14 22:25:25 2013 Return-Path: Delivered-To: freebsd-virtualization@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 AA53D313; Wed, 14 Aug 2013 22:25:25 +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 3BE0E2BA8; Wed, 14 Aug 2013 22:25:24 +0000 (UTC) Received: from x23.lan (89.164.9.9) by MAIL.fer.hr (161.53.72.233) with Microsoft SMTP Server (TLS) id 14.2.342.3; Thu, 15 Aug 2013 00:25:21 +0200 From: Marko Zec To: Mikolaj Golub Subject: Re: RFC: ipfw nat VIMAGE improvements Date: Thu, 15 Aug 2013 00:25:34 +0200 User-Agent: KMail/1.9.10 References: <20130811200111.GA49895@gmail.com> <201308141728.31361.zec@fer.hr> <20130814204303.GA13541@gmail.com> In-Reply-To: <20130814204303.GA13541@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-ID: <201308150025.34517.zec@fer.hr> X-Originating-IP: [89.164.9.9] Cc: freebsd-virtualization@freebsd.org X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Aug 2013 22:25:25 -0000 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