Patchwork Major network performance regression in 3.7

login
register
mail settings
Submitter Eric Dumazet
Date Jan. 6, 2013, 2:16 a.m.
Message ID <1357438591.1678.5205.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/209712/
State RFC
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Jan. 6, 2013, 2:16 a.m.
On Sat, 2013-01-05 at 17:51 -0800, Eric Dumazet wrote:
> On Sat, 2013-01-05 at 17:40 -0800, Eric Dumazet wrote:
> > On Sun, 2013-01-06 at 02:30 +0100, Willy Tarreau wrote:
> > 
> > > Ah interesting because these were some of the mm patches that I had
> > > tried to revert.
> > 
> > Hmm, or we should fix __skb_splice_bits()
> > 
> > I'll send a patch.
> > 
> 
> Could you try the following ?

Or more exactly...



--
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, 2:18 a.m.
On Sat, Jan 05, 2013 at 06:16:31PM -0800, Eric Dumazet wrote:
> On Sat, 2013-01-05 at 17:51 -0800, Eric Dumazet wrote:
> > On Sat, 2013-01-05 at 17:40 -0800, Eric Dumazet wrote:
> > > On Sun, 2013-01-06 at 02:30 +0100, Willy Tarreau wrote:
> > > 
> > > > Ah interesting because these were some of the mm patches that I had
> > > > tried to revert.
> > > 
> > > Hmm, or we should fix __skb_splice_bits()
> > > 
> > > I'll send a patch.
> > > 
> > 
> > Could you try the following ?
> 
> Or more exactly...

The first one did not change a iota unfortunately. I'm about to
spot the commit causing the loopback regression. It's a few patches
before the first one you pointed. It's almost finished and I test
your patch below immediately after.

