Re: nf_conntrack allocation issue

[Patrick McHardy <kaber@trash.net>]

Jozsef Kadlecsik wrote:
> Hi Patrick,
> 
> In your patch the part
> 
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -579,7 +579,8 @@ __nf_conntrack_alloc(const struct nf_con
>         /* FIXME: protect helper list per RCU */
>         read_lock_bh(&nf_conntrack_lock);
>         helper = __nf_ct_helper_find(repl);
> -       if (helper)
> +       /* NAT might want to assign a helper later */
> +       if (helper || features & NF_CT_F_NAT)
>                 features |= NF_CT_F_HELP;
>         read_unlock_bh(&nf_conntrack_lock);
>  
> means that in the IPv4+NAT case we loose what we wanted to gain by the 
> features, i.e. for IPv4 with NAT all conntrack entries will be full sized.

Yes - actually that part is similar to your patch besides the
keep_helper option.

>>we need to search for both helpers and expectations twice for
>>every new connection, so this needs to be redesigned anyway.
> 
> 
> As I see this is the price we pay for the features.

I see it as a sign of serious misfit of the allocation scheme.
The helper and expectation lookups both really suck performance
wise already, doing them twice is just horrible.

> What disturbs me is that by the patch segment above, in spite of the 
> features we does not gain anything in memory footprint. And the only 
> reason for this is that we execute an internal helper lookup after (D)NAT, 
> which can *possibly* change the helper (and which is a nice feature).
> 
> In order not to loose practically the features in the IPv4+NAT case I 
> added the 'keep_helper' module parameter. Because - I think - the typical 
> DNAT setups do not rely on the second helper lookup, the good default 
> value were '1', i.e. do not waste time on looking up the helper again 
> after DNAT *and* in the new structure keep the features. The backward 
> compatibility would dictate that the default value is '0'.
> 
> I do not argue for the module parameter. But let us do not throw away the 
> features in the case of the most usual configuration in the present days.

The reason why I didn't want to add it is that IMO the allocation
scheme needs to be redesigned anyway, so when we do it we might end
up with a module parameter we had for two months and need to carry
around forever. We can still add it any time we want, but for now
my priority is to get it right (i.e. working properly, not
optimized) without doing anything we might regret later. Module
parameters for rather obscure internal stuff tend not to get used
anyway, so most users don't loose anything.