fine grain locking for tcp helper

PATCH In review Pablo Neira 2 participants 5 messages May 2, 2004 – May 3, 2004

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira

author

2004-05-02 03:40 UTC #0 lore raw

PATCH 1/1 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

net/ipv4/netfilter/ip_conntrack_proto_tcp.c +8 -8

@@ -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;
 }

include/linux/netfilter_ipv4/ip_conntrack_tcp.h +3 -0

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

Pablo Neira

author

2004-05-02 03:51 UTC #1 lore raw

Quoted 1 line · Pablo Neira

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

net/ipv4/netfilter/ip_conntrack_proto_tcp.c +8 -8

@@ -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;
 }

include/linux/netfilter_ipv4/ip_conntrack_tcp.h +3 -0

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

Jozsef Kadlecsik

2004-05-03 08:55 UTC #2 lore raw

In reply to Pablo Neira

Hi Pablo,

Quoted 3 lines · Pablo Neira

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 : [email protected], [email protected] 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

Pablo Neira

author

2004-05-03 13:55 UTC #3 lore raw

Hi Jozsef,

Quoted 5 lines · Jozsef Kadlecsik

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?

Quoted 5 lines

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?

Quoted 7 lines

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

Jozsef Kadlecsik

2004-05-03 14:49 UTC #4 lore raw

Hi Pablo,

Quoted 3 lines · Pablo Neira

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

:-)

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines
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 : [email protected], [email protected] 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