Thanks,
Willy

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 3ab989b..01f222c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1736,11 +1736,8 @@ static bool __splice_segment(struct page *page, unsigned int poff,
>  		return false;
>  	}
>  
> -	/* ignore any bits we already processed */
> -	if (*off) {
> -		__segment_seek(&page, &poff, &plen, *off);
> -		*off = 0;
> -	}
> +	__segment_seek(&page, &poff, &plen, *off);
> +	*off = 0;
>  
>  	do {
>  		unsigned int flen = min(*len, plen);
> @@ -1768,14 +1765,15 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
>  			      struct splice_pipe_desc *spd, struct sock *sk)
>  {
>  	int seg;
> +	struct page *page = virt_to_page(skb->data);
> +	unsigned int poff = skb->data - (unsigned char *)page_address(page);
>  
>  	/* map the linear part :
>  	 * If skb->head_frag is set, this 'linear' part is backed by a
>  	 * fragment, and if the head is not shared with any clones then
>  	 * we can avoid a copy since we own the head portion of this page.
>  	 */
> -	if (__splice_segment(virt_to_page(skb->data),
> -			     (unsigned long) skb->data & (PAGE_SIZE - 1),
> +	if (__splice_segment(page, poff,
>  			     skb_headlen(skb),
>  			     offset, len, skb, spd,
>  			     skb_head_is_locked(skb),
> 
--
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, 2:22 a.m.
On Sun, 2013-01-06 at 03:18 +0100, Willy Tarreau wrote:
> On Sat, Jan 05, 2013 at 06:16:31PM -0800, Eric Dumazet wrote:
> > On Sat, 2013-01-05 at 17:51 -0800, Eric Dumazet wrote:
> > > On Sat, 2013-01-05 at 17:40 -0800, Eric Dumazet wrote:
> > > > On Sun, 2013-01-06 at 02:30 +0100, Willy Tarreau wrote:
> > > > 
> > > > > Ah interesting because these were some of the mm patches that I had
> > > > > tried to revert.
> > > > 
> > > > Hmm, or we should fix __skb_splice_bits()
> > > > 
> > > > I'll send a patch.
> > > > 
> > > 
> > > Could you try the following ?
> > 
> > Or more exactly...
> 
> The first one did not change a iota unfortunately. I'm about to
> spot the commit causing the loopback regression. It's a few patches
> before the first one you pointed. It's almost finished and I test
> your patch below immediately after.

I bet you are going to find commit
69b08f62e17439ee3d436faf0b9a7ca6fffb78db
(net: use bigger pages in __netdev_alloc_frag )

Am I wrong ?


--
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, 2:32 a.m.
On Sat, Jan 05, 2013 at 06:22:13PM -0800, Eric Dumazet wrote:
> On Sun, 2013-01-06 at 03:18 +0100, Willy Tarreau wrote:
> > On Sat, Jan 05, 2013 at 06:16:31PM -0800, Eric Dumazet wrote:
> > > On Sat, 2013-01-05 at 17:51 -0800, Eric Dumazet wrote:
> > > > On Sat, 2013-01-05 at 17:40 -0800, Eric Dumazet wrote:
> > > > > On Sun, 2013-01-06 at 02:30 +0100, Willy Tarreau wrote:
> > > > > 
> > > > > > Ah interesting because these were some of the mm patches that I had
> > > > > > tried to revert.
> > > > > 
> > > > > Hmm, or we should fix __skb_splice_bits()
> > > > > 
> > > > > I'll send a patch.
> > > > > 
> > > > 
> > > > Could you try the following ?
> > > 
> > > Or more exactly...
> > 
> > The first one did not change a iota unfortunately. I'm about to
> > spot the commit causing the loopback regression. It's a few patches
> > before the first one you pointed. It's almost finished and I test
> > your patch below immediately after.
> 
> I bet you are going to find commit
> 69b08f62e17439ee3d436faf0b9a7ca6fffb78db
> (net: use bigger pages in __netdev_alloc_frag )
> 
> Am I wrong ?

Yes this time you guessed wrong :-) Well maybe it's participating
to the issue.

It's 0cf833ae (net: loopback: set default mtu to 64K). And I could
reproduce it with 3.6 by setting loopback's MTU to 65536 by hand.
The trick is that once the MTU has been set to this large a value,
even when I set it back to 16kB the problem persists.

Now I'm retrying your other patch to see if it brings the 10GE back
to full speed.

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, 2:44 a.m.
On Sun, 2013-01-06 at 03:32 +0100, Willy Tarreau wrote:

> It's 0cf833ae (net: loopback: set default mtu to 64K). And I could
> reproduce it with 3.6 by setting loopback's MTU to 65536 by hand.
> The trick is that once the MTU has been set to this large a value,
> even when I set it back to 16kB the problem persists.
> 

Well, this MTU change can uncover a prior bug, or make it happen faster,
for sure.



--
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, 2:52 a.m.
On Sat, Jan 05, 2013 at 06:16:31PM -0800, Eric Dumazet wrote:
> On Sat, 2013-01-05 at 17:51 -0800, Eric Dumazet wrote:
> > On Sat, 2013-01-05 at 17:40 -0800, Eric Dumazet wrote:
> > > On Sun, 2013-01-06 at 02:30 +0100, Willy Tarreau wrote:
> > > 
> > > > Ah interesting because these were some of the mm patches that I had
> > > > tried to revert.
> > > 
> > > Hmm, or we should fix __skb_splice_bits()
> > > 
> > > I'll send a patch.
> > > 
> > 
> > Could you try the following ?
> 
> Or more exactly...
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 3ab989b..01f222c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1736,11 +1736,8 @@ static bool __splice_segment(struct page *page, unsigned int poff,
>  		return false;
>  	}
>  
> -	/* ignore any bits we already processed */
> -	if (*off) {
> -		__segment_seek(&page, &poff, &plen, *off);
> -		*off = 0;
> -	}
> +	__segment_seek(&page, &poff, &plen, *off);
> +	*off = 0;
>  
>  	do {
>  		unsigned int flen = min(*len, plen);
> @@ -1768,14 +1765,15 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
>  			      struct splice_pipe_desc *spd, struct sock *sk)
>  {
>  	int seg;
> +	struct page *page = virt_to_page(skb->data);
> +	unsigned int poff = skb->data - (unsigned char *)page_address(page);
>  
>  	/* map the linear part :
>  	 * If skb->head_frag is set, this 'linear' part is backed by a
>  	 * fragment, and if the head is not shared with any clones then
>  	 * we can avoid a copy since we own the head portion of this page.
>  	 */
> -	if (__splice_segment(virt_to_page(skb->data),
> -			     (unsigned long) skb->data & (PAGE_SIZE - 1),
> +	if (__splice_segment(page, poff,
>  			     skb_headlen(skb),
>  			     offset, len, skb, spd,
>  			     skb_head_is_locked(skb),
> 

OK so I observed no change with this patch, either on the loopback
data rate at >16kB MTU, or on the myri. I'm keeping it at hand for
experimentation anyway.

Concerning the loopback MTU, I find it strange that the MTU changes
the splice() behaviour and not send/recv. I thought that there could
be a relation between the MTU and the pipe size, but it does not
appear to be the case either, as I tried various sizes between 16kB
and 256kB without achieving original performance.

I've started to bisect the 10GE issue again (since both issues are
unrelated), but I'll finish tomorrow, it's time to get some sleep
now.

Best regards,
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

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3ab989b..01f222c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1736,11 +1736,8 @@  static bool __splice_segment(struct page *page, unsigned int poff,
 		return false;
 	}
 
-	/* ignore any bits we already processed */
-	if (*off) {
-		__segment_seek(&page, &poff, &plen, *off);
-		*off = 0;
-	}
+	__segment_seek(&page, &poff, &plen, *off);
+	*off = 0;
 
 	do {
 		unsigned int flen = min(*len, plen);
@@ -1768,14 +1765,15 @@  static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 			      struct splice_pipe_desc *spd, struct sock *sk)
 {
 	int seg;
+	struct page *page = virt_to_page(skb->data);
+	unsigned int poff = skb->data - (unsigned char *)page_address(page);
 
 	/* map the linear part :
 	 * If skb->head_frag is set, this 'linear' part is backed by a
 	 * fragment, and if the head is not shared with any clones then
 	 * we can avoid a copy since we own the head portion of this page.
 	 */
-	if (__splice_segment(virt_to_page(skb->data),
-			     (unsigned long) skb->data & (PAGE_SIZE - 1),
+	if (__splice_segment(page, poff,
 			     skb_headlen(skb),
 			     offset, len, skb, spd,
 			     skb_head_is_locked(skb),