RFC: ipfw nat VIMAGE improvements

[Mikolaj Golub]

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.

-- 
Mikolaj Golub
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ip_fw_nat.c.VIMAGE.2.patch
Type: text/x-diff
Size: 6229 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-virtualization/attachments/20130814/3b4f2207/attachment.patch>