*icsk_ca_priv: Reduce inet_connection_sock size?

PATCH In review Daniele Lacamera 3 participants 7 messages Aug 31, 2008 – Sep 1, 2008

Subsystems (4)

Networking Network drivers TCP TCP low priority module

lore

Share & tools

lore

Daniele Lacamera

author

2008-08-31 18:26 UTC #0 lore raw

PATCH 1/1 *icsk_ca_priv: Reduce inet_connection_sock size?

This simple patch (2.6.26) replaces the icsk_ca_priv big array with a void pointer, reducing the default inet_connection_sock size by 60Bytes.

Each "congestion controlled" socket must allocate and free its own priv structure if needed, from the connection init and release callbacks inside each module.

Wouldn't this reduce memory usage for sockets that don't use ca?

Thanks

Signature · 1 line

Daniele

include/net/inet_connection_sock.h +1 -1

diff -ruN a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
@@ -123,7 +123,7 @@
 		/* Information on the current probe. */
 		int		  probe_size;
 	} icsk_mtup;
-	u32			  icsk_ca_priv[16];
+	void 			  *icsk_ca_priv;
 #define ICSK_CA_PRIV_SIZE	(16 * sizeof(u32))
 };

include/net/tcp.h +3 -0

diff -ruN a/include/net/tcp.h b/include/net/tcp.h
@@ -673,6 +673,9 @@
 	struct module 	*owner;
 };
 
+extern void *tcp_register_ca_priv(struct sock *sk, u32 size);
+extern void tcp_release_ca_priv(struct sock *sk);
+
 extern int tcp_register_congestion_control(struct tcp_congestion_ops *type);
 extern void tcp_unregister_congestion_control(struct tcp_congestion_ops *type);

net/ipv4/tcp_bic.c +7 -0

diff -ruN a/net/ipv4/tcp_bic.c b/net/ipv4/tcp_bic.c
@@ -70,8 +70,14 @@
 	ca->delayed_ack = 2 << ACK_RATIO_SHIFT;
 }
 
+static void bictcp_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void bictcp_init(struct sock *sk)
 {
+	tcp_register_ca_priv(sk, sizeof(struct bictcp));
 	bictcp_reset(inet_csk_ca(sk));
 	if (initial_ssthresh)
 		tcp_sk(sk)->snd_ssthresh = initial_ssthresh;

@@ -220,6 +226,7 @@
 
 static struct tcp_congestion_ops bictcp = {
 	.init		= bictcp_init,
+	.release	= bictcp_release,
 	.ssthresh	= bictcp_recalc_ssthresh,
 	.cong_avoid	= bictcp_cong_avoid,
 	.set_state	= bictcp_state,

net/ipv4/tcp_cong.c +21 -0

diff -ruN a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
@@ -73,6 +73,27 @@
 }
 EXPORT_SYMBOL_GPL(tcp_unregister_congestion_control);
 
+void *tcp_register_ca_priv(struct sock *sk, u32 size)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	if (size > ICSK_CA_PRIV_SIZE){
+		return NULL;	
+	}
+	icsk->icsk_ca_priv = kmalloc(size, GFP_KERNEL);
+	return icsk->icsk_ca_priv;
+}
+EXPORT_SYMBOL_GPL(tcp_register_ca_priv);
+
+void tcp_release_ca_priv(struct sock *sk)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	if (icsk->icsk_ca_priv) {
+		kfree(icsk->icsk_ca_priv);
+		icsk->icsk_ca_priv = (void *)0;
+	}
+}
+EXPORT_SYMBOL_GPL(tcp_release_ca_priv);
+
 /* Assign choice of congestion control. */
 void tcp_init_congestion_control(struct sock *sk)
 {

net/ipv4/tcp_cubic.c +7 -0

diff -ruN a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
@@ -78,8 +78,14 @@
 	ca->tcp_cwnd = 0;
 }
 
