[RFC][PATCH 0/7]: ct_extend

[RFC][PATCH 0/7]: ct_extend

[Yasuyuki KOZAKAI]

Hi all,

I'm re-writing ct_extend patchset. This patchset is a snapshot.
Like Rusty's original one, it allocates memory area by kmem_cache_alloc()
to store basic informations about a connection, but it uses kmalloc() to
allocate memory area for extension (currently helper and NAT).

Some concepts are following.
- 'struct ct_extend' keeps offsets to each area and the total size of them,
  instead of identifiers of extensions. This scheme can eliminate loops and
  make codes simple, but limits the total size of area to 256 bytes. Is it
  too small ? If so, we can be back to the original ct_extend.

- To add a new area, it reallocates ct_extend. To fix the pointer to it from
  other objects, the 'move' operation defined by extension is used.
  
- It does not have the operation to remove a memory area from
  ct_extend. Because it causes reallocating and restructuring ct_extend.
  It is complicated operation.  And removing extension from all conntracks
  would require heavy processing. I prefer to leave unused area.

- I added 'destroy' operation which is called in destroy_conntrack(). It
  can kill global operation 'nf_conntrack_destroyed' used by NAT.

The issues I noticed are following.
- This patchset does not consider locking. The places calling nfct_help() or
  nfct_nat() must be protected with new read-write lock. It will add
  some processing time and complexities.

- ct_extend_add() reallocates memory area for all extensions. All helpers
  and NAT codes are necessary to take care that the pointer nfct_help(ct)
  and nfct_nat(ct) might be changed. It would make difficult to find bugs
  caused by this issue.

And some improvements can be considered/are necessary.
- Introducing new locking
- 'move' operation might be better to have a argument 'struct nf_conn *ct'.
- Moving other stuff in 'struct nf_conn' to ct_extend
- Spliting nf_conn_nat into helper parts and others
- Fixes of function names (nf_ct_ext_, nfct_ext_, nf_cte_, or ?)

Comments are welcome.

-- Yasuyuki Kozakai

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

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> I'm re-writing ct_extend patchset. This patchset is a snapshot.
> Like Rusty's original one, it allocates memory area by kmem_cache_alloc()
> to store basic informations about a connection, but it uses kmalloc() to
> allocate memory area for extension (currently helper and NAT).
> 
> Some concepts are following.
> - 'struct ct_extend' keeps offsets to each area and the total size of them,
>   instead of identifiers of extensions. This scheme can eliminate loops and
>   make codes simple, but limits the total size of area to 256 bytes. Is it
>   too small ? If so, we can be back to the original ct_extend.


I think this will do, I believe we're somewhere in the 40-50 byte range
now with both NAT and helpers.

> - To add a new area, it reallocates ct_extend. To fix the pointer to it from
>   other objects, the 'move' operation defined by extension is used.
>   
> - It does not have the operation to remove a memory area from
>   ct_extend. Because it causes reallocating and restructuring ct_extend.
>   It is complicated operation.  And removing extension from all conntracks
>   would require heavy processing. I prefer to leave unused area.


Agreed.

> - I added 'destroy' operation which is called in destroy_conntrack(). It
>   can kill global operation 'nf_conntrack_destroyed' used by NAT.


Nice work :)

> The issues I noticed are following.
> - This patchset does not consider locking. The places calling nfct_help() or
>   nfct_nat() must be protected with new read-write lock. It will add
>   some processing time and complexities.


This will add quite some overhead since at least nfct_help is looked at
for every packet (we might avoid this using a status flag though).
I'm guessing the locks are needed to protect against concurrent
reallocations. Maybe we can avoid this by adding the limitation that
only unconfirmed entries may add new extensions?

> - ct_extend_add() reallocates memory area for all extensions. All helpers
>   and NAT codes are necessary to take care that the pointer nfct_help(ct)
>   and nfct_nat(ct) might be changed. It would make difficult to find bugs
>   caused by this issue.


