| Message ID | 1357438591.1678.5205.camel@edumazet-glaptop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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),