+static void bictcp_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void bictcp_init(struct sock *sk)
 {
+	tcp_register_ca_priv(sk, sizeof(struct bictcp));
 	bictcp_reset(inet_csk_ca(sk));
 	if (initial_ssthresh)
 		tcp_sk(sk)->snd_ssthresh = initial_ssthresh;

@@ -318,6 +324,7 @@
 
 static struct tcp_congestion_ops cubictcp = {
 	.init		= bictcp_init,
+	.release	= bictcp_release,
 	.ssthresh	= bictcp_recalc_ssthresh,
 	.cong_avoid	= bictcp_cong_avoid,
 	.set_state	= bictcp_state,

net/ipv4/tcp_highspeed.c +7 -1

diff -ruN a/net/ipv4/tcp_highspeed.c b/net/ipv4/tcp_highspeed.c
@@ -97,10 +97,15 @@
 	u32	ai;
 };
 
+static void hstcp_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void hstcp_init(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct hstcp *ca = inet_csk_ca(sk);
+	struct hstcp *ca = tcp_register_ca_priv(sk, sizeof(struct hstcp));
 
 	ca->ai = 0;

@@ -160,6 +165,7 @@
 
 static struct tcp_congestion_ops tcp_highspeed = {
 	.init		= hstcp_init,
+	.release	= hstcp_release,
 	.ssthresh	= hstcp_ssthresh,
 	.cong_avoid	= hstcp_cong_avoid,
 	.min_cwnd	= tcp_reno_min_cwnd,

net/ipv4/tcp_htcp.c +8 -2

diff -ruN a/net/ipv4/tcp_htcp.c b/net/ipv4/tcp_htcp.c
@@ -251,10 +251,15 @@
 	}
 }
 
-static void htcp_init(struct sock *sk)
+static void htcp_release(struct sock *sk)
 {
-	struct htcp *ca = inet_csk_ca(sk);
+	tcp_release_ca_priv(sk);	
+}
 
+static void htcp_init(struct sock *sk)
+{
+	struct htcp *ca = tcp_register_ca_priv(sk, sizeof(struct htcp));
+	
 	memset(ca, 0, sizeof(struct htcp));
 	ca->alpha = ALPHA_BASE;
 	ca->beta = BETA_MIN;

@@ -281,6 +286,7 @@
 
 static struct tcp_congestion_ops htcp = {
 	.init		= htcp_init,
+	.release	= htcp_release,
 	.ssthresh	= htcp_recalc_ssthresh,
 	.cong_avoid	= htcp_cong_avoid,
 	.set_state	= htcp_state,

net/ipv4/tcp_hybla.c +7 -1

diff -ruN a/net/ipv4/tcp_hybla.c b/net/ipv4/tcp_hybla.c
@@ -42,10 +42,15 @@
 	ca->rho2 = ca->rho2_7ls >>7;
 }
 
+static void hybla_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void hybla_init(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct hybla *ca = inet_csk_ca(sk);
+	struct hybla *ca = tcp_register_ca_priv(sk, sizeof(struct hybla));
 
 	ca->rho = 0;
 	ca->rho2 = 0;

@@ -160,6 +165,7 @@
 
 static struct tcp_congestion_ops tcp_hybla = {
 	.init		= hybla_init,
+	.release	= hybla_release,
 	.ssthresh	= tcp_reno_ssthresh,
 	.min_cwnd	= tcp_reno_min_cwnd,
 	.cong_avoid	= hybla_cong_avoid,

net/ipv4/tcp_illinois.c +7 -1

diff -ruN a/net/ipv4/tcp_illinois.c b/net/ipv4/tcp_illinois.c
@@ -66,9 +66,14 @@
 	/* TODO: age max_rtt? */
 }
 
+static void tcp_illinois_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void tcp_illinois_init(struct sock *sk)
 {
-	struct illinois *ca = inet_csk_ca(sk);
+	struct illinois *ca = tcp_register_ca_priv(sk, sizeof(struct illinois));
 
 	ca->alpha = ALPHA_MAX;
 	ca->beta = BETA_BASE;

@@ -325,6 +330,7 @@
 static struct tcp_congestion_ops tcp_illinois = {
 	.flags		= TCP_CONG_RTT_STAMP,
 	.init		= tcp_illinois_init,
+	.release	= tcp_illinois_release,
 	.ssthresh	= tcp_illinois_ssthresh,
 	.min_cwnd	= tcp_reno_min_cwnd,
 	.cong_avoid	= tcp_illinois_cong_avoid,

net/ipv4/tcp_lp.c +7 -1

diff -ruN a/net/ipv4/tcp_lp.c b/net/ipv4/tcp_lp.c
@@ -86,6 +86,11 @@
 	u32 inference;
 };
 
+static void tcp_lp_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 /**
  * tcp_lp_init
  *

@@ -94,7 +99,7 @@
  */
 static void tcp_lp_init(struct sock *sk)
 {
-	struct lp *lp = inet_csk_ca(sk);
+	struct lp *lp = tcp_register_ca_priv(sk, sizeof(struct lp));
 
 	lp->flag = 0;
 	lp->sowd = 0;

@@ -316,6 +321,7 @@
 static struct tcp_congestion_ops tcp_lp = {
 	.flags = TCP_CONG_RTT_STAMP,
 	.init = tcp_lp_init,
+	.release = tcp_lp_release,
 	.ssthresh = tcp_reno_ssthresh,
 	.cong_avoid = tcp_lp_cong_avoid,
 	.min_cwnd = tcp_reno_min_cwnd,

net/ipv4/tcp_vegas.c +7 -1

diff -ruN a/net/ipv4/tcp_vegas.c b/net/ipv4/tcp_vegas.c
@@ -95,9 +95,14 @@
 	vegas->doing_vegas_now = 0;
 }
 
+void tcp_vegas_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 void tcp_vegas_init(struct sock *sk)
 {
-	struct vegas *vegas = inet_csk_ca(sk);
+	struct vegas *vegas = tcp_register_ca_priv(sk, sizeof(struct vegas));
 
 	vegas->baseRTT = 0x7fffffff;
 	vegas_enable(sk);

@@ -358,6 +363,7 @@
 static struct tcp_congestion_ops tcp_vegas = {
 	.flags		= TCP_CONG_RTT_STAMP,
 	.init		= tcp_vegas_init,
+	.release	= tcp_vegas_release,	
 	.ssthresh	= tcp_reno_ssthresh,
 	.cong_avoid	= tcp_vegas_cong_avoid,
 	.min_cwnd	= tcp_reno_min_cwnd,

net/ipv4/tcp_veno.c +7 -1

diff -ruN a/net/ipv4/tcp_veno.c b/net/ipv4/tcp_veno.c
@@ -59,9 +59,14 @@
 	veno->doing_veno_now = 0;
 }
 
+static void tcp_veno_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void tcp_veno_init(struct sock *sk)
 {
-	struct veno *veno = inet_csk_ca(sk);
+	struct veno *veno = tcp_register_ca_priv(sk, sizeof(struct veno));
 
 	veno->basertt = 0x7fffffff;
 	veno->inc = 1;

@@ -210,6 +215,7 @@
 static struct tcp_congestion_ops tcp_veno = {
 	.flags		= TCP_CONG_RTT_STAMP,
 	.init		= tcp_veno_init,
+	.release	= tcp_veno_release,
 	.ssthresh	= tcp_veno_ssthresh,
 	.cong_avoid	= tcp_veno_cong_avoid,
 	.pkts_acked	= tcp_veno_pkts_acked,

net/ipv4/tcp_westwood.c +7 -1

diff -ruN a/net/ipv4/tcp_westwood.c b/net/ipv4/tcp_westwood.c
@@ -47,6 +47,11 @@
 #define TCP_WESTWOOD_RTT_MIN   (HZ/20)	/* 50ms */
 #define TCP_WESTWOOD_INIT_RTT  (20*HZ)	/* maybe too conservative?! */
 
+static void tcp_westwood_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 /*
  * @tcp_westwood_create
  * This function initializes fields used in TCP Westwood+,

@@ -60,7 +65,7 @@
  */
 static void tcp_westwood_init(struct sock *sk)
 {
-	struct westwood *w = inet_csk_ca(sk);
+	struct westwood *w = tcp_register_ca_priv(sk, sizeof(struct westwood));
 
 	w->bk = 0;
 	w->bw_ns_est = 0;

@@ -274,6 +279,7 @@
 
 static struct tcp_congestion_ops tcp_westwood = {
 	.init		= tcp_westwood_init,
+	.release	= tcp_westwood_release,
 	.ssthresh	= tcp_reno_ssthresh,
 	.cong_avoid	= tcp_reno_cong_avoid,
 	.min_cwnd	= tcp_westwood_bw_rttmin,

net/ipv4/tcp_yeah.c +7 -1

diff -ruN a/net/ipv4/tcp_yeah.c b/net/ipv4/tcp_yeah.c
@@ -39,10 +39,15 @@
 	u32 pkts_acked;
 };
 
+static void tcp_yeah_release(struct sock *sk)
+{
+	tcp_release_ca_priv(sk);
+}
+
 static void tcp_yeah_init(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct yeah *yeah = inet_csk_ca(sk);
+	struct yeah *yeah = tcp_register_ca_priv(sk, sizeof(struct yeah));
 
 	tcp_vegas_init(sk);

@@ -235,6 +240,7 @@
 static struct tcp_congestion_ops tcp_yeah = {
 	.flags		= TCP_CONG_RTT_STAMP,
 	.init		= tcp_yeah_init,
+	.release	= tcp_yeah_release,
 	.ssthresh	= tcp_yeah_ssthresh,
 	.cong_avoid	= tcp_yeah_cong_avoid,
 	.min_cwnd	= tcp_reno_min_cwnd,

Evgeniy Polyakov

2008-08-31 19:09 UTC #1 lore raw

Hi.

Quoted 8 lines · Daniele Lacamera

This simple patch (2.6.26) replaces the icsk_ca_priv big array with a void pointer, reducing the default inet_connection_sock size by 60Bytes.
Each "congestion controlled" socket must allocate and free its own priv structure if needed, from the connection init and release callbacks inside each module.
Wouldn't this reduce memory usage for sockets that don't use ca?

This will also increase number of allocations in the fast path, isn't it? This overhead is not acceptible.

Signature · 1 line

	Evgeniy Polyakov

Daniele Lacamera

author

2008-08-31 19:19 UTC #2 lore raw

Quoted 6 lines · Evgeniy Polyakov

Hi.
On Sun, Aug 31, 2008 at 08:26:50PM +0200, Daniele Lacamera ([email protected]) wrote:
This will also increase number of allocations in the fast path, isn't
it? This overhead is not acceptible.

Hi Evgeniy,

fast path? I don't think so... the modules allocate the ca_priv once per connection, at the beginning of the connection and only if that specific module needs that. Am I missing something?

Signature · 1 line

Daniele

Evgeniy Polyakov

2008-08-31 20:23 UTC #3 lore raw

Hi Daniele.

Quoted 4 lines · Daniele Lacamera

fast path? I don't think so... the modules allocate the ca_priv once per connection, at the beginning of the connection and only if that specific module needs that.
Am I missing something?

That's actually a very fast path: connection establishment both for listened and connection sockets (tcp_init_congestion_control() is called for TCP_SYN_RECV state too), so you forces every socket allocation (if appropriate congestion contttrol requires that) to be doubled.

This actually may be not a big problem, since we allocate and free objects all the time, but still it is additional overhead which can be avoided. Did you run some perfromance analysis to check if influence is small or not visible at all?

Signature · 1 line

	Evgeniy Polyakov

Daniele Lacamera

author

2008-08-31 21:35 UTC #4 lore raw

Quoted 7 lines · Evgeniy Polyakov

Hi Daniele.
This actually may be not a big problem, since we allocate and free objects all the time, but still it is additional overhead which can be avoided. Did you run some perfromance analysis to check if influence is small or not visible at all?

Evgeniy, thanks for pointing out the issue.

On my virtual machine (kvm), the alloc operation always takes less than one jiffie, but I can't say exactly how long. Is there a way to measure fast path time more accurately (e.g. nanoseconds)?

David Miller

maintainer

2008-09-01 04:05 UTC #5 lore raw

In reply to Daniele Lacamera

From: Daniele Lacamera <[email protected]>
Date: Sun, 31 Aug 2008 21:19:38 +0200

Quoted 3 lines

fast path? I don't think so... the modules allocate the ca_priv once per connection, at the beginning of the connection and only if that specific module needs that. Am I missing something?

Creating and destroying a connection is a fast path, that is what you miss.

The performance of many workloads is directly dependent upon how fast we can create and tear down a TCP connection.

David Miller

maintainer

2008-09-01 04:03 UTC #6 lore raw

In reply to Daniele Lacamera

From: Daniele Lacamera <[email protected]>
Date: Sun, 31 Aug 2008 20:26:50 +0200

Quoted 8 lines

This simple patch (2.6.26) replaces the icsk_ca_priv big array with a void pointer, reducing the default inet_connection_sock size by 60Bytes.
Each "congestion controlled" socket must allocate and free its own priv structure if needed, from the connection init and release callbacks inside each module.
Wouldn't this reduce memory usage for sockets that don't use ca?

Yes, but for every TCP socket there is a new memory allocation and free.

We're trying to decrease the number of those rather than increase them because having more of them non-trivially impacts TCP connection rates.