From: Ian Campbell <hidden> Date: 2012-10-09 13:47:50
Hi Eric,
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Are all net drivers expected to be able to handle compound pages in the
frags? Obviously it is to their benefit to do so, so it is something
I'll want to look into for netback.
I expect the main factor here is bridging/forwarding, since the
receiving NIC and its driver appear to support compound pages but the
outgoing NIC (netback in this case) does not.
I guess my question is should I be rushing to fix netback ASAP or should
I rather be looking for a bug somewhere which caused a frag of this type
to get as far as netback's start_xmit in the first place?
Or am I just barking up the wrong tree to start with?
Thanks,
Ian.
From: Eric Dumazet <hidden> Date: 2012-10-09 13:54:28
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
Hi Eric,
Hi Ian
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
Are all net drivers expected to be able to handle compound pages in the
frags? Obviously it is to their benefit to do so, so it is something
I'll want to look into for netback.
Not sure why a net driver would care of COMPOUND page at all ?
a Fragment has a struct page *, and a size.
a page can be order-0, order-1, order-2, order-3, ...
I expect the main factor here is bridging/forwarding, since the
receiving NIC and its driver appear to support compound pages but the
outgoing NIC (netback in this case) does not.
I guess my question is should I be rushing to fix netback ASAP or should
I rather be looking for a bug somewhere which caused a frag of this type
to get as far as netback's start_xmit in the first place?
Or am I just barking up the wrong tree to start with?
From: Eric Dumazet <hidden> Date: 2012-10-09 14:02:52
On Tue, 2012-10-09 at 15:54 +0200, Eric Dumazet wrote:
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
quoted
Hi Eric,
Hi Ian
quoted
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
Hmm, I take it back. It also can give you the same problem :
We use this allocator for rx path of drivers :
__netdev_alloc_skb()
So its now absolutely possible that one skb->head is backed by a order-3
page.
Is the problem coming from xen_netbk_count_skb_slots() ?
Give me more information if you want me to help.
From: Ian Campbell <hidden> Date: 2012-10-09 14:18:13
On Tue, 2012-10-09 at 14:54 +0100, Eric Dumazet wrote:
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
quoted
Hi Eric,
Hi Ian
quoted
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
quoted
Are all net drivers expected to be able to handle compound pages in the
frags? Obviously it is to their benefit to do so, so it is something
I'll want to look into for netback.
Not sure why a net driver would care of COMPOUND page at all ?
a Fragment has a struct page *, and a size.
a page can be order-0, order-1, order-2, order-3, ...
I keep falling into this trap that a struct page * can be order > 0.
The Xen PV interfaces deal in order-0 pages only. Also things which are
contiguous in physical space may not be contiguous in DMA space (which
we call "machine memory" in Xen terminology).
The first is probably a specific quirk of Xen, but I thought there were
other architectures where physical and DMA space we not necessarily
contiguous and which would therefore need special handling (I guess
those platforms all have IOMMUs)
quoted
I expect the main factor here is bridging/forwarding, since the
receiving NIC and its driver appear to support compound pages but the
outgoing NIC (netback in this case) does not.
I guess my question is should I be rushing to fix netback ASAP or should
I rather be looking for a bug somewhere which caused a frag of this type
to get as far as netback's start_xmit in the first place?
Or am I just barking up the wrong tree to start with?
I think I can mostly deal with this in the same way netback deals with
large skb heads i.e. by busting the multipage page into individual 4096
page chunks.
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
If it's the latter then I think fixing netback is simple, if it's the
former then I might need to think a bit harder.
Ian.
From: Ian Campbell <hidden> Date: 2012-10-09 14:23:39
On Tue, 2012-10-09 at 15:01 +0100, Eric Dumazet wrote:
On Tue, 2012-10-09 at 15:54 +0200, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
quoted
Hi Eric,
Hi Ian
quoted
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
Hmm, I take it back. It also can give you the same problem :
We use this allocator for rx path of drivers :
__netdev_alloc_skb()
So its now absolutely possible that one skb->head is backed by a order-3
page.
Is the problem coming from xen_netbk_count_skb_slots() ?
Give me more information if you want me to help.
The interesting code is in netbk_gop_skb(), specifically the two calls
to netbk_gop_frag_copy.
netbk_gop_frag_copy can only copy order-0 pages to the peer since they
go over a shared ring transport which can only deal in order-0 pages.
For the SKB head there is a loop which handles order>0 heads, I suspect
we just need something similar for the frag case.
Although see my question in the other response about the maximum number
of frags we can have when order is > 0 since if using larger pages
causes us to end up with a much larger number of order-0 pages once
we've broken them up then we have a problem and I need to put my
thinking cap on a bit (perhaps substantially) tighter.
Konrad, it looks like netfront has a similar issue in
xennet_make_frags() since it doesn't shatter large order mappings
either.
Ian.
From: Eric Dumazet <hidden> Date: 2012-10-09 14:27:32
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
From: Eric Dumazet <hidden> Date: 2012-10-09 14:33:42
On Tue, 2012-10-09 at 15:23 +0100, Ian Campbell wrote:
On Tue, 2012-10-09 at 15:01 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:54 +0200, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
quoted
Hi Eric,
Hi Ian
quoted
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
Hmm, I take it back. It also can give you the same problem :
We use this allocator for rx path of drivers :
__netdev_alloc_skb()
So its now absolutely possible that one skb->head is backed by a order-3
page.
Is the problem coming from xen_netbk_count_skb_slots() ?
Give me more information if you want me to help.
The interesting code is in netbk_gop_skb(), specifically the two calls
to netbk_gop_frag_copy.
netbk_gop_frag_copy can only copy order-0 pages to the peer since they
go over a shared ring transport which can only deal in order-0 pages.
For the SKB head there is a loop which handles order>0 heads, I suspect
we just need something similar for the frag case.
Although see my question in the other response about the maximum number
of frags we can have when order is > 0 since if using larger pages
causes us to end up with a much larger number of order-0 pages once
we've broken them up then we have a problem and I need to put my
thinking cap on a bit (perhaps substantially) tighter.
Konrad, it looks like netfront has a similar issue in
xennet_make_frags() since it doesn't shatter large order mappings
either.
Hmm...
In theory, if a skb has 16+1 frags backed by compound pages, you could
need ~48 order-0 frags.
(4098 bytes could need 1-4096-1 (3 frags))
In practice, it should be around ~17 order-0 frags as before.
From: Ian Campbell <hidden> Date: 2012-10-09 14:40:34
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
As an aside, when the skb head is < 4096 bytes is that necessarily a
compound page or might it just be a large kmalloc area?
Only really relevant since it impacts the possibility for code sharing
between the head and the frags sending.
Ian
From: Ian Campbell <hidden> Date: 2012-10-09 14:54:37
On Tue, 2012-10-09 at 15:33 +0100, Eric Dumazet wrote:
On Tue, 2012-10-09 at 15:23 +0100, Ian Campbell wrote:
quoted
On Tue, 2012-10-09 at 15:01 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:54 +0200, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 14:47 +0100, Ian Campbell wrote:
quoted
Hi Eric,
Hi Ian
quoted
Sander has discovered an issue where xen-netback is given a compound
page as one of the skb frag pages to transmit. Currently netback can
only handle PAGE_SIZE'd frags and bugs out.
I suspect this is something to do with 69b08f62e174 "net: use bigger
pages in __netdev_alloc_frag", although perhaps not because it looks
like only tg3 uses it and Sander has an r8169. Also tg3 seems to only
call netdev_alloc_frag for sizes < PAGE_SIZE. I'm probably missing
something.
Its not the commit you want ;)
Hmm, I take it back. It also can give you the same problem :
We use this allocator for rx path of drivers :
__netdev_alloc_skb()
So its now absolutely possible that one skb->head is backed by a order-3
page.
Is the problem coming from xen_netbk_count_skb_slots() ?
Give me more information if you want me to help.
The interesting code is in netbk_gop_skb(), specifically the two calls
to netbk_gop_frag_copy.
netbk_gop_frag_copy can only copy order-0 pages to the peer since they
go over a shared ring transport which can only deal in order-0 pages.
For the SKB head there is a loop which handles order>0 heads, I suspect
we just need something similar for the frag case.
Although see my question in the other response about the maximum number
of frags we can have when order is > 0 since if using larger pages
causes us to end up with a much larger number of order-0 pages once
we've broken them up then we have a problem and I need to put my
thinking cap on a bit (perhaps substantially) tighter.
Konrad, it looks like netfront has a similar issue in
xennet_make_frags() since it doesn't shatter large order mappings
either.
Hmm...
In theory, if a skb has 16+1 frags backed by compound pages, you could
need ~48 order-0 frags.
(4098 bytes could need 1-4096-1 (3 frags))
In practice, it should be around ~17 order-0 frags as before.
Right, thanks. I think I can cope with that without needing to change
the PV protocol in any way.
Ian.
From: Eric Dumazet <hidden> Date: 2012-10-09 15:28:50
On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
As an aside, when the skb head is < 4096 bytes is that necessarily a
compound page or might it just be a large kmalloc area?
skb->head can be either allocated by kmalloc() (standard alloc_skb()) or
a page frag (if allocated in rx path)
Not sure its related to headlen/size...
From: Ian Campbell <hidden> Date: 2012-10-10 10:13:07
On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
The following seems to work for me.
I haven't tackled netfront yet.
8<--------------------------------------------------------------
From 551e42e3dd203f2eb97cb082985013bb33b8f020 Mon Sep 17 00:00:00 2001
From: Ian Campbell <redacted>
Date: Tue, 9 Oct 2012 15:51:20 +0100
Subject: [PATCH] xen: netback: handle compound page fragments on transmit.
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in netbk_gop_frag_copy and xen_netbk_count_skb_slots by
iterating over the frames which make up the page.
Signed-off-by: Ian Campbell <redacted>
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: Sander Eikelenboom <redacted>
---
drivers/net/xen-netback/netback.c | 40 ++++++++++++++++++++++++++++++++----
1 files changed, 35 insertions(+), 5 deletions(-)
@@ -403,14 +417,24 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,unsignedlongbytes;/* Data must not cross a page boundary. */-BUG_ON(size+offset>PAGE_SIZE);+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));meta=npo->meta+npo->meta_prod-1;+/* Skip unused frames from start of page */+page+=offset>>PAGE_SHIFT;+offset&=~PAGE_MASK;+while(size>0){+BUG_ON(offset>=PAGE_SIZE);BUG_ON(npo->copy_off>MAX_BUFFER_OFFSET);-if(start_new_rx_buffer(npo->copy_off,size,*head)){+bytes=PAGE_SIZE-offset;++if(bytes>size)+bytes=size;++if(start_new_rx_buffer(npo->copy_off,bytes,*head)){/**Netfrontrequirestheretobesomedatainthehead*buffer.
@@ -453,6 +476,13 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,offset+=bytes;size-=bytes;+/* Next frame */+if(offset==PAGE_SIZE){+BUG_ON(!PageCompound(page));+page++;+offset=0;+}+/* Leave a gap for the GSO descriptor. */if(*head&&skb_shinfo(skb)->gso_size&&!vif->gso_prefix)vif->rx.req_cons++;
Wednesday, October 10, 2012, 12:13:04 PM, you wrote:
On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
quoted
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
The following seems to work for me.
But it doesn't seem to work for me ... dmesg attached.
I don't know if the "mcelog:4359 map pfn expected mapping type write-back for [mem 0x0009f000-0x000a0fff], got uncached-minus"
is related, is shows up right after the nics get initialized ?
netback still fails with:
[ 191.777994] ------------[ cut here ]------------
[ 191.784245] kernel BUG at drivers/net/xen-netback/netback.c:481!
[ 191.790423] invalid opcode: 0000 [#1] PREEMPT SMP
[ 191.796462] Modules linked in:
[ 191.802315] CPU 1
[ 191.802367] Pid: 1177, comm: netback/1 Tainted: G W 3.6.0pre-rc1-20121010 #1 MSI MS-7640/890FXA-GD70 (MS-7640)
[ 191.814043] RIP: e030:[<ffffffff8146de61>] [<ffffffff8146de61>] netbk_gop_frag_copy+0x3f1/0x400
[ 191.820171] RSP: e02b:ffff880037c6bb98 EFLAGS: 00010246
[ 191.826271] RAX: 0000000000000244 RBX: ffffc90010827f98 RCX: ffff880031ed9880
[ 191.832450] RDX: 00000000000000a8 RSI: ffff880037c6bd24 RDI: ffffea0000b03f80
[ 191.838581] RBP: ffff880037c6bc28 R08: ffff8800319f8100 R09: 0000000000001000
[ 191.844739] R10: 0000000000000000 R11: 0000000000000132 R12: 00000000000000a8
[ 191.850785] R13: ffff880037c6bcd8 R14: 0000000000001000 R15: ffffc9001082cf70
[ 191.856741] FS: 00007f9f3c944700(0000) GS:ffff88003f840000(0000) knlGS:0000000000000000
[ 191.862841] CS: e033 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 191.868901] CR2: 0000000001337ca0 CR3: 0000000032cec000 CR4: 0000000000000660
[ 191.875053] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 191.881175] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 191.887247] Process netback/1 (pid: 1177, threadinfo ffff880037c6a000, task ffff880039984140)
[ 191.893325] Stack:
[ 191.899328] ffff880037c6bd24 00000000000000a8 ffff8800319f8100 ffff880031ed9880
[ 191.905534] ffffc90000000000 0000000000001000 0000000000000000 0000000000000000
[ 191.911742] ffff880000000000 ffffffff817459f3 ffffc90010823420 ffffea0000b03f80
[ 191.917898] Call Trace:
[ 191.923939] [<ffffffff817459f3>] ? _raw_spin_unlock_irqrestore+0x53/0xa0
[ 191.930141] [<ffffffff8146e1cb>] xen_netbk_rx_action+0x30b/0x830
[ 191.936543] [<ffffffff810ad22d>] ? trace_hardirqs_on+0xd/0x10
[ 191.942942] [<ffffffff8146f6da>] xen_netbk_kthread+0xba/0xa90
[ 191.949147] [<ffffffff81095b06>] ? try_to_wake_up+0x1b6/0x310
[ 191.955250] [<ffffffff81086b40>] ? wake_up_bit+0x40/0x40
[ 191.961421] [<ffffffff8146f620>] ? xen_netbk_tx_build_gops+0xa70/0xa70
[ 191.967660] [<ffffffff810864d6>] kthread+0xd6/0xe0
[ 191.973834] [<ffffffff81086400>] ? __init_kthread_worker+0x70/0x70
[ 191.979953] [<ffffffff8174677c>] ret_from_fork+0x7c/0x90
[ 191.986107] [<ffffffff81086400>] ? __init_kthread_worker+0x70/0x70
[ 191.992174] Code: b8 b3 00 00 48 8d 8c f1 60 01 00 00 48 3b 14 01 0f 85 72 fc ff ff e9 7a fc ff ff 0f 0b eb fe 0f 0b eb fe 0f 0b eb fe 0f 0b eb fe <0f> 0b eb fe 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 48 83
[ 192.005230] RIP [<ffffffff8146de61>] netbk_gop_frag_copy+0x3f1/0x400
[ 192.011786] RSP <ffff880037c6bb98>
[ 192.018402] ---[ end trace c51ab5e2c2c918fc ]---
--
Sander
@@ -403,14 +417,24 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,unsignedlongbytes;/* Data must not cross a page boundary. */-BUG_ON(size+offset>PAGE_SIZE);+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));meta=npo->meta+npo->meta_prod-1;+/* Skip unused frames from start of page */+page+=offset>>PAGE_SHIFT;+offset&=~PAGE_MASK;+while(size>0){+BUG_ON(offset>=PAGE_SIZE);BUG_ON(npo->copy_off>MAX_BUFFER_OFFSET);-if(start_new_rx_buffer(npo->copy_off,size,*head)){+bytes=PAGE_SIZE-offset;++if(bytes>size)+bytes=size;++if(start_new_rx_buffer(npo->copy_off,bytes,*head)){/**Netfrontrequirestheretobesomedatainthehead*buffer.
@@ -453,6 +476,13 @@ static void netbk_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,offset+=bytes;size-=bytes;+/* Next frame */+if(offset==PAGE_SIZE){+BUG_ON(!PageCompound(page));+page++;+offset=0;+}+/* Leave a gap for the GSO descriptor. */if(*head&&skb_shinfo(skb)->gso_size&&!vif->gso_prefix)vif->rx.req_cons++;
From: Ian Campbell <hidden> Date: 2012-10-10 12:29:42
On Wed, 2012-10-10 at 13:24 +0100, Sander Eikelenboom wrote:
Wednesday, October 10, 2012, 12:13:04 PM, you wrote:
quoted
On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
quoted
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
quoted
The following seems to work for me.
But it doesn't seem to work for me ... dmesg attached.
[ 191.777994] ------------[ cut here ]------------
[ 191.784245] kernel BUG at drivers/net/xen-netback/netback.c:481!
Looks like that BUG_ON is a little aggressive. It'll trigger if the data
happens to end on a frame boundary. Hopefully this will fix it for you:
From: Ian Campbell <hidden> Date: 2012-10-10 13:10:03
On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
I haven't tackled netfront yet.
I seem to be totally unable to reproduce the equivalent issue on the
netfront xmit side, even though it seems like the loop in
xennet_make_frags ought to be obviously susceptible to it.
Konrad, Sander, are either of you able to repro, e.g. with:
Wednesday, October 10, 2012, 2:29:09 PM, you wrote:
On Wed, 2012-10-10 at 13:24 +0100, Sander Eikelenboom wrote:
quoted
Wednesday, October 10, 2012, 12:13:04 PM, you wrote:
quoted
On Tue, 2012-10-09 at 15:40 +0100, Ian Campbell wrote:
quoted
On Tue, 2012-10-09 at 15:27 +0100, Eric Dumazet wrote:
quoted
On Tue, 2012-10-09 at 15:17 +0100, Ian Campbell wrote:
quoted
Does the higher order pages effectively reduce the number of frags which
are in use? e.g if MAX_SKB_FRAGS is 16, then for order-0 pages you could
have 64K worth of frag data.
If we switch to order-3 pages everywhere then can the skb contain 512K
of data, or does the effective maximum number of frags in an skb reduce
to 2?
effective number of frags reduce to 2 or 3
(We still limit GSO packets to ~63536 bytes)
Great! Then I think the fix is more/less trivial...
quoted
The following seems to work for me.
But it doesn't seem to work for me ... dmesg attached.
quoted
[ 191.777994] ------------[ cut here ]------------
[ 191.784245] kernel BUG at drivers/net/xen-netback/netback.c:481!
Looks like that BUG_ON is a little aggressive. It'll trigger if the data
happens to end on a frame boundary. Hopefully this will fix it for you:
Yes it does !
Thanks .. will recompile and test the netfront case as well
--
Sander
Wednesday, October 10, 2012, 3:09:58 PM, you wrote:
On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
quoted
I haven't tackled netfront yet.
I seem to be totally unable to reproduce the equivalent issue on the
netfront xmit side, even though it seems like the loop in
xennet_make_frags ought to be obviously susceptible to it.
Konrad, Sander, are either of you able to repro, e.g. with:
Hmrrrmm i don't see any traces, only strange behaviour ..
- i can connect to guests by ssh, but it's sluggish, and sometimes stops working
- The guest seem to keep trying to connect to netback:
[ 658.276719] xen_bridge: port 2(vif40.0) entered forwarding state
[ 658.282258] xen_bridge: port 2(vif40.0) entered forwarding state
[ 663.945964] xen_bridge: port 7(vif39.0) entered forwarding state
[ 669.674277] xen_bridge: port 2(vif40.0) entered disabled state
[ 669.680290] device vif40.0 left promiscuous mode
[ 669.685464] xen_bridge: port 2(vif40.0) entered disabled state
[ 672.857222] device vif41.0 entered promiscuous mode
[ 673.166254] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 673.176368] xen_bridge: port 2(vif41.0) entered forwarding state
[ 673.182042] xen_bridge: port 2(vif41.0) entered forwarding state
[ 674.439725] xen_bridge: port 7(vif39.0) entered disabled state
[ 674.445708] device vif39.0 left promiscuous mode
[ 674.450955] xen_bridge: port 7(vif39.0) entered disabled state
[ 677.726040] device vif42.0 entered promiscuous mode
[ 678.053381] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 678.062804] xen_bridge: port 7(vif42.0) entered forwarding state
[ 678.068433] xen_bridge: port 7(vif42.0) entered forwarding state
[ 688.224736] xen_bridge: port 2(vif41.0) entered forwarding state
[ 693.080557] xen_bridge: port 7(vif42.0) entered forwarding state
[ 700.786276] xen_bridge: port 7(vif42.0) entered disabled state
[ 700.792484] device vif42.0 left promiscuous mode
[ 700.802409] xen_bridge: port 7(vif42.0) entered disabled state
[ 704.133606] device vif43.0 entered promiscuous mode
[ 704.460160] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 704.469800] xen_bridge: port 7(vif43.0) entered forwarding state
[ 704.475303] xen_bridge: port 7(vif43.0) entered forwarding state
[ 719.493788] xen_bridge: port 7(vif43.0) entered forwarding state
[ 726.302456] xen_bridge: port 7(vif43.0) entered disabled state
[ 726.308898] device vif43.0 left promiscuous mode
[ 726.314029] xen_bridge: port 7(vif43.0) entered disabled state
All the guests are already up, but this keeps on going and going and going ....
From: Ian Campbell <hidden> Date: 2012-10-11 08:02:30
On Wed, 2012-10-10 at 15:49 +0100, Sander Eikelenboom wrote:
Wednesday, October 10, 2012, 3:09:58 PM, you wrote:
quoted
On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
quoted
I haven't tackled netfront yet.
quoted
I seem to be totally unable to reproduce the equivalent issue on the
netfront xmit side, even though it seems like the loop in
xennet_make_frags ought to be obviously susceptible to it.
quoted
Konrad, Sander, are either of you able to repro, e.g. with:
Hmrrrmm i don't see any traces, only strange behaviour ..
- i can connect to guests by ssh, but it's sluggish, and sometimes stops working
I saw something like this (ssh sluggish) even with dom0 itself. I'm
trying to see if I can characterise it enough to reliably bisect it.
I already switched out xen-unstable for 4.2-testing but that didn't make
any difference.
- The guest seem to keep trying to connect to netback:
[ 658.276719] xen_bridge: port 2(vif40.0) entered forwarding state
[ 658.282258] xen_bridge: port 2(vif40.0) entered forwarding state
[ 663.945964] xen_bridge: port 7(vif39.0) entered forwarding state
[ 669.674277] xen_bridge: port 2(vif40.0) entered disabled state
[ 669.680290] device vif40.0 left promiscuous mode
[ 669.685464] xen_bridge: port 2(vif40.0) entered disabled state
[ 672.857222] device vif41.0 entered promiscuous mode
[ 673.166254] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 673.176368] xen_bridge: port 2(vif41.0) entered forwarding state
[ 673.182042] xen_bridge: port 2(vif41.0) entered forwarding state
[ 674.439725] xen_bridge: port 7(vif39.0) entered disabled state
[ 674.445708] device vif39.0 left promiscuous mode
[ 674.450955] xen_bridge: port 7(vif39.0) entered disabled state
[ 677.726040] device vif42.0 entered promiscuous mode
[ 678.053381] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 678.062804] xen_bridge: port 7(vif42.0) entered forwarding state
[ 678.068433] xen_bridge: port 7(vif42.0) entered forwarding state
[ 688.224736] xen_bridge: port 2(vif41.0) entered forwarding state
[ 693.080557] xen_bridge: port 7(vif42.0) entered forwarding state
[ 700.786276] xen_bridge: port 7(vif42.0) entered disabled state
[ 700.792484] device vif42.0 left promiscuous mode
[ 700.802409] xen_bridge: port 7(vif42.0) entered disabled state
[ 704.133606] device vif43.0 entered promiscuous mode
[ 704.460160] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 704.469800] xen_bridge: port 7(vif43.0) entered forwarding state
[ 704.475303] xen_bridge: port 7(vif43.0) entered forwarding state
[ 719.493788] xen_bridge: port 7(vif43.0) entered forwarding state
[ 726.302456] xen_bridge: port 7(vif43.0) entered disabled state
[ 726.308898] device vif43.0 left promiscuous mode
[ 726.314029] xen_bridge: port 7(vif43.0) entered disabled state
All the guests are already up, but this keeps on going and going and going ....
The domain number seems to be climbing, are you sure something isn't
(crashing and) restarting?
Thursday, October 11, 2012, 10:02:26 AM, you wrote:
On Wed, 2012-10-10 at 15:49 +0100, Sander Eikelenboom wrote:
quoted
Wednesday, October 10, 2012, 3:09:58 PM, you wrote:
quoted
On Wed, 2012-10-10 at 11:13 +0100, Ian Campbell wrote:
quoted
I haven't tackled netfront yet.
quoted
I seem to be totally unable to reproduce the equivalent issue on the
netfront xmit side, even though it seems like the loop in
xennet_make_frags ought to be obviously susceptible to it.
quoted
Konrad, Sander, are either of you able to repro, e.g. with:
Hmrrrmm i don't see any traces, only strange behaviour ..
- i can connect to guests by ssh, but it's sluggish, and sometimes stops working
I saw something like this (ssh sluggish) even with dom0 itself. I'm
trying to see if I can characterise it enough to reliably bisect it.
I already switched out xen-unstable for 4.2-testing but that didn't make
any difference.
quoted
- The guest seem to keep trying to connect to netback:
[ 658.276719] xen_bridge: port 2(vif40.0) entered forwarding state
[ 658.282258] xen_bridge: port 2(vif40.0) entered forwarding state
[ 663.945964] xen_bridge: port 7(vif39.0) entered forwarding state
[ 669.674277] xen_bridge: port 2(vif40.0) entered disabled state
[ 669.680290] device vif40.0 left promiscuous mode
[ 669.685464] xen_bridge: port 2(vif40.0) entered disabled state
[ 672.857222] device vif41.0 entered promiscuous mode
[ 673.166254] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 673.176368] xen_bridge: port 2(vif41.0) entered forwarding state
[ 673.182042] xen_bridge: port 2(vif41.0) entered forwarding state
[ 674.439725] xen_bridge: port 7(vif39.0) entered disabled state
[ 674.445708] device vif39.0 left promiscuous mode
[ 674.450955] xen_bridge: port 7(vif39.0) entered disabled state
[ 677.726040] device vif42.0 entered promiscuous mode
[ 678.053381] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 678.062804] xen_bridge: port 7(vif42.0) entered forwarding state
[ 678.068433] xen_bridge: port 7(vif42.0) entered forwarding state
[ 688.224736] xen_bridge: port 2(vif41.0) entered forwarding state
[ 693.080557] xen_bridge: port 7(vif42.0) entered forwarding state
[ 700.786276] xen_bridge: port 7(vif42.0) entered disabled state
[ 700.792484] device vif42.0 left promiscuous mode
[ 700.802409] xen_bridge: port 7(vif42.0) entered disabled state
[ 704.133606] device vif43.0 entered promiscuous mode
[ 704.460160] xen-blkback:ring-ref 8, event-channel 9, protocol 1 (x86_64-abi)
[ 704.469800] xen_bridge: port 7(vif43.0) entered forwarding state
[ 704.475303] xen_bridge: port 7(vif43.0) entered forwarding state
[ 719.493788] xen_bridge: port 7(vif43.0) entered forwarding state
[ 726.302456] xen_bridge: port 7(vif43.0) entered disabled state
[ 726.308898] device vif43.0 left promiscuous mode
[ 726.314029] xen_bridge: port 7(vif43.0) entered disabled state
All the guests are already up, but this keeps on going and going and going ....
The domain number seems to be climbing, are you sure something isn't
(crashing and) restarting?
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
[ 34.298549] ------------[ cut here ]------------
[ 34.298567] WARNING: at drivers/net/xen-netfront.c:465 xennet_start_xmit+0x7fe/0x860()
[ 34.298574] Modules linked in:
[ 34.298597] Pid: 1580, comm: sshd Not tainted 3.6.0pre-rc1-20121011 #1
[ 34.298603] Call Trace:
[ 34.298611] [<ffffffff810664ea>] warn_slowpath_common+0x7a/0xb0
[ 34.298617] [<ffffffff81066535>] warn_slowpath_null+0x15/0x20
[ 34.298623] [<ffffffff8146d89e>] xennet_start_xmit+0x7fe/0x860
[ 34.298631] [<ffffffff8161f349>] dev_hard_start_xmit+0x209/0x460
[ 34.298637] [<ffffffff8163b036>] sch_direct_xmit+0xf6/0x290
[ 34.298643] [<ffffffff8161f746>] dev_queue_xmit+0x1a6/0x5a0
[ 34.298649] [<ffffffff8161f5a0>] ? dev_hard_start_xmit+0x460/0x460
[ 34.298656] [<ffffffff810aa8e5>] ? trace_softirqs_off+0x85/0x1b0
[ 34.298663] [<ffffffff816b9536>] ip_finish_output+0x226/0x530
[ 34.298668] [<ffffffff816b93dd>] ? ip_finish_output+0xcd/0x530
[ 34.298674] [<ffffffff816b9899>] ip_output+0x59/0xe0
[ 34.298680] [<ffffffff816b83b8>] ip_local_out+0x28/0x90
[ 34.298687] [<ffffffff816b896f>] ip_queue_xmit+0x17f/0x4a0
[ 34.298692] [<ffffffff816b87f0>] ? ip_send_unicast_reply+0x340/0x340
[ 34.298699] [<ffffffff810a0ba7>] ? getnstimeofday+0x47/0xe0
[ 34.298705] [<ffffffff8160f4c9>] ? __skb_clone+0x29/0x120
[ 34.298711] [<ffffffff816cea20>] tcp_transmit_skb+0x400/0x8d0
[ 34.298717] [<ffffffff816d19fa>] tcp_write_xmit+0x21a/0xa50
[ 34.298723] [<ffffffff816d225b>] tcp_push_one+0x2b/0x40
[ 34.298728] [<ffffffff816c2dec>] tcp_sendmsg+0x8dc/0xe20
[ 34.298735] [<ffffffff816e8f19>] inet_sendmsg+0xa9/0x100
[ 34.298740] [<ffffffff816e8e70>] ? inet_autobind+0x70/0x70
[ 34.298746] [<ffffffff810b0f88>] ? lock_acquire+0xd8/0x100
[ 34.298753] [<ffffffff8160630d>] sock_aio_write+0x12d/0x140
[ 34.298762] [<ffffffff811435b2>] do_sync_write+0xa2/0xe0
[ 34.298768] [<ffffffff810ad22d>] ? trace_hardirqs_on+0xd/0x10
[ 34.298774] [<ffffffff811441d4>] vfs_write+0x174/0x190
[ 34.298779] [<ffffffff811442fa>] sys_write+0x5a/0xa0
[ 34.298786] [<ffffffff812b33de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 34.298792] [<ffffffff817491cc>] cstar_dispatch+0x7/0x26
[ 34.298797] ---[ end trace 2e28eec93b7a8b74 ]---
Complete dmesg from guest attached.
From: Eric Dumazet <hidden> Date: 2012-10-11 10:05:27
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
From: Ian Campbell <hidden> Date: 2012-10-11 10:15:16
On Thu, 2012-10-11 at 11:05 +0100, Eric Dumazet wrote:
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
quoted
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
Right, I just want to be reproduce the issue so I can know I've fixed it
properly ;-)
Ian.
Thursday, October 11, 2012, 12:14:54 PM, you wrote:
On Thu, 2012-10-11 at 11:05 +0100, Eric Dumazet wrote:
quoted
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
quoted
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
Right, I just want to be reproduce the issue so I can know I've fixed it
properly ;-)
Trying to scp/sftp files from a guest seems to trigger it for me ..
On Thu, 2012-10-11 at 11:05 +0100, Eric Dumazet wrote:
quoted
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
quoted
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
Right, I just want to be reproduce the issue so I can know I've fixed it
properly ;-)
Hi Ian,
I can reproduce this BUG_ON when running netperf/netserver test between two domus running on the same dom0. The domu and dom0 all use v3.7-rc1.
When I tried to rebase my persistent grant netfront/netback patch on latest kernel, netperf/netserver test never succeeded. I did some test to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and v3.7-rc4 does not succeed in netperf/netserver test. So I keep my persistent grant patch only based on v3.4-rc3 now.
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in v3.7-rc1, and suggested me to test your debug patch in netfront. This BUG_ON happens soon after running the netperf/netserver test case.
Thanks
Annie
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Thursday, November 15, 2012, 3:31:42 AM, you wrote:
On 2012-10-11 18:14, Ian Campbell wrote:
quoted
On Thu, 2012-10-11 at 11:05 +0100, Eric Dumazet wrote:
quoted
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
quoted
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
Right, I just want to be reproduce the issue so I can know I've fixed it
properly ;-)
Hi Ian,
I can reproduce this BUG_ON when running netperf/netserver test between
two domus running on the same dom0. The domu and dom0 all use v3.7-rc1.
When I tried to rebase my persistent grant netfront/netback patch on
latest kernel, netperf/netserver test never succeeded. I did some test
to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and
v3.7-rc4 does not succeed in netperf/netserver test. So I keep my
persistent grant patch only based on v3.4-rc3 now.
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in
v3.7-rc1, and suggested me to test your debug patch in netfront. This
BUG_ON happens soon after running the netperf/netserver test case.
Thanks
Annie
Is there any progression with this bug (rc6 is out the door, so the release of 3.7-final seems to be eminent and this bug completely cripples any networking with guests) ?
--
Sander
quoted
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
From: Stefan Bader <hidden> Date: 2012-11-20 08:30:36
On 19.11.2012 16:43, Sander Eikelenboom wrote:
Thursday, November 15, 2012, 3:31:42 AM, you wrote:
quoted
On 2012-10-11 18:14, Ian Campbell wrote:
quoted
On Thu, 2012-10-11 at 11:05 +0100, Eric Dumazet wrote:
quoted
On Thu, 2012-10-11 at 12:00 +0200, Sander Eikelenboom wrote:
quoted
Probably due to the BUG_ON from the patch below, i changed it into a WARN_ON.
And i seem to hit it, but only in one of the guests at the moment and it triggers quite irregularly.
xennet_make_frags() is able to split the skb->head in multiple page-size
chunks.
It should do the same for fragments
Right, I just want to be reproduce the issue so I can know I've fixed it
properly ;-)
Hi Ian,
quoted
I can reproduce this BUG_ON when running netperf/netserver test between
two domus running on the same dom0. The domu and dom0 all use v3.7-rc1.
quoted
When I tried to rebase my persistent grant netfront/netback patch on
latest kernel, netperf/netserver test never succeeded. I did some test
to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and
v3.7-rc4 does not succeed in netperf/netserver test. So I keep my
persistent grant patch only based on v3.4-rc3 now.
quoted
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in
v3.7-rc1, and suggested me to test your debug patch in netfront. This
BUG_ON happens soon after running the netperf/netserver test case.
quoted
Thanks
Annie
Is there any progression with this bug (rc6 is out the door, so the release of 3.7-final seems to be eminent and this bug completely cripples any networking with guests) ?
+1 on that. I was testing yesterday with a PVM domU running 3.7-rc5 on Xen 4.2
(but also reported from EC2 running Xen 3.4.3) c with one VCPU. I actually can
trigger it by just ssh'ing into the domU (from another machine) and then run
"find /". Output starts to stutter and then stops completely. When this happens
a new connection still can be made and as long as only shorter output is
generated the ssh connection is ok. From a dump taken it looks like user-space
is waiting in some select call (without any warnon I rather won't see the tx path).
-Stefan
From: Ian Campbell <hidden> Date: 2012-11-20 09:21:26
On Tue, 2012-11-20 at 08:30 +0000, Stefan Bader wrote:
quoted
quoted
When I tried to rebase my persistent grant netfront/netback patch on
latest kernel, netperf/netserver test never succeeded. I did some test
to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and
v3.7-rc4 does not succeed in netperf/netserver test. So I keep my
persistent grant patch only based on v3.4-rc3 now.
quoted
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in
v3.7-rc1, and suggested me to test your debug patch in netfront. This
BUG_ON happens soon after running the netperf/netserver test case.
quoted
Thanks
Annie
Is there any progression with this bug (rc6 is out the door, so the
release of 3.7-final seems to be eminent and this bug completely
cripples any networking with guests) ?
quoted
+1 on that. I was testing yesterday with a PVM domU running 3.7-rc5 on Xen 4.2
(but also reported from EC2 running Xen 3.4.3) c with one VCPU. I actually can
trigger it by just ssh'ing into the domU (from another machine) and then run
"find /". Output starts to stutter and then stops completely. When this happens
a new connection still can be made and as long as only shorter output is
generated the ssh connection is ok. From a dump taken it looks like user-space
is waiting in some select call (without any warnon I rather won't see the tx path).
Annie, are you still looking into this or shall I?
Ian.
From: Ian Campbell <hidden> Date: 2012-11-20 11:36:56
On Tue, 2012-11-20 at 09:21 +0000, Ian Campbell wrote:
On Tue, 2012-11-20 at 08:30 +0000, Stefan Bader wrote:
quoted
quoted
quoted
When I tried to rebase my persistent grant netfront/netback patch on
latest kernel, netperf/netserver test never succeeded. I did some test
to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and
v3.7-rc4 does not succeed in netperf/netserver test. So I keep my
persistent grant patch only based on v3.4-rc3 now.
quoted
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in
v3.7-rc1, and suggested me to test your debug patch in netfront. This
BUG_ON happens soon after running the netperf/netserver test case.
quoted
Thanks
Annie
Is there any progression with this bug (rc6 is out the door, so the
release of 3.7-final seems to be eminent and this bug completely
cripples any networking with guests) ?
quoted
+1 on that. I was testing yesterday with a PVM domU running 3.7-rc5 on Xen 4.2
(but also reported from EC2 running Xen 3.4.3) c with one VCPU. I actually can
trigger it by just ssh'ing into the domU (from another machine) and then run
"find /". Output starts to stutter and then stops completely. When this happens
a new connection still can be made and as long as only shorter output is
generated the ssh connection is ok. From a dump taken it looks like user-space
is waiting in some select call (without any warnon I rather won't see the tx path).
Annie, are you still looking into this or shall I?
I'll assume that silence == No. Will post a patch shortly.
Ian.
From: Ian Campbell <hidden> Date: 2012-11-20 11:40:08
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell <redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: ANNIE LI <redacted>
Cc: Sander Eikelenboom <redacted>
Cc: Stefan Bader <redacted>
---
drivers/net/xen-netfront.c | 58 +++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 14 deletions(-)
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;-tx->flags|=XEN_NETTXF_more_data;+/* Data must not cross a page boundary. */+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));-id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);-np->tx_skbs[id].skb=skb_get(skb);-tx=RING_GET_REQUEST(&np->tx,prod++);-tx->id=id;-ref=gnttab_claim_grant_reference(&np->gref_tx_head);-BUG_ON((signedshort)ref<0);+/* Skip unused frames from start of page */+page+=offset>>PAGE_SHIFT;+offset&=~PAGE_MASK;-mfn=pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));-gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,-mfn,GNTMAP_readonly);+while(size>0){+unsignedlongbytes;-tx->gref=np->grant_tx_ref[id]=ref;-tx->offset=frag->page_offset;-tx->size=skb_frag_size(frag);-tx->flags=0;+BUG_ON(offset>=PAGE_SIZE);++bytes=PAGE_SIZE-offset;+if(bytes>size)+bytes=size;++tx->flags|=XEN_NETTXF_more_data;++id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);+np->tx_skbs[id].skb=skb_get(skb);+tx=RING_GET_REQUEST(&np->tx,prod++);+tx->id=id;+ref=gnttab_claim_grant_reference(&np->gref_tx_head);+BUG_ON((signedshort)ref<0);++mfn=pfn_to_mfn(page_to_pfn(page));+gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,+mfn,GNTMAP_readonly);++tx->gref=np->grant_tx_ref[id]=ref;+tx->offset=offset;+tx->size=bytes;+tx->flags=0;++offset+=bytes;+size-=bytes;++/* Next frame */+if(offset==PAGE_SIZE&&size){+BUG_ON(!PageCompound(page));+page++;+offset=0;+}+}}np->tx.req_prod_pvt=prod;
From: Jan Beulich <hidden> Date: 2012-11-20 12:27:20
quoted
quoted
On 20.11.12 at 12:40, Ian Campbell [off-list ref] wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Wouldn't you need to be at least a little more conservative here
with respect to resource use: I realize that get_id_from_freelist()
return values were never checked, and failure of
gnttab_claim_grant_reference() was always dealt with via
BUG_ON(), but considering that netfront_tx_slot_available()
doesn't account for compound page fragments, I think this (lack
of) error handling needs improvement in the course of the
change here (regardless of - I think - someone having said that
usually the sum of all pages referenced from an skb's fragments
would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
imo).
Jan
quoted hunk
Signed-off-by: Ian Campbell <redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: ANNIE LI <redacted>
Cc: Sander Eikelenboom <redacted>
Cc: Stefan Bader <redacted>
---
drivers/net/xen-netfront.c | 58 +++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 14 deletions(-)
From: Stefan Bader <hidden> Date: 2012-11-20 13:31:00
Aside from Jans comments about error handling, I tried below patch and it seems
to solve the problem with transfers out of the domU for me (though only shallow
testing done, otoh 5 times is more than getting stuck the first time).
-Stefan
On 20.11.2012 12:40, Ian Campbell wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell <redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: ANNIE LI <redacted>
Cc: Sander Eikelenboom <redacted>
Cc: Stefan Bader <redacted>
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;-tx->flags|=XEN_NETTXF_more_data;+/* Data must not cross a page boundary. */+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));-id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);-np->tx_skbs[id].skb=skb_get(skb);-tx=RING_GET_REQUEST(&np->tx,prod++);-tx->id=id;-ref=gnttab_claim_grant_reference(&np->gref_tx_head);-BUG_ON((signedshort)ref<0);+/* Skip unused frames from start of page */+page+=offset>>PAGE_SHIFT;+offset&=~PAGE_MASK;-mfn=pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));-gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,-mfn,GNTMAP_readonly);+while(size>0){+unsignedlongbytes;-tx->gref=np->grant_tx_ref[id]=ref;-tx->offset=frag->page_offset;-tx->size=skb_frag_size(frag);-tx->flags=0;+BUG_ON(offset>=PAGE_SIZE);++bytes=PAGE_SIZE-offset;+if(bytes>size)+bytes=size;++tx->flags|=XEN_NETTXF_more_data;++id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);+np->tx_skbs[id].skb=skb_get(skb);+tx=RING_GET_REQUEST(&np->tx,prod++);+tx->id=id;+ref=gnttab_claim_grant_reference(&np->gref_tx_head);+BUG_ON((signedshort)ref<0);++mfn=pfn_to_mfn(page_to_pfn(page));+gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,+mfn,GNTMAP_readonly);++tx->gref=np->grant_tx_ref[id]=ref;+tx->offset=offset;+tx->size=bytes;+tx->flags=0;++offset+=bytes;+size-=bytes;++/* Next frame */+if(offset==PAGE_SIZE&&size){+BUG_ON(!PageCompound(page));+page++;+offset=0;+}+}}np->tx.req_prod_pvt=prod;
From: Ian Campbell <hidden> Date: 2012-11-20 13:35:19
On Tue, 2012-11-20 at 12:28 +0000, Jan Beulich wrote:
quoted
quoted
quoted
On 20.11.12 at 12:40, Ian Campbell [off-list ref] wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Wouldn't you need to be at least a little more conservative here
with respect to resource use: I realize that get_id_from_freelist()
return values were never checked, and failure of
gnttab_claim_grant_reference() was always dealt with via
BUG_ON(), but considering that netfront_tx_slot_available()
doesn't account for compound page fragments, I think this (lack
of) error handling needs improvement in the course of the
change here (regardless of - I think - someone having said that
usually the sum of all pages referenced from an skb's fragments
would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
imo).
I think it is more than "usually", it is derived from the number of
pages needed to contain 64K of data which is the maximum size of the
data associated with an skb (AIUI).
Unwinding from failure in xennet_make_frags looks pretty tricky, but how
about this incremental patch:
Tuesday, November 20, 2012, 2:30:54 PM, you wrote:
Aside from Jans comments about error handling, I tried below patch and it seems
to solve the problem with transfers out of the domU for me (though only shallow
testing done, otoh 5 times is more than getting stuck the first time).
-Stefan
I'm running with this patch now, it seems to fix the problems for me as well.
--
Sander
On 20.11.2012 12:40, Ian Campbell wrote:
quoted
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell <redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: ANNIE LI <redacted>
Cc: Sander Eikelenboom <redacted>
Cc: Stefan Bader <redacted>
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;-tx->flags|=XEN_NETTXF_more_data;+/* Data must not cross a page boundary. */+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));-id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);-np->tx_skbs[id].skb=skb_get(skb);-tx=RING_GET_REQUEST(&np->tx,prod++);-tx->id=id;-ref=gnttab_claim_grant_reference(&np->gref_tx_head);-BUG_ON((signedshort)ref<0);+/* Skip unused frames from start of page */+page+=offset>>PAGE_SHIFT;+offset&=~PAGE_MASK;-mfn=pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));-gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,-mfn,GNTMAP_readonly);+while(size>0){+unsignedlongbytes;-tx->gref=np->grant_tx_ref[id]=ref;-tx->offset=frag->page_offset;-tx->size=skb_frag_size(frag);-tx->flags=0;+BUG_ON(offset>=PAGE_SIZE);++bytes=PAGE_SIZE-offset;+if(bytes>size)+bytes=size;++tx->flags|=XEN_NETTXF_more_data;++id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);+np->tx_skbs[id].skb=skb_get(skb);+tx=RING_GET_REQUEST(&np->tx,prod++);+tx->id=id;+ref=gnttab_claim_grant_reference(&np->gref_tx_head);+BUG_ON((signedshort)ref<0);++mfn=pfn_to_mfn(page_to_pfn(page));+gnttab_grant_foreign_access_ref(ref,np->xbdev->otherend_id,+mfn,GNTMAP_readonly);++tx->gref=np->grant_tx_ref[id]=ref;+tx->offset=offset;+tx->size=bytes;+tx->flags=0;++offset+=bytes;+size-=bytes;++/* Next frame */+if(offset==PAGE_SIZE&&size){+BUG_ON(!PageCompound(page));+page++;+offset=0;+}+}}np->tx.req_prod_pvt=prod;
From: Jan Beulich <hidden> Date: 2012-11-20 13:50:56
quoted
quoted
On 20.11.12 at 14:35, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 12:28 +0000, Jan Beulich wrote:
quoted
quoted
quoted
quoted
On 20.11.12 at 12:40, Ian Campbell [off-list ref] wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Wouldn't you need to be at least a little more conservative here
with respect to resource use: I realize that get_id_from_freelist()
return values were never checked, and failure of
gnttab_claim_grant_reference() was always dealt with via
BUG_ON(), but considering that netfront_tx_slot_available()
doesn't account for compound page fragments, I think this (lack
of) error handling needs improvement in the course of the
change here (regardless of - I think - someone having said that
usually the sum of all pages referenced from an skb's fragments
would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
imo).
I think it is more than "usually", it is derived from the number of
pages needed to contain 64K of data which is the maximum size of the
data associated with an skb (AIUI).
Unwinding from failure in xennet_make_frags looks pretty tricky,
Yes, I agree.
but how about this incremental patch:
Looks good, but can probably be simplified quite a bit:
@@ -505,6 +505,46 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,np->tx.req_prod_pvt=prod;}+/*+*Counthowmanyringslotsarerequiredtosendthefragsofthis+*skb.Eachfragmightbeacompoundpage.+*/+staticintxennet_count_skb_frag_pages(structsk_buff*skb)+{+inti,frags=skb_shinfo(skb)->nr_frags;+intpages=0;++for(i=0;i<frags;i++){+skb_frag_t*frag=skb_shinfo(skb)->frags+i;+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;++/* Skip unused frames from start of page */+offset&=~PAGE_MASK;++while(size>0){+unsignedlongbytes;++BUG_ON(offset>=PAGE_SIZE);++bytes=PAGE_SIZE-offset;+if(bytes>size)+bytes=size;++offset+=bytes;+size-=bytes;++/* Next frame */+if(offset==PAGE_SIZE&&size){+pages++;+offset=0;+}+}
Isn't the whole loop equivalent to
pages = PFN_UP(offset + size);
(at least as long as size is not zero)?
Plus I think the increment of pages would need to be pulled out
of the if() body.
quoted hunk
+ }++ return pages;+}+ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) { unsigned short id;
@@ -517,12 +557,13 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) grant_ref_t ref; unsigned long mfn; int notify;- int frags = skb_shinfo(skb)->nr_frags;+ int frags; unsigned int offset = offset_in_page(data); unsigned int len = skb_headlen(skb); unsigned long flags;- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);+ frags = xennet_count_skb_frag_pages(skb) ++ DIV_ROUND_UP(offset + len, PAGE_SIZE); if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
This condition would now need adjustment, though (because
"frags" is no longer what its name says).
Jan
printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
frags);
From: Ian Campbell <hidden> Date: 2012-11-20 14:14:28
On Tue, 2012-11-20 at 13:51 +0000, Jan Beulich wrote:
quoted
quoted
quoted
On 20.11.12 at 14:35, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 12:28 +0000, Jan Beulich wrote:
quoted
quoted
quoted
quoted
On 20.11.12 at 12:40, Ian Campbell [off-list ref] wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Wouldn't you need to be at least a little more conservative here
with respect to resource use: I realize that get_id_from_freelist()
return values were never checked, and failure of
gnttab_claim_grant_reference() was always dealt with via
BUG_ON(), but considering that netfront_tx_slot_available()
doesn't account for compound page fragments, I think this (lack
of) error handling needs improvement in the course of the
change here (regardless of - I think - someone having said that
usually the sum of all pages referenced from an skb's fragments
would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
imo).
I think it is more than "usually", it is derived from the number of
pages needed to contain 64K of data which is the maximum size of the
data associated with an skb (AIUI).
Unwinding from failure in xennet_make_frags looks pretty tricky,
Yes, I agree.
quoted
but how about this incremental patch:
Looks good, but can probably be simplified quite a bit:
@@ -505,6 +505,46 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,np->tx.req_prod_pvt=prod;}+/*+*Counthowmanyringslotsarerequiredtosendthefragsofthis+*skb.Eachfragmightbeacompoundpage.+*/+staticintxennet_count_skb_frag_pages(structsk_buff*skb)+{+inti,frags=skb_shinfo(skb)->nr_frags;+intpages=0;++for(i=0;i<frags;i++){+skb_frag_t*frag=skb_shinfo(skb)->frags+i;+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;++/* Skip unused frames from start of page */+offset&=~PAGE_MASK;++while(size>0){+unsignedlongbytes;++BUG_ON(offset>=PAGE_SIZE);++bytes=PAGE_SIZE-offset;+if(bytes>size)+bytes=size;++offset+=bytes;+size-=bytes;++/* Next frame */+if(offset==PAGE_SIZE&&size){+pages++;+offset=0;+}+}
Isn't the whole loop equivalent to
pages = PFN_UP(offset + size);
(at least as long as size is not zero)?
Er, yes. Wood for the trees etc...
I think using PFN_UP overcounts a bit since the data needed start in the
first frame of a compound frame, but if you keep the
/* Skip unused frames from start of page */
offset &= ~PAGE_MASK;
I think that does the right thing
quoted
@@ -517,12 +557,13 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) grant_ref_t ref; unsigned long mfn; int notify;- int frags = skb_shinfo(skb)->nr_frags;+ int frags; unsigned int offset = offset_in_page(data); unsigned int len = skb_headlen(skb); unsigned long flags;- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);+ frags = xennet_count_skb_frag_pages(skb) ++ DIV_ROUND_UP(offset + len, PAGE_SIZE); if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
This condition would now need adjustment, though (because
"frags" is no longer what its name says).
I think it already wasn't what the name says, since it included the skb
head too. Perhaps "slots" would be a better name?
From: Jan Beulich <hidden> Date: 2012-11-20 14:31:48
quoted
quoted
On 20.11.12 at 15:14, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 13:51 +0000, Jan Beulich wrote:
quoted
quoted
quoted
quoted
On 20.11.12 at 14:35, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 12:28 +0000, Jan Beulich wrote:
quoted
quoted
quoted
quoted
On 20.11.12 at 12:40, Ian Campbell [off-list ref] wrote:
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Wouldn't you need to be at least a little more conservative here
with respect to resource use: I realize that get_id_from_freelist()
return values were never checked, and failure of
gnttab_claim_grant_reference() was always dealt with via
BUG_ON(), but considering that netfront_tx_slot_available()
doesn't account for compound page fragments, I think this (lack
of) error handling needs improvement in the course of the
change here (regardless of - I think - someone having said that
usually the sum of all pages referenced from an skb's fragments
would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
imo).
I think it is more than "usually", it is derived from the number of
pages needed to contain 64K of data which is the maximum size of the
data associated with an skb (AIUI).
Unwinding from failure in xennet_make_frags looks pretty tricky,
Yes, I agree.
quoted
but how about this incremental patch:
Looks good, but can probably be simplified quite a bit:
np->tx.req_prod_pvt = prod; }+/*+ * Count how many ring slots are required to send the frags of this+ * skb. Each frag might be a compound page.+ */+static int xennet_count_skb_frag_pages(struct sk_buff *skb)+{+ int i, frags = skb_shinfo(skb)->nr_frags;+ int pages = 0;++ for (i = 0; i < frags; i++) {+ skb_frag_t *frag = skb_shinfo(skb)->frags + i;+ unsigned long size = skb_frag_size(frag);+ unsigned long offset = frag->page_offset;++ /* Skip unused frames from start of page */+ offset &= ~PAGE_MASK;++ while (size > 0) {+ unsigned long bytes;++ BUG_ON(offset >= PAGE_SIZE);++ bytes = PAGE_SIZE - offset;+ if (bytes > size)+ bytes = size;++ offset += bytes;+ size -= bytes;++ /* Next frame */+ if (offset == PAGE_SIZE && size) {+ pages++;+ offset = 0;+ }+ }
Isn't the whole loop equivalent to
pages = PFN_UP(offset + size);
(at least as long as size is not zero)?
Er, yes. Wood for the trees etc...
I think using PFN_UP overcounts a bit since the data needed start in the
first frame of a compound frame, but if you keep the
/* Skip unused frames from start of page */
offset &= ~PAGE_MASK;
I think that does the right thing
Right, that's what I said (I only wanted the loop to be replaced, not
what was prior to it).
quoted hunk
@@ -517,15 +540,16 @@ static int xennet_start_xmit(struct sk_buff *skb,
struct net_device *dev)
grant_ref_t ref;
unsigned long mfn;
int notify;
- int frags = skb_shinfo(skb)->nr_frags;
+ int slots;
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
unsigned long flags;
- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
- if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
- printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
- frags);
+ slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
+ xennet_count_skb_frag_slots(skb);
+ if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
But still - isn't this wrong now (i.e. can't it now validly exceed the
boundary checked for)?
Jan
From: Eric Dumazet <hidden> Date: 2012-11-20 14:45:39
On Tue, 2012-11-20 at 11:40 +0000, Ian Campbell wrote:
quoted hunk
An SKB paged fragment can consist of a compound page with order > 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell <redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet <redacted>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: ANNIE LI <redacted>
Cc: Sander Eikelenboom <redacted>
Cc: Stefan Bader <redacted>
---
drivers/net/xen-netfront.c | 58 +++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 14 deletions(-)
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;-tx->flags|=XEN_NETTXF_more_data;+/* Data must not cross a page boundary. */+BUG_ON(size+offset>PAGE_SIZE<<compound_order(page));-id=get_id_from_freelist(&np->tx_skb_freelist,np->tx_skbs);-np->tx_skbs[id].skb=skb_get(skb);-tx=RING_GET_REQUEST(&np->tx,prod++);-tx->id=id;-ref=gnttab_claim_grant_reference(&np->gref_tx_head);-BUG_ON((signedshort)ref<0);+/* Skip unused frames from start of page */
BTW this skb_get() means extra atomic operations for every 4096 bytes
unit, and an extra atomic op (and test for final 0) at TX completion.
This could be avoided, by setting np->tx_skbs[id].skb = skb only for the
very last unit.
From: Ian Campbell <hidden> Date: 2012-11-20 15:05:45
On Tue, 2012-11-20 at 14:45 +0000, Eric Dumazet wrote:
quoted
+ /* Skip unused frames from start of page */
'frame' in the comment means an order-0 page ?
Yes. Confusing in the context of a network driver I know! I couldn't
think of a better term.
quoted
+ id = get_id_from_freelist(&np->tx_skb_freelist, np->tx_skbs);
+ np->tx_skbs[id].skb = skb_get(skb);
BTW this skb_get() means extra atomic operations for every 4096 bytes
unit, and an extra atomic op (and test for final 0) at TX completion.
This could be avoided, by setting np->tx_skbs[id].skb = skb only for the
very last unit.
Thanks. Might be tricky because guests can ack the individual requests
in any order but it's something worth having a look at.
From: Ian Campbell <hidden> Date: 2012-11-20 15:06:57
On Tue, 2012-11-20 at 14:32 +0000, Jan Beulich wrote:
quoted
@@ -517,15 +540,16 @@ static int xennet_start_xmit(struct sk_buff *skb,
struct net_device *dev)
grant_ref_t ref;
unsigned long mfn;
int notify;
- int frags = skb_shinfo(skb)->nr_frags;
+ int slots;
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
unsigned long flags;
- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
- if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
- printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
- frags);
+ slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
+ xennet_count_skb_frag_slots(skb);
+ if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
But still - isn't this wrong now (i.e. can't it now validly exceed the
boundary checked for)?
In practice no because of the property that the number of pages backing
the frags is <= MAX_SKB_FRAGS even if you are using compound pages as
the frags.
Ian.
From: Eric Dumazet <hidden> Date: 2012-11-20 15:28:33
On Tue, 2012-11-20 at 15:06 +0000, Ian Campbell wrote:
In practice no because of the property that the number of pages backing
the frags is <= MAX_SKB_FRAGS even if you are using compound pages as
the frags.
Yes, but you can make this test trigger with some hacks from userland
(since the frag allocator is per task instead of per socket), so you
should remove the dump_stack() ?
Best way would be to count exact number of slots.
This could be something like 48 slots for a single skb
(if each frag is 4098 (1+4096+1)bytes, only the last one is around 4000
bytes)
MAX_SKB_FRAGS is really number of frags, while your driver needs a count
of 'order-0' 'frames'
From: Jan Beulich <hidden> Date: 2012-11-20 15:43:56
quoted
quoted
On 20.11.12 at 16:06, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 14:32 +0000, Jan Beulich wrote:
quoted
quoted
@@ -517,15 +540,16 @@ static int xennet_start_xmit(struct sk_buff *skb,
struct net_device *dev)
grant_ref_t ref;
unsigned long mfn;
int notify;
- int frags = skb_shinfo(skb)->nr_frags;
+ int slots;
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
unsigned long flags;
- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
- if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
- printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
- frags);
+ slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
+ xennet_count_skb_frag_slots(skb);
+ if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
But still - isn't this wrong now (i.e. can't it now validly exceed the
boundary checked for)?
In practice no because of the property that the number of pages backing
the frags is <= MAX_SKB_FRAGS even if you are using compound pages as
the frags.
So are you saying that there is something in the system
preventing up to MAX_SKB_FRAGS * SKB_FRAG_PAGE_ORDER
(or NETDEV_FRAG_PAGE_MAX_ORDER) skb-s to be created? I
didn't find any. I do notice that __netdev_alloc_frag() currently
never gets called with a size larger than PAGE_SIZE, but
considering that the function just recently got made capable of
that, I'm sure respective users will show up rather sooner than
later.
Jan
From: Ian Campbell <hidden> Date: 2012-11-20 15:54:40
On Tue, 2012-11-20 at 15:28 +0000, Eric Dumazet wrote:
On Tue, 2012-11-20 at 15:06 +0000, Ian Campbell wrote:
quoted
In practice no because of the property that the number of pages backing
the frags is <= MAX_SKB_FRAGS even if you are using compound pages as
the frags.
Yes, but you can make this test trigger with some hacks from userland
(since the frag allocator is per task instead of per socket), so you
should remove the dump_stack() ?
Best way would be to count exact number of slots.
This could be something like 48 slots for a single skb
(if each frag is 4098 (1+4096+1)bytes, only the last one is around 4000
bytes)
MAX_SKB_FRAGS is really number of frags, while your driver needs a count
of 'order-0' 'frames'
The use of MAX_SKB_FRAGS is a bit misleading here, it's really the max
number of slots which the other end will be willing to receive as a
single frame (in the Ethernet sense), as defined by the PV protocol. It
happens to be the same as MAX_SKB_FRAGS (or it is at least
MAX_SKB_FRAGS, I'm not too sure).
I'll nuke the dump_stack() though -- it's not clear what sort of useful
context it would contain anyway.
Ian.
From: Eric Dumazet <hidden> Date: 2012-11-20 16:14:17
At least TCP skbs are limited to 65536 bytes in tcp_sendmsg()
(around 45 1460-bytes MSS segments)
Thats probably because IPv4 stack only copes with this limit.
This probably could be relaxed for TCP friends, but this kind of skbs
should not hit a driver.
On Tue, Nov 20, 2012 at 7:44 AM, Jan Beulich [off-list ref] wrote:
quoted
quoted
quoted
On 20.11.12 at 16:06, Ian Campbell [off-list ref] wrote:
On Tue, 2012-11-20 at 14:32 +0000, Jan Beulich wrote:
quoted
quoted
@@ -517,15 +540,16 @@ static int xennet_start_xmit(struct sk_buff
*skb,
quoted
quoted
quoted
struct net_device *dev)
grant_ref_t ref;
unsigned long mfn;
int notify;
- int frags = skb_shinfo(skb)->nr_frags;
+ int slots;
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
unsigned long flags;
- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
- if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
- printk(KERN_ALERT "xennet: skb rides the rocket: %d
But still - isn't this wrong now (i.e. can't it now validly exceed the
boundary checked for)?
In practice no because of the property that the number of pages backing
the frags is <= MAX_SKB_FRAGS even if you are using compound pages as
the frags.
So are you saying that there is something in the system
preventing up to MAX_SKB_FRAGS * SKB_FRAG_PAGE_ORDER
(or NETDEV_FRAG_PAGE_MAX_ORDER) skb-s to be created? I
didn't find any. I do notice that __netdev_alloc_frag() currently
never gets called with a size larger than PAGE_SIZE, but
considering that the function just recently got made capable of
that, I'm sure respective users will show up rather sooner than
later.
Jan
On Tue, 2012-11-20 at 09:21 +0000, Ian Campbell wrote:
quoted
On Tue, 2012-11-20 at 08:30 +0000, Stefan Bader wrote:
quoted
quoted
quoted
When I tried to rebase my persistent grant netfront/netback patch on
latest kernel, netperf/netserver test never succeeded. I did some test
to find out that v3.6-rc7 works fine, but v3.7-rc1, v3.7-rc2 and
v3.7-rc4 does not succeed in netperf/netserver test. So I keep my
persistent grant patch only based on v3.4-rc3 now.
Konrad thought about commit 6a8ed462f16b8455eec5ae00eb6014159a6721f0 in
v3.7-rc1, and suggested me to test your debug patch in netfront. This
BUG_ON happens soon after running the netperf/netserver test case.
Thanks
Annie
Is there any progression with this bug (rc6 is out the door, so the
release of 3.7-final seems to be eminent and this bug completely
cripples any networking with guests) ?
+1 on that. I was testing yesterday with a PVM domU running 3.7-rc5 on Xen 4.2
(but also reported from EC2 running Xen 3.4.3) c with one VCPU. I actually can
trigger it by just ssh'ing into the domU (from another machine) and then run
"find /". Output starts to stutter and then stops completely. When this happens
a new connection still can be made and as long as only shorter output is
generated the ssh connection is ok. From a dump taken it looks like user-space
is waiting in some select call (without any warnon I rather won't see the tx path).
Annie, are you still looking into this or shall I?
I'll assume that silence == No. Will post a patch shortly.
Sorry for the delay response, I did create a patch, but did not post it out in time.
Thanks
Annie
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
An SKB paged fragment can consist of a compound page with order> 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell<redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet<redacted>
Cc: Konrad Rzeszutek Wilk<konrad@kernel.org>
Cc: ANNIE LI<redacted>
Cc: Sander Eikelenboom<redacted>
Cc: Stefan Bader<redacted>
---
drivers/net/xen-netfront.c | 58 +++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 14 deletions(-)
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;
There are following definitions at the beginning of xennet_make_frags,
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
Is it better to reuse those definitions, and not define new size and offset again in this for loop? And unsigned int is enough here, right?
quoted hunk
- tx->flags |= XEN_NETTXF_more_data;+ /* Data must not cross a page boundary. */+ BUG_ON(size + offset> PAGE_SIZE<<compound_order(page));- id = get_id_from_freelist(&np->tx_skb_freelist, np->tx_skbs);- np->tx_skbs[id].skb = skb_get(skb);- tx = RING_GET_REQUEST(&np->tx, prod++);- tx->id = id;- ref = gnttab_claim_grant_reference(&np->gref_tx_head);- BUG_ON((signed short)ref< 0);+ /* Skip unused frames from start of page */+ page += offset>> PAGE_SHIFT;+ offset&= ~PAGE_MASK;- mfn = pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));- gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,- mfn, GNTMAP_readonly);+ while (size> 0) {+ unsigned long bytes;- tx->gref = np->grant_tx_ref[id] = ref;- tx->offset = frag->page_offset;- tx->size = skb_frag_size(frag);- tx->flags = 0;+ BUG_ON(offset>= PAGE_SIZE);++ bytes = PAGE_SIZE - offset;+ if (bytes> size)+ bytes = size;++ tx->flags |= XEN_NETTXF_more_data;++ id = get_id_from_freelist(&np->tx_skb_freelist, np->tx_skbs);
From: Ian Campbell <hidden> Date: 2012-11-21 11:09:30
On Wed, 2012-11-21 at 02:52 +0000, ANNIE LI wrote:
On 2012-11-20 19:40, Ian Campbell wrote:
quoted
An SKB paged fragment can consist of a compound page with order> 0.
However the netchannel protocol deals only in PAGE_SIZE frames.
Handle this in xennet_make_frags by iterating over the frames which
make up the page.
This is the netfront equivalent to 6a8ed462f16b for netback.
Signed-off-by: Ian Campbell<redacted>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Eric Dumazet<redacted>
Cc: Konrad Rzeszutek Wilk<konrad@kernel.org>
Cc: ANNIE LI<redacted>
Cc: Sander Eikelenboom<redacted>
Cc: Stefan Bader<redacted>
---
drivers/net/xen-netfront.c | 58 +++++++++++++++++++++++++++++++++----------
1 files changed, 44 insertions(+), 14 deletions(-)
@@ -452,24 +452,54 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,/* Grant backend access to each skb fragment page. */for(i=0;i<frags;i++){skb_frag_t*frag=skb_shinfo(skb)->frags+i;+structpage*page=skb_frag_page(frag);+unsignedlongsize=skb_frag_size(frag);+unsignedlongoffset=frag->page_offset;
There are following definitions at the beginning of xennet_make_frags,
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
So they are, well spotted.
Is it better to reuse those definitions, and not define new size and
offset again in this for loop? And unsigned int is enough here, right?