Thats something we'll have to live with I guess. The same is true for
the skb reallocation functions, maybe we can get a sparse check for
this some day.

> And some improvements can be considered/are necessary.
> - Introducing new locking
> - 'move' operation might be better to have a argument 'struct nf_conn *ct'.
> - Moving other stuff in 'struct nf_conn' to ct_extend
> - Spliting nf_conn_nat into helper parts and others
> - Fixes of function names (nf_ct_ext_, nfct_ext_, nf_cte_, or ?)


I prefer nf_ct_ext_ or nfct_ext_.

> Comments are welcome.


I'm thinking we could probably avoid some reallocations by doing
a more accurate CT_EXTEND_MIN_SIZE calculation by taking
nf_nat_module_is_loaded into account. Only makes sense if it
currently is smaller than the space needed for NAT+MASQUERADE
of course.

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

[Yasuyuki KOZAKAI]

Hi, Patrick,

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 08 May 2007 11:46:40 +0200

> > The issues I noticed are following.
> > - This patchset does not consider locking. The places calling nfct_help() or
> >   nfct_nat() must be protected with new read-write lock. It will add
> >   some processing time and complexities.
> 
> 
> This will add quite some overhead since at least nfct_help is looked at
> for every packet (we might avoid this using a status flag though).
> I'm guessing the locks are needed to protect against concurrent
> reallocations. Maybe we can avoid this by adding the limitation that
> only unconfirmed entries may add new extensions?

It sounds good to avoid competition of 2 packet processing. I think
that the remain is nfctnetlink. It can assign helper to a confirmed
conntrack. Do you think that it's enough to assume that it disables bh ?

> > - ct_extend_add() reallocates memory area for all extensions. All helpers
> >   and NAT codes are necessary to take care that the pointer nfct_help(ct)
> >   and nfct_nat(ct) might be changed. It would make difficult to find bugs
> >   caused by this issue.
> 
> 
> Thats something we'll have to live with I guess. The same is true for
> the skb reallocation functions, maybe we can get a sparse check for
> this some day.

I agree.

> > And some improvements can be considered/are necessary.
> > - Introducing new locking
> > - 'move' operation might be better to have a argument 'struct nf_conn *ct'.
> > - Moving other stuff in 'struct nf_conn' to ct_extend
> > - Spliting nf_conn_nat into helper parts and others
> > - Fixes of function names (nf_ct_ext_, nfct_ext_, nf_cte_, or ?)
> 
> 
> I prefer nf_ct_ext_ or nfct_ext_.

Ok, I'll replace them with nf_ct_ext_*.

> I'm thinking we could probably avoid some reallocations by doing
> a more accurate CT_EXTEND_MIN_SIZE calculation by taking
> nf_nat_module_is_loaded into account. Only makes sense if it
> currently is smaller than the space needed for NAT+MASQUERADE
> of course.

Good idea. On my laptop, CT_EXTEND_MIN_SIZE is 32 bytes, the size of
helper area is 32 bytes, and 36 bytes for NAT. The helper area is allocated
before the NAT, so pre-allocating NAT area can reduce the number of
allocation. I'll add a flag to 'ct_extend_type' to intend pre-allocating.

Thanks for comments,

-- Yasuyuki Kozakai

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

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Tue, 08 May 2007 11:46:40 +0200
> 
>>This will add quite some overhead since at least nfct_help is looked at
>>for every packet (we might avoid this using a status flag though).
>>I'm guessing the locks are needed to protect against concurrent
>>reallocations. Maybe we can avoid this by adding the limitation that
>>only unconfirmed entries may add new extensions?
> 
> 
> It sounds good to avoid competition of 2 packet processing. I think
> that the remain is nfctnetlink. It can assign helper to a confirmed
> conntrack. Do you think that it's enough to assume that it disables bh ?


No, that would only help for the CPU its running on. But so far
we only allow to change helpers, not assign completely new ones,
so no reallocation is needed. I'm wondering if this behaviour
is correct though ..

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

[Yasuyuki KOZAKAI]

Hi, Patrick, Pablo and all,

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 10 May 2007 14:34:00 +0200

> >>This will add quite some overhead since at least nfct_help is looked at
> >>for every packet (we might avoid this using a status flag though).
> >>I'm guessing the locks are needed to protect against concurrent
> >>reallocations. Maybe we can avoid this by adding the limitation that
> >>only unconfirmed entries may add new extensions?
> > 
> > 
> > It sounds good to avoid competition of 2 packet processing. I think
> > that the remain is nfctnetlink. It can assign helper to a confirmed
> > conntrack. Do you think that it's enough to assume that it disables bh ?
> 
> 
> No, that would only help for the CPU its running on. But so far
> we only allow to change helpers, not assign completely new ones,
> so no reallocation is needed. I'm wondering if this behaviour
> is correct though ..

IIRC, Harald had a idea of userland helper using nfctnetlink and nfqueue (2
years ago ?). ??? Please wait...... oh.. but to do that, nfctnetlink is
enough to assign a helper to an unconfirmed conntrack (currently it does
not).

And I'm not sure about the case of conntrackd.

Pablo, is there any case that conntrackd assign helper to a confirmed
conntrack ?

-- Yasuyuki Kozakai

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

[Mohammad Mohsenzadeh]

Thanks for the patches, applied and works just fine. I was just
wondering if you would ever consider including these in the main
release of netfilter??

Thanks again
Mohammad

On 5/7/07, Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp> wrote:
>
> Hi all,
>
> I'm re-writing ct_extend patchset. This patchset is a snapshot.
> Like Rusty's original one, it allocates memory area by kmem_cache_alloc()
> to store basic informations about a connection, but it uses kmalloc() to
> allocate memory area for extension (currently helper and NAT).
>
> Some concepts are following.
> - 'struct ct_extend' keeps offsets to each area and the total size of them,
>   instead of identifiers of extensions. This scheme can eliminate loops and
>   make codes simple, but limits the total size of area to 256 bytes. Is it
>   too small ? If so, we can be back to the original ct_extend.
>
> - To add a new area, it reallocates ct_extend. To fix the pointer to it from
>   other objects, the 'move' operation defined by extension is used.
>
> - It does not have the operation to remove a memory area from
>   ct_extend. Because it causes reallocating and restructuring ct_extend.
>   It is complicated operation.  And removing extension from all conntracks
>   would require heavy processing. I prefer to leave unused area.
>
> - I added 'destroy' operation which is called in destroy_conntrack(). It
>   can kill global operation 'nf_conntrack_destroyed' used by NAT.
>
> The issues I noticed are following.
> - This patchset does not consider locking. The places calling nfct_help() or
>   nfct_nat() must be protected with new read-write lock. It will add
>   some processing time and complexities.
>
> - ct_extend_add() reallocates memory area for all extensions. All helpers
>   and NAT codes are necessary to take care that the pointer nfct_help(ct)
>   and nfct_nat(ct) might be changed. It would make difficult to find bugs
>   caused by this issue.
>
> And some improvements can be considered/are necessary.
> - Introducing new locking
> - 'move' operation might be better to have a argument 'struct nf_conn *ct'.
> - Moving other stuff in 'struct nf_conn' to ct_extend
> - Spliting nf_conn_nat into helper parts and others
> - Fixes of function names (nf_ct_ext_, nfct_ext_, nf_cte_, or ?)
>
> Comments are welcome.
>
> -- Yasuyuki Kozakai
>
>

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

[Patrick McHardy]

Mohammad Mohsenzadeh wrote:
> Thanks for the patches, applied and works just fine. I was just
> wondering if you would ever consider including these in the main
> release of netfilter??


Thanks for testing. I'm also hoping we can get this ready ASAP
to get rid of the overhead of searching the expectations/helpers
twice. I'm even considering switching to worst-case allocations
for the time being since it costs us quite a bit of performance.

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

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 31 May 2007 06:38:49 +0200

> Mohammad Mohsenzadeh wrote:
> > Thanks for the patches, applied and works just fine. I was just
> > wondering if you would ever consider including these in the main
> > release of netfilter??
> 
> 
> Thanks for testing. I'm also hoping we can get this ready ASAP
> to get rid of the overhead of searching the expectations/helpers
> twice. I'm even considering switching to worst-case allocations
> for the time being since it costs us quite a bit of performance.

I've revisited the issue of competition between NAT referring
extension area for NAT and nfctnetlink trying to assign helper (which might
result in reallocating extension area for NAT).

And I've found that the current nfctnetlink has similar but different
problem. It is possible to change helper infomations while helper referring
them.

After all, if we don't want to introduce rwlock for such competition,
we'd better to limit nfctnetlink so that it doesn't assign, change, or
remove helper of confirmed conntrack.

If people agree to remove ctnetlink_change_helper(), I'll submit the latest
pactchset of ct_extend.

-- Yasuyuki Kozakai

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

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> I've revisited the issue of competition between NAT referring
> extension area for NAT and nfctnetlink trying to assign helper (which might
> result in reallocating extension area for NAT).
> 
> And I've found that the current nfctnetlink has similar but different
> problem. It is possible to change helper infomations while helper referring
> them.
> 
> After all, if we don't want to introduce rwlock for such competition,
> we'd better to limit nfctnetlink so that it doesn't assign, change, or
> remove helper of confirmed conntrack.
> 
> If people agree to remove ctnetlink_change_helper(), I'll submit the latest
> pactchset of ct_extend.


I don't think we can do that, it has been part of the ABI since the
beginning I think and we might need it for userspace helpers.

How about grabbing nf_conntrack_lock and replacing the entire conntrack
structure in this case?

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

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 01 Jun 2007 17:33:59 +0200

> > After all, if we don't want to introduce rwlock for such competition,
> > we'd better to limit nfctnetlink so that it doesn't assign, change, or
> > remove helper of confirmed conntrack.
> > 
> > If people agree to remove ctnetlink_change_helper(), I'll submit the latest
> > pactchset of ct_extend.
> 
> 
> I don't think we can do that, it has been part of the ABI since the
> beginning I think and we might need it for userspace helpers.

OK, I'll try to find an other idea.

> How about grabbing nf_conntrack_lock and replacing the entire conntrack
> structure in this case?

It requires very complicated operations. I want to avoid that as possible.

I'm thinking about exporting nf_nat_lock and locks of helpers, and grabbing
all of them just before reallocating extended area. But I'm not sure it is
possible because we have to take care about deadlock due to grabbing
multiple locks in different order.

One more idea is Rusty's comment in the original patch, it replaces array
of extended area with linked list. But it results in so many small memory
objects. Hmm...

-- Yasuyuki Kozakai

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

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Fri, 01 Jun 2007 17:33:59 +0200
> 
>>How about grabbing nf_conntrack_lock and replacing the entire conntrack
>>structure in this case?
> 
> 
> It requires very complicated operations. I want to avoid that as possible.
> 
> I'm thinking about exporting nf_nat_lock and locks of helpers, and grabbing
> all of them just before reallocating extended area. But I'm not sure it is
> possible because we have to take care about deadlock due to grabbing
> multiple locks in different order.
> 
> One more idea is Rusty's comment in the original patch, it replaces array
> of extended area with linked list. But it results in so many small memory
> objects. Hmm...


If it results in one allocation per extension I don't think its a good
idea, one benefit of the array is that we can allocate the room for
multiple extensions at once if we already know its going to be needed.