bridge: orphan frags on local receive
Subsystems (4)
PATCH 1/1 bridge: orphan frags on local receive
with vhost tx zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when a LAST_ACK state tcp occuring between guest and host.
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
net/bridge/br_input.c +3 -0
@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;
@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; }
Signature · 1 line
1.7.3.1.msysgit.0
Quoted 6 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>
Do you actually observe guest hang?
I would expect orphan frags in__netif_receive_skb_core to be enough.
Quoted 27 lines
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +3
-0@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; }Signature · 1 line
1.7.3.1.msysgit.0
Quoted 10 lines · Michael S. Tsirkin
Quoted 6 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>Do you actually observe guest hang?
yes, guest nic could not xmit any more until the skb holded by host has been freed, the mainly reason is that though virtio-net could use vring desc out of order, but ubufs is been used by vhost in order. so only one skb could cause guest nic hang.
Quoted 3 lines
I would expect orphan frags in
__netif_receive_skb_coreto be enough.
yes, it would be better. I would deliver another patch soon.
Quoted 30 lines
Quoted 28 lines
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +3
-1@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; } --1.7.3.1.msysgit.0
Quoted 11 lines · Qin Chuanyu
Quoted 10 lines · Michael S. Tsirkin
Quoted 6 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>Do you actually observe guest hang?
Quoted 9 lines
yes, guest nic could not xmit any more until the skb holded by host has been freed, the mainly reason is that though virtio-net could use vring desc out of order, but ubufs is been used by vhost in order. so only one skb could cause guest nic hang.
Quoted 3 lines
I would expect orphan frags in
__netif_receive_skb_coreto be enough.yes, it would be better. I would deliver another patch soon.
Quoted 1 line
After thinking for a while, I think that orphan frags could only been involved in virtual nic receive path, because after __netif_receive_skb_core, skb would been transmitted by physic nic, in this way orphan frags is unnecessary.
Quoted 37 lines
Quoted 19 lines
Quoted 19 lines
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +2
-0@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:Quoted 10 lines
Quoted 8 lines
net/bridge/br_input.c +1
-1return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; } --1.7.3.1.msysgit.0
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Qin Chuanyu
Quoted 10 lines · Michael S. Tsirkin
Quoted 6 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>Do you actually observe guest hang?
Quoted 8 lines
yes, guest nic could not xmit any more until the skb holded by host has been freed, the mainly reason is that though virtio-net could use vring desc out of order, but ubufs is been used by vhost in order. so only one skb could cause guest nic hang.
Quoted 3 lines
I would expect orphan frags in
__netif_receive_skb_coreto be enough.yes, it would be better. I would deliver another patch soon.
well __netif_receive_skb_core already calls orphan frags already. What's left to do?
Quoted 31 lines
Quoted 30 lines
Quoted 28 lines
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +3
-1@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; } --1.7.3.1.msysgit.0
Quoted 16 lines · Michael S. Tsirkin
Quoted 15 lines · Qin Chuanyu
Quoted 10 lines · Michael S. Tsirkin
Quoted 6 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>Do you actually observe guest hang?
yes, guest nic could not xmit any more until the skb holded by host has been freed, the mainly reason is that though virtio-net could use vring desc out of order, but ubufs is been used by vhost in order. so only one skb could cause guest nic hang.
Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
I would expect orphan frags in
__netif_receive_skb_coreto be enough.yes, it would be better. I would deliver another patch soon.
well
__netif_receive_skb_corealready calls orphan frags already. What's left to do?
None :) , I will use with this change.
Quoted 34 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +2
-0@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop:Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
net/bridge/br_input.c +1
-1+skb_tx_error (skb);kfree_skb (skb); goto out; } --1.7.3.1.msysgit.0
.
Quoted 24 lines · Qin Chuanyu
with vhost tx
zero_copy, guest nic might get hang when host reserving skb in socket queue delivered by guest, the case has been solved in tun, it also been needed by bridge. This could easily happened when aLAST_ACKstate tcp occuring between guest and host.Signed-off-by Chuanyu Qin <[email protected]>--- net/bridge/br_input.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)net/bridge/br_input.c +2
-0@@ -117,6 +117,8 @@ int br_handle_frame_finish(struct sk_buff *skb) br->dev->stats.multicast++; }else if ((dst =__br_fdb_get (br, dest, vid)) && dst->is_local) { +if (unlikely (skb_orphan_frags (skb,GFP_ATOMIC ))) + goto drop; skb2 = skb;/* Do not forward the packet since it's local. */ skb =NULL ;
net/bridge/br_input.c int br_handle_frame_finish(struct sk_buff *skb) · line 144
Quoted 7 lines
net/bridge/br_input.c +1
-0@@ -136,6 +138,7 @@ int br_handle_frame_finish(struct sk_buff *skb) out:return 0 ; drop: +skb_tx_error (skb);kfree_skb (skb); goto out; }
Pretty similar to the issue we found for non-work conserving qdiscs. The questions is whether or not should we audit all such cases ( I believe we could find even more ) and does the skb_orphan_frags(), or doing something like switch to use data copy in this case
I will post a patch that switch to use data copy when the number of pending DMAs exceed a limit. Looks like it can help here.
Another question is whether or nor do we need a skb_orphan() here now (at least for packets from tun) ?