[PATCH] fine grain locking for tcp helper

Hi,

This patch provides a fine-grain locking for the tcp helper in 
conntrack. A per-conntrack lock is used, instead of having a global lock 
to protect tcp specific data. If I'm missing something, please let me know.

regards,
Pablo

--- linux-2.6.3-old/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-05-02
04:19:30.000000000 +0200
+++ linux-2.6.3/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-05-02 04:18:18.000000000 +0200
 <at>  <at>  -28,9 +28,6  <at>  <at> 
 #define DEBUGP(format, args...)
 #endif

-/* Protects conntrack->proto.tcp */
-static DECLARE_RWLOCK(tcp_lock);
-
 /* FIXME: Examine ipfilter's timeouts and conntrack transitions more
    closely.  They're more complex. --RR */

 <at>  <at>  -151,9 +148,9  <at>  <at> 
 {
 	enum tcp_conntrack state;

-	READ_LOCK(&tcp_lock);
+	READ_LOCK(&conntrack->proto.tcp.lock);
 	state = conntrack->proto.tcp.state;

Re: [PATCH] fine grain locking for tcp helper

Pablo Neira wrote:

> This patch provides a fine-grain locking for the tcp helper in 
> conntrack. A per-conntrack lock is used, instead of having a global 
> lock to protect tcp specific data. If I'm missing something, please 
> let me know.

oops, sorry, wrong patch, it doesn't compile, I made a mistake in the .h 
file, please review this patch instead.

regards,
Pablo

--- linux-2.6.3-old/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-05-02
04:19:30.000000000 +0200
+++ linux-2.6.3/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-05-02 04:18:18.000000000 +0200
 <at>  <at>  -28,9 +28,6  <at>  <at> 
 #define DEBUGP(format, args...)
 #endif

-/* Protects conntrack->proto.tcp */
-static DECLARE_RWLOCK(tcp_lock);
-
 /* FIXME: Examine ipfilter's timeouts and conntrack transitions more
    closely.  They're more complex. --RR */

 <at>  <at>  -151,9 +148,9  <at>  <at> 
 {
 	enum tcp_conntrack state;

Re: [PATCH] fine grain locking for tcp helper

Hi Pablo,

On Sun, 2 May 2004, Pablo Neira wrote:

> This patch provides a fine-grain locking for the tcp helper in
> conntrack. A per-conntrack lock is used, instead of having a global lock
> to protect tcp specific data. If I'm missing something, please let me know.

I'm not conviced that much could be gained with per TCP locking assuming
normal traffic, predominated by TCP. I'm fairly sure that per bucket
locking is the way we should go.

Could you check and revive the patches

conntrack_arefcount	- revised reference counting
conntrack_locking	- per bucket locking
[conntrack_nonat	- optimization for the conntrack-only case]

The patches could be pushed forward if there were independent
success reports for general cases, not just performance test reports.

Thank you your work indeed!

Best regards,
Jozsef
-
E-mail  : kadlec <at> blackhole.kfki.hu, kadlec <at> sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: locking issue in ip_conntrack_alter_reply

Hi Pablo,

On Fri, 30 Apr 2004, Pablo Neira wrote:

> Well, actually I found a contradiction here, we write-locked the
> conntrack table but that assertion says that this conntrack is not
> confirmed, this implies that the conntrack is not in the hash table yet.
> If that assertion is right, we should replace that write_lock for a
> read_lock while calling __ip_conntrack_find and remove that write_lock
> at the end of the function.
>
> int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
>                              const struct ip_conntrack_tuple *newreply)
> {
>         WRITE_LOCK(&ip_conntrack_lock);
>         if (__ip_conntrack_find(newreply, conntrack)) {
>                 WRITE_UNLOCK(&ip_conntrack_lock);
>                 return 0;
>         }
>         /* Should be unconfirmed, so not in hash table yet */
>         IP_NF_ASSERT(!is_confirmed(conntrack)); <--------------------------
>
>
> I'll study if that assertion is right before submitting a patch.

The assertion is right, because we alter unconfirmed packets when setting
up nat info. And you're also right, write-locking is unnecessary: the
entry is not in the hash table yet, so nobody else can grab and modify the
conntrack entry. [And it is overlooked in the per bucket locking patch too.]

Re: locking issue in ip_conntrack_alter_reply

Jozsef Kadlecsik wrote:

>On Fri, 30 Apr 2004, Pablo Neira wrote:
>
>  
>
>>I'll study if that assertion is right before submitting a patch.
>>    
>>
>
>The assertion is right, because we alter unconfirmed packets when setting
>up nat info. And you're also right, write-locking is unnecessary: the
>entry is not in the hash table yet, so nobody else can grab and modify the
>conntrack entry. [And it is overlooked in the per bucket locking patch too.]
>  
>

I've ported your conntrack patches to 2.6, I will include this change
before adding them to CVS.

Regards
Patrick

>Best regards,
>Jozsef
>-
>E-mail  : kadlec <at> blackhole.kfki.hu, kadlec <at> sunserv.kfki.hu
>PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
>Address : KFKI Research Institute for Particle and Nuclear Physics
>          H-1525 Budapest 114, POB. 49, Hungary

Re: locking issue in ip_conntrack_alter_reply

On Mon, 3 May 2004, Patrick McHardy wrote:

> I've ported your conntrack patches to 2.6, I will include this change
> before adding them to CVS.

Thanks! Finally, finally I'm before a a big pom-ng
(runme/Netfilter_POM.pm) update, stay tuned 

Best regards,
Jozsef
-
E-mail  : kadlec <at> blackhole.kfki.hu, kadlec <at> sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: locking issue in ip_conntrack_alter_reply

Hi Patrick,

Patrick McHardy wrote:

> Jozsef Kadlecsik wrote:
>
>> The assertion is right, because we alter unconfirmed packets when 
>> setting
>> up nat info. And you're also right, write-locking is unnecessary: the
>> entry is not in the hash table yet, so nobody else can grab and 
>> modify the
>> conntrack entry. [And it is overlooked in the per bucket locking 
>> patch too.]
>
>
> I've ported your conntrack patches to 2.6, I will include this change
> before adding them to CVS.

Attached the patch to fix this.

regards,
Pablo

Index: net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
RCS file: /home/cvs/linux-2.6/net/ipv4/netfilter/ip_conntrack_core.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	2 May 2004 13:26:10 -0000	1.1.1.1

Re: locking issue in ip_conntrack_alter_reply

On Mon, 3 May 2004, Pablo Neira wrote:

> Hi Patrick,
> 
> Patrick McHardy wrote:
> 
> > Jozsef Kadlecsik wrote:
> >
> >> The assertion is right, because we alter unconfirmed packets when 
> >> setting
> >> up nat info. And you're also right, write-locking is unnecessary: the
> >> entry is not in the hash table yet, so nobody else can grab and 
> >> modify the
> >> conntrack entry. [And it is overlooked in the per bucket locking 
> >> patch too.]
> >
> >
> > I've ported your conntrack patches to 2.6, I will include this change
> > before adding them to CVS.
> 
> 
> Attached the patch to fix this.

Isn't there a LOCK/UNLOCK imbalance in your patch?

Regards
Henrik

Re: locking issue in ip_conntrack_alter_reply

Hi,

Henrik Nordstrom wrote:

>>Attached the patch to fix this.
>>    
>>
>
>Isn't there a LOCK/UNLOCK imbalance in your patch?
>  
>

you are right, I should rush...  Sorry!

regards,
Pablo

Index: net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
RCS file: /home/cvs/linux-2.6/net/ipv4/netfilter/ip_conntrack_core.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	2 May 2004 13:26:10 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	3 May 2004 13:23:43 -0000
 <at>  <at>  -1093,9 +1093,9  <at>  <at> 
 int ip_conntrack_alter_reply(struct ip_conntrack *conntrack,
 			     const struct ip_conntrack_tuple *newreply)
 {

Re: [PATCH] fine grain locking for tcp helper

Hi Jozsef,

Jozsef Kadlecsik wrote:

>I'm not conviced that much could be gained with per TCP locking assuming
>normal traffic, predominated by TCP. I'm fairly sure that per bucket
>locking is the way we should go.
>  
>

Actually I was having a look at conntrack_locking for per bucket locking 
some days ago, I find it interesting. I had in mind to port them to 2.6 
but Patrick was faster .

BTW, is there any problem in using both patches at the same time? If we 
have a machine with mostly tcp traffic, when we take a conntrack, two 
buckets will be locked (for original and reply tuples), but when calling 
tcp specific functions, it will spin until tcp_lock is released possibly 
by other conntrack.

mmm could this reduce the time both buckets are locked?

>Could you check and revive the patches
>
>conntrack_arefcount	- revised reference counting
>  
>
     ^^^
Is conntrack_locking dependent of this patch? in pom-ng is marked as 
dependent, but I don't understand why.