[PATCH] [RFC] tcp: TSQ - do not always throttle.

[PATCH] [RFC] tcp: TSQ - do not always throttle.

[Krishna Kumar]

Do not throttle if sysctl_tcp_limit_output_bytes==0.

Maybe it is better to throttle earlier in the loop, after
calling tcp_init_tso_segs().

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
 tcp_output.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -ruNp org/net/ipv4/tcp_output.c new/net/ipv4/tcp_output.c
--- org/net/ipv4/tcp_output.c	2012-07-17 09:56:12.000000000 +0530
+++ new/net/ipv4/tcp_output.c	2012-07-17 13:02:12.476111697 +0530
@@ -1948,7 +1948,8 @@ static bool tcp_write_xmit(struct sock *
 		/* TSQ : sk_wmem_alloc accounts skb truesize,
 		 * including skb overhead. But thats OK.
 		 */
-		if (atomic_read(&sk->sk_wmem_alloc) >= sysctl_tcp_limit_output_bytes) {
+		if (sysctl_tcp_limit_output_bytes > 0 &&
+		    atomic_read(&sk->sk_wmem_alloc) >= sysctl_tcp_limit_output_bytes) {
 			set_bit(TSQ_THROTTLED, &tp->tsq_flags);
 			break;
 		}

Re: [PATCH] [RFC] tcp: TSQ - do not always throttle.

[Eric Dumazet]

On Tue, 2012-07-17 at 17:33 +0530, Krishna Kumar wrote:
> Do not throttle if sysctl_tcp_limit_output_bytes==0.
> 
> Maybe it is better to throttle earlier in the loop, after
> calling tcp_init_tso_segs().
> 

I wonder why, and why you put this question in a changelog instead of
outside of it...

Idea was to avoid setting TSQ_THROTTLED if we break out the loop.


About disabling TSQ, my initial intent was to instead use a negative
sysctl_tcp_limit_output_bytes value.

Thats why I have in tcp_transmit_skb() :

skb->destructor = (sysctl_tcp_limit_output_bytes > 0) ?
		  tcp_wfree : sock_wfree;

So I suggest you change the tcp_write_xmit(() test to a single unsigned
compare :

if (atomic_read(&sk->sk_wmem_alloc) >=
    (unsigned) sysctl_tcp_limit_output_bytes) {

Also use :

skb->destructor = (sysctl_tcp_limit_output_bytes >= 0) ?
  tcp_wfree : sock_wfree;

and document the 'negative value disables TSQ' in
Documentation/networking/ip-sysctl.txt

Re: [PATCH] [RFC] tcp: TSQ - do not always throttle.

[Krishna Kumar2]

Eric Dumazet <eric.dumazet@gmail.com> wrote on 07/17/2012 06:40:54 PM:

> > Do not throttle if sysctl_tcp_limit_output_bytes==0.
> >
> > Maybe it is better to throttle earlier in the loop, after
> > calling tcp_init_tso_segs().
> >
>
> I wonder why, and why you put this question in a changelog instead of
> outside of it...
>
> Idea was to avoid setting TSQ_THROTTLED if we break out the loop.

The reason I mentioned it (in the wrong place) is because
I thought this is a likely case and the checks before that
might all pass only to get throttled. Some of the checks
are quite lengthy.

> About disabling TSQ, my initial intent was to instead use a negative
> sysctl_tcp_limit_output_bytes value.
>
> Thats why I have in tcp_transmit_skb() :
>
> skb->destructor = (sysctl_tcp_limit_output_bytes > 0) ?
>         tcp_wfree : sock_wfree;
>
> So I suggest you change the tcp_write_xmit(() test to a single unsigned
> compare :
>
> if (atomic_read(&sk->sk_wmem_alloc) >=
>     (unsigned) sysctl_tcp_limit_output_bytes) {
>
> Also use :
>
> skb->destructor = (sysctl_tcp_limit_output_bytes >= 0) ?
>   tcp_wfree : sock_wfree;
>
> and document the 'negative value disables TSQ' in
> Documentation/networking/ip-sysctl.txt

Sure, will post with this change.

thanks,
- KK