[RFC][PATCH 0/10]: ct_extend

[RFC][PATCH 0/10]: ct_extend

[Yasuyuki KOZAKAI]

Hi,

This patch set is a snapshot of the ct_extend, which shrinks memory
usage where extensions such as NAT and helper are not used.

I've applied all suggestions from Patrick. If nat module is loaded,
ct_extend pre-allocates extended area of NAT when it initially allocates
extended area of helper. I've also renamed functions to nf_ct_ext_* and
merged the members in 'struct nf_nat_info' into 'struct nf_conn_nat'.

-- Yasuyuki Kozakai

Re: [RFC][PATCH 0/10]: ct_extend

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> Hi,
> 
> This patch set is a snapshot of the ct_extend, which shrinks memory
> usage where extensions such as NAT and helper are not used.
> 
> I've applied all suggestions from Patrick. If nat module is loaded,
> ct_extend pre-allocates extended area of NAT when it initially allocates
> extended area of helper. I've also renamed functions to nf_ct_ext_* and
> merged the members in 'struct nf_nat_info' into 'struct nf_conn_nat'.


The patches look pretty good to me. For normal conntracks without
NAT or helpers its a clear win since we avoid the double search of
expectations. The only case I'm a bit worried about is NAT, for that
case we get an extra allocation for every new connection. But I
guess thats better than what we have now, the double search reportedly
costs about 10% performance for all cases.

Do you think we should put these patches in 2.6.23?

Re: [RFC][PATCH 0/10]: ct_extend

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 25 Jun 2007 12:16:35 +0200

> Yasuyuki KOZAKAI wrote:
> > Hi,
> > 
> > This patch set is a snapshot of the ct_extend, which shrinks memory
> > usage where extensions such as NAT and helper are not used.
> > 
> > I've applied all suggestions from Patrick. If nat module is loaded,
> > ct_extend pre-allocates extended area of NAT when it initially allocates
> > extended area of helper. I've also renamed functions to nf_ct_ext_* and
> > merged the members in 'struct nf_nat_info' into 'struct nf_conn_nat'.
> 
> 
> The patches look pretty good to me. For normal conntracks without
> NAT or helpers its a clear win since we avoid the double search of
> expectations. The only case I'm a bit worried about is NAT, for that
> case we get an extra allocation for every new connection. But I
> guess thats better than what we have now, the double search reportedly
> costs about 10% performance for all cases.
> 
> Do you think we should put these patches in 2.6.23?

I think it's ready.

I've re-checked race at nf_conntrack_netlink and I think there is no
race on members in nfct_nat(ct) at ct_netlink_change_helper(). Because

- 'bysource' and 'ct' are protected by nf_nat_lock when the area of
  nfct_nat(ct) is reallocated.
- 'seq' and 'help' are used by only helper, but we don't change helper.
  So there is no race.
- 'masq_index' is set in ipt_MASQUERADE.c while conntrack is
  unconfirmed. Besides nfctnetlink can change only confirmed conntracks.
  And 'masq_index' in confirmed conntrack is never changed.

BTW, I don't understand why ipt_MQASQUERADE needs lock. I think there is no
difference of results if we remove the lock. If we want to completely
discards conntracks related to output device, MASQUERADE should wait
in *_event() until all packets pass through network stack. But I'm not sure
it's possible.


Anyway, I'll resend updated patches.

-- Yasuyuki Kozakai

Re: [RFC][PATCH 0/10]: ct_extend

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon, 25 Jun 2007 12:16:35 +0200
> 
>>Do you think we should put these patches in 2.6.23?
> 
> 
> I think it's ready.

Great.

> I've re-checked race at nf_conntrack_netlink and I think there is no
> race on members in nfct_nat(ct) at ct_netlink_change_helper(). Because
> 
> - 'bysource' and 'ct' are protected by nf_nat_lock when the area of
>   nfct_nat(ct) is reallocated.
> - 'seq' and 'help' are used by only helper, but we don't change helper.
>   So there is no race.
> - 'masq_index' is set in ipt_MASQUERADE.c while conntrack is
>   unconfirmed. Besides nfctnetlink can change only confirmed conntracks.
>   And 'masq_index' in confirmed conntrack is never changed.

Sounds right.

> BTW, I don't understand why ipt_MQASQUERADE needs lock. I think there is no
> difference of results if we remove the lock. If we want to completely
> discards conntracks related to output device, MASQUERADE should wait
> in *_event() until all packets pass through network stack. But I'm not sure
> it's possible.


Yes it looks unnecessary. Reliably removing all conntracks is not
really necessary, its only an optimization to keep the table smaller.

I'm actually happy about every missed connection since my DSL line
has an almost permanent address and connections usually live on
after a reconnect unless NAT remaps them :)