[PATCH] fine grain locking for tcp helper

[PATCH] fine grain locking for tcp helper

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

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

[-- Attachment #2: locking_tcp_proto_helper.patch --]
[-- Type: text/plain, Size: 2112 bytes --]

--- 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
@@ -28,9 +28,6 @@
 #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 */
 
@@ -151,9 +148,9 @@
 {
 	enum tcp_conntrack state;
 
-	READ_LOCK(&tcp_lock);
+	READ_LOCK(&conntrack->proto.tcp.lock);
 	state = conntrack->proto.tcp.state;
-	READ_UNLOCK(&tcp_lock);
+	READ_UNLOCK(&conntrack->proto.tcp.lock);
 
 	return sprintf(buffer, "%s ", tcp_conntrack_names[state]);
 }
@@ -188,7 +185,7 @@
 		return NF_ACCEPT;
 	}
 
-	WRITE_LOCK(&tcp_lock);
+	WRITE_LOCK(&conntrack->proto.tcp.lock);
 	oldtcpstate = conntrack->proto.tcp.state;
 	newconntrack
 		= tcp_conntracks
@@ -200,7 +197,7 @@
 		DEBUGP("ip_conntrack_tcp: Invalid dir=%i index=%u conntrack=%u\n",
 		       CTINFO2DIR(ctinfo), get_conntrack_index(&tcph),
 		       conntrack->proto.tcp.state);
-		WRITE_UNLOCK(&tcp_lock);
+		WRITE_UNLOCK(&conntrack->proto.tcp.lock);
 		return -1;
 	}
 
@@ -222,7 +219,7 @@
 	    && tcph.ack_seq == conntrack->proto.tcp.handshake_ack)
 		set_bit(IPS_ASSURED_BIT, &conntrack->status);
 
-out:	WRITE_UNLOCK(&tcp_lock);
+out:	WRITE_UNLOCK(&conntrack->proto.tcp.lock);
 	ip_ct_refresh(conntrack, *tcp_timeouts[newconntrack]);
 
 	return NF_ACCEPT;
@@ -249,6 +246,9 @@
 	}
 
 	conntrack->proto.tcp.state = newconntrack;
+	/* make sure that lock is correctly initialized */
+	conntrack->proto.tcp.lock = RW_LOCK_UNLOCKED;
+
 	return 1;
 }
 
--- linux-2.6.3-old/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-02-18 04:57:29.000000000 +0100
+++ linux-2.6.3/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-05-02 04:19:04.000000000 +0200
@@ -18,6 +18,9 @@
 
 struct ip_ct_tcp
 {
+	/* Protects conntrack tcp protocol specific information */
+	rwlock_t tcp_lock;
+
 	enum tcp_conntrack state;
 
 	/* Poor man's window tracking: sequence number of valid ACK

Re: [PATCH] fine grain locking for tcp helper

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

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

[-- Attachment #2: locking_tcp_proto_helper.patch --]
[-- Type: text/plain, Size: 2108 bytes --]

--- 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
@@ -28,9 +28,6 @@
 #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 */
 
@@ -151,9 +148,9 @@
 {
 	enum tcp_conntrack state;
 
-	READ_LOCK(&tcp_lock);
+	READ_LOCK(&conntrack->proto.tcp.lock);
 	state = conntrack->proto.tcp.state;
-	READ_UNLOCK(&tcp_lock);
+	READ_UNLOCK(&conntrack->proto.tcp.lock);
 
 	return sprintf(buffer, "%s ", tcp_conntrack_names[state]);
 }
@@ -188,7 +185,7 @@
 		return NF_ACCEPT;
 	}
 
-	WRITE_LOCK(&tcp_lock);
+	WRITE_LOCK(&conntrack->proto.tcp.lock);
 	oldtcpstate = conntrack->proto.tcp.state;
 	newconntrack
 		= tcp_conntracks
@@ -200,7 +197,7 @@
 		DEBUGP("ip_conntrack_tcp: Invalid dir=%i index=%u conntrack=%u\n",
 		       CTINFO2DIR(ctinfo), get_conntrack_index(&tcph),
 		       conntrack->proto.tcp.state);
-		WRITE_UNLOCK(&tcp_lock);
+		WRITE_UNLOCK(&conntrack->proto.tcp.lock);
 		return -1;
 	}
 
@@ -222,7 +219,7 @@
 	    && tcph.ack_seq == conntrack->proto.tcp.handshake_ack)
 		set_bit(IPS_ASSURED_BIT, &conntrack->status);
 
-out:	WRITE_UNLOCK(&tcp_lock);
+out:	WRITE_UNLOCK(&conntrack->proto.tcp.lock);
 	ip_ct_refresh(conntrack, *tcp_timeouts[newconntrack]);
 
 	return NF_ACCEPT;
@@ -249,6 +246,9 @@
 	}
 
 	conntrack->proto.tcp.state = newconntrack;
+	/* make sure that lock is correctly initialized */
+	conntrack->proto.tcp.lock = RW_LOCK_UNLOCKED;
+
 	return 1;
 }
 
--- linux-2.6.3-old/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-02-18 04:57:29.000000000 +0100
+++ linux-2.6.3/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-05-02 05:48:49.000000000 +0200
@@ -18,6 +18,9 @@
 
 struct ip_ct_tcp
 {
+	/* Protects conntrack tcp protocol specific information */
+	rwlock_t lock;
+
 	enum tcp_conntrack state;
 
 	/* Poor man's window tracking: sequence number of valid ACK

Re: [PATCH] fine grain locking for tcp helper

[Jozsef Kadlecsik]

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@blackhole.kfki.hu, kadlec@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: [PATCH] fine grain locking for tcp helper

[Pablo Neira]

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.

If there's no real dependency I think that I could only test 
conntrack_locking which is more simple (well, I think), this way we 
could push for kernel inclusion, and after that, go test the revised 
reference counting. Am I missing anything?

>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.
>  
>
sure, I'll have a look at them :-).

regards,
Pablo

Re: [PATCH] fine grain locking for tcp helper

[Jozsef Kadlecsik]

Hi Pablo,

On Mon, 3 May 2004, Pablo Neira wrote:

> 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.

I see, your're right. Actually, it seems I was blind, sorry. Both patches
could be used and your patch could give more speed.

> >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.

It's a must. Without the revised refcounting and well-separated
ip_conntrack_lock and ip_conntrack_expect_lock, per bucket locking cannot
simply be introduced. The locking patch cannot be applied without it.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@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