[PATCH net-next] tcp: tcp_sendmsg() page recycling

[PATCH net-next] tcp: tcp_sendmsg() page recycling

[Eric Dumazet]

If our TCP_PAGE(sk) is not shared (page_count() == 1), we can set page
offset to 0.

This permits better filling of the pages on small to medium tcp writes.

"tbench 16" results on my dev server (2x4x2 machine) :

Before : 3072 MB/s
After  : 3146 MB/s  (2.4 % gain)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/tcp.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 45156be..a09fe25 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1009,7 +1009,12 @@ new_segment:
 				int merge = 0;
 				int i = skb_shinfo(skb)->nr_frags;
 				struct page *page = TCP_PAGE(sk);
-				int off = TCP_OFF(sk);
+				int off;
+
+				if (page && page_count(page) == 1)
+					TCP_OFF(sk) = 0;
+
+				off = TCP_OFF(sk);
 
 				if (skb_can_coalesce(skb, i, page, off) &&
 				    off != PAGE_SIZE) {

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[Joe Perches]

On Sun, 2011-12-04 at 18:05 +0100, Eric Dumazet wrote:
> If our TCP_PAGE(sk) is not shared (page_count() == 1), we can set page
> offset to 0.
[]
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[]
> @@ -1009,7 +1009,12 @@ new_segment:
>  				int merge = 0;
>  				int i = skb_shinfo(skb)->nr_frags;
>  				struct page *page = TCP_PAGE(sk);
> -				int off = TCP_OFF(sk);
> +				int off;
> +
> +				if (page && page_count(page) == 1)
> +					TCP_OFF(sk) = 0;
> +
> +				off = TCP_OFF(sk);

This might be clearer and take one less indirection as

				if (page && page_count(page) == 1) {
					TCP_OFF(sk) = 0;
					off = 0;
				} else {
					off = 0;
				}

or maybe

				if (page && page_count(page) == 1)
					off = TCP_OFF(sk) = 0;
				else
					off = 0;

And maybe the TCP_OFF and TCP_PAGE macros should be removed.

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[Nicolas de Pesloüan]

Le 04/12/2011 18:20, Joe Perches a écrit :
> On Sun, 2011-12-04 at 18:05 +0100, Eric Dumazet wrote:
>> If our TCP_PAGE(sk) is not shared (page_count() == 1), we can set page
>> offset to 0.
> []
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> []
>> @@ -1009,7 +1009,12 @@ new_segment:
>>   				int merge = 0;
>>   				int i = skb_shinfo(skb)->nr_frags;
>>   				struct page *page = TCP_PAGE(sk);
>> -				int off = TCP_OFF(sk);
>> +				int off;
>> +
>> +				if (page&&  page_count(page) == 1)
>> +					TCP_OFF(sk) = 0;
>> +
>> +				off = TCP_OFF(sk);
>
> This might be clearer and take one less indirection as
>
> 				if (page&&  page_count(page) == 1) {
> 					TCP_OFF(sk) = 0;
> 					off = 0;
> 				} else {
> 					off = 0;
> 				}
>
> or maybe
>
> 				if (page&&  page_count(page) == 1)
> 					off = TCP_OFF(sk) = 0;
> 				else
> 					off = 0;

Well, those two fragments don't look equivalent to the original one, unless TCP_OFF(sk) happens to 
be 0 before the test. But if this is true, then the whole thing seems useless. Do I miss something?

For as far as I understand, the following should be equivalent to Eric's original, but I don't 
consider it better than the original.

  				if (page&&  page_count(page) == 1)
  					off = TCP_OFF(sk) = 0;
  				else
  					off = TCP_OFF(sk);

	Nicolas.

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[Joe Perches]

On Sun, 2011-12-04 at 19:27 +0100, Nicolas de Pesloüan wrote:
> Well, those two fragments don't look equivalent to the original one, unless TCP_OFF(sk) happens to 
> be 0 before the test. But if this is true, then the whole thing seems useless. Do I miss something?

Nope, you're completely correct.  It's just me not typing correctly.

[PATCH net-next] tcp: remove TCP_OFF and TCP_PAGE macros

[Eric Dumazet]

Le dimanche 04 décembre 2011 à 09:20 -0800, Joe Perches a écrit :
> And maybe the TCP_OFF and TCP_PAGE macros should be removed.
> 

Agreed, thanks.

[PATCH net-next] tcp: remove TCP_OFF and TCP_PAGE macros

As mentioned by Joe Perches, TCP_OFF() and TCP_PAGE() macros are
useless.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/tcp.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index a09fe25..43dfccc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -888,9 +888,6 @@ int tcp_sendpage(struct sock *sk, struct page *page, int offset,
 }
 EXPORT_SYMBOL(tcp_sendpage);
 
-#define TCP_PAGE(sk)	(sk->sk_sndmsg_page)
-#define TCP_OFF(sk)	(sk->sk_sndmsg_off)
-
 static inline int select_size(const struct sock *sk, bool sg)
 {
 	const struct tcp_sock *tp = tcp_sk(sk);
@@ -1008,13 +1005,13 @@ new_segment:
 			} else {
 				int merge = 0;
 				int i = skb_shinfo(skb)->nr_frags;
-				struct page *page = TCP_PAGE(sk);
+				struct page *page = sk->sk_sndmsg_page;
 				int off;
 
 				if (page && page_count(page) == 1)
-					TCP_OFF(sk) = 0;
+					sk->sk_sndmsg_off = 0;
 
-				off = TCP_OFF(sk);
+				off = sk->sk_sndmsg_off;
 
 				if (skb_can_coalesce(skb, i, page, off) &&
 				    off != PAGE_SIZE) {
@@ -1031,7 +1028,7 @@ new_segment:
 				} else if (page) {
 					if (off == PAGE_SIZE) {
 						put_page(page);
-						TCP_PAGE(sk) = page = NULL;
+						sk->sk_sndmsg_page = page = NULL;
 						off = 0;
 					}
 				} else
@@ -1057,9 +1054,9 @@ new_segment:
 					/* If this page was new, give it to the
 					 * socket so it does not get leaked.
 					 */
-					if (!TCP_PAGE(sk)) {
-						TCP_PAGE(sk) = page;
-						TCP_OFF(sk) = 0;
+					if (!sk->sk_sndmsg_page) {
+						sk->sk_sndmsg_page = page;
+						sk->sk_sndmsg_off = 0;
 					}
 					goto do_error;
 				}
@@ -1069,15 +1066,15 @@ new_segment:
 					skb_frag_size_add(&skb_shinfo(skb)->frags[i - 1], copy);
 				} else {
 					skb_fill_page_desc(skb, i, page, off, copy);
-					if (TCP_PAGE(sk)) {
+					if (sk->sk_sndmsg_page) {
 						get_page(page);
 					} else if (off + copy < PAGE_SIZE) {
 						get_page(page);
-						TCP_PAGE(sk) = page;
+						sk->sk_sndmsg_page = page;
 					}
 				}
 
-				TCP_OFF(sk) = off + copy;
+				sk->sk_sndmsg_off = off + copy;
 			}
 
 			if (!copied)

Re: [PATCH net-next] tcp: remove TCP_OFF and TCP_PAGE macros

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 05 Dec 2011 12:07:15 +0100

> Le dimanche 04 décembre 2011 à 09:20 -0800, Joe Perches a écrit :
>> And maybe the TCP_OFF and TCP_PAGE macros should be removed.
>> 
> 
> Agreed, thanks.
> 
> [PATCH net-next] tcp: remove TCP_OFF and TCP_PAGE macros
> 
> As mentioned by Joe Perches, TCP_OFF() and TCP_PAGE() macros are
> useless.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[Eric Dumazet]

Le dimanche 04 décembre 2011 à 18:05 +0100, Eric Dumazet a écrit :
> If our TCP_PAGE(sk) is not shared (page_count() == 1), we can set page
> offset to 0.
> 
> This permits better filling of the pages on small to medium tcp writes.
> 
> "tbench 16" results on my dev server (2x4x2 machine) :
> 
> Before : 3072 MB/s
> After  : 3146 MB/s  (2.4 % gain)

By the way current linux tree on the same machine gets 2957 MB/s

So the combination of select_size() patch and this one gives
a 6.4 % gain.

Definitely worth the pain, isnt it ?

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 04 Dec 2011 18:36:52 +0100

> Definitely worth the pain, isnt it ?

As long as such gains aren't lost in the noise when talking over
real devices.

I'm not a big fan of tbench over loopback numbers, frankly :-)

Re: [PATCH net-next] tcp: tcp_sendmsg() page recycling

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 04 Dec 2011 18:05:17 +0100

> If our TCP_PAGE(sk) is not shared (page_count() == 1), we can set page
> offset to 0.
> 
> This permits better filling of the pages on small to medium tcp writes.
> 
> "tbench 16" results on my dev server (2x4x2 machine) :
> 
> Before : 3072 MB/s
> After  : 3146 MB/s  (2.4 % gain)
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.