Patchwork Major network performance regression in 3.7

login
register
mail settings
Submitter Eric Dumazet
Date Jan. 6, 2013, 5:10 p.m.
Message ID <1357492255.6919.336.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/209786/
State RFC
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Jan. 6, 2013, 5:10 p.m.
On Sun, 2013-01-06 at 17:44 +0100, Willy Tarreau wrote:
> On Sun, Jan 06, 2013 at 08:39:53AM -0800, Eric Dumazet wrote:
> > Hmm, I'll have to check if this really can be reverted without hurting
> > vmsplice() again.
> 
> Looking at the code I've been wondering whether we shouldn't transform
> the condition to perform the push if we can't push more segments, but
> I don't know what to rely on. It would be something like this :
> 
>        if (copied &&
>           (!(flags & MSG_SENDPAGE_NOTLAST) || cant_push_more))
>                 tcp_push(sk, flags, mss_now, tp->nonagle);

Good point !

Maybe the following fix then ?




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
willy tarreau - Jan. 6, 2013, 5:35 p.m.
On Sun, Jan 06, 2013 at 09:10:55AM -0800, Eric Dumazet wrote:
> On Sun, 2013-01-06 at 17:44 +0100, Willy Tarreau wrote:
> > On Sun, Jan 06, 2013 at 08:39:53AM -0800, Eric Dumazet wrote:
> > > Hmm, I'll have to check if this really can be reverted without hurting
> > > vmsplice() again.
> > 
> > Looking at the code I've been wondering whether we shouldn't transform
> > the condition to perform the push if we can't push more segments, but
> > I don't know what to rely on. It would be something like this :
> > 
> >        if (copied &&
> >           (!(flags & MSG_SENDPAGE_NOTLAST) || cant_push_more))
> >                 tcp_push(sk, flags, mss_now, tp->nonagle);
> 
> Good point !
> 
> Maybe the following fix then ?
> 
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1ca2536..7ba0717 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -941,8 +941,10 @@ out:
>  	return copied;
>  
>  do_error:
> -	if (copied)
> +	if (copied) {
> +		flags &= ~MSG_SENDPAGE_NOTLAST;
>  		goto out;
> +	}
>  out_err:
>  	return sk_stream_error(sk, flags, err);
>  }

Unfortunately it does not work any better, which means to me
that we don't leave via this code path. I tried other tricks
which failed too. I need to understand this part better before
randomly fiddling with it.

Willy

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Jan. 6, 2013, 6:39 p.m.
On Sun, 2013-01-06 at 18:35 +0100, Willy Tarreau wrote:

> Unfortunately it does not work any better, which means to me
> that we don't leave via this code path. I tried other tricks
> which failed too. I need to understand this part better before
> randomly fiddling with it.
> 

OK, now I have your test program, I can work on a fix, dont worry ;)

The MSG_SENDPAGE_NOTLAST logic needs to be tweaked.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Jan. 6, 2013, 6:43 p.m.
On Sun, 2013-01-06 at 10:39 -0800, Eric Dumazet wrote:
> On Sun, 2013-01-06 at 18:35 +0100, Willy Tarreau wrote:
> 
> > Unfortunately it does not work any better, which means to me
> > that we don't leave via this code path. I tried other tricks
> > which failed too. I need to understand this part better before
> > randomly fiddling with it.
> > 
> 
> OK, now I have your test program, I can work on a fix, dont worry ;)
> 
> The MSG_SENDPAGE_NOTLAST logic needs to be tweaked.
> 


(sd->len is usually 4096, which is expected, but sd->total_len value is
huge in your case, so we always set the flag in fs/splice.c)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Jan. 6, 2013, 6:51 p.m.
> 
> (sd->len is usually 4096, which is expected, but sd->total_len value is
> huge in your case, so we always set the flag in fs/splice.c)

I am testing :

       if (sd->len < sd->total_len && pipe->nrbufs > 1)
                more |= MSG_SENDPAGE_NOTLAST;



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1ca2536..7ba0717 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -941,8 +941,10 @@  out:
 	return copied;
 
 do_error:
-	if (copied)
+	if (copied) {
+		flags &= ~MSG_SENDPAGE_NOTLAST;
 		goto out;
+	}
 out_err:
 	return sk_stream_error(sk, flags, err);
 }