From ca2f09f2b2c6c25047cfc545d057c4edfcfe561c Mon Sep 17 00:00:00 2001 From: Paul Durrant Date: Fri, 6 Dec 2013 16:36:07 +0000 Subject: xen-netback: improve guest-receive-side flow control The way that flow control works without this patch is that, in start_xmit() the code uses xenvif_count_skb_slots() to predict how many slots xenvif_gop_skb() will consume and then adds this to a 'req_cons_peek' counter which it then uses to determine if the shared ring has that amount of space available by checking whether 'req_prod' has passed that value. If the ring doesn't have space the tx queue is stopped. xenvif_gop_skb() will then consume slots and update 'req_cons' and issue responses, updating 'rsp_prod' as it goes. The frontend will consume those responses and post new requests, by updating req_prod. So, req_prod chases req_cons which chases rsp_prod, and can never exceed that value. Thus if xenvif_count_skb_slots() ever returns a number of slots greater than xenvif_gop_skb() uses, req_cons_peek will get to a value that req_prod cannot possibly achieve (since it's limited by the 'real' req_cons) and, if this happens enough times, req_cons_peek gets more than a ring size ahead of req_cons and the tx queue then remains stopped forever waiting for an unachievable amount of space to become available in the ring. Having two routines trying to calculate the same value is always going to be fragile, so this patch does away with that. All we essentially need to do is make sure that we have 'enough stuff' on our internal queue without letting it build up uncontrollably. So start_xmit() makes a cheap optimistic check of how much space is needed for an skb and only turns the queue off if that is unachievable. net_rx_action() is the place where we could do with an accurate predicition but, since that has proven tricky to calculate, a cheap worse-case (but not too bad) estimate is all we really need since the only thing we *must* prevent is xenvif_gop_skb() consuming more slots than are available. Without this patch I can trivially stall netback permanently by just doing a large guest to guest file copy between two Windows Server 2008R2 VMs on a single host. Patch tested with frontends in: - Windows Server 2008R2 - CentOS 6.0 - Debian Squeeze - Debian Wheezy - SLES11 Signed-off-by: Paul Durrant Cc: Wei Liu Cc: Ian Campbell Cc: David Vrabel Cc: Annie Li Cc: Konrad Rzeszutek Wilk Acked-by: Wei Liu Signed-off-by: David S. Miller --- drivers/net/xen-netback/common.h | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) (limited to 'drivers/net/xen-netback/common.h') diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index 08ae01b41c83..ba30a6d9fefa 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -136,12 +136,10 @@ struct xenvif { char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */ struct xen_netif_rx_back_ring rx; struct sk_buff_head rx_queue; - - /* Allow xenvif_start_xmit() to peek ahead in the rx request - * ring. This is a prediction of what rx_req_cons will be - * once all queued skbs are put on the ring. + /* Set when the RX interrupt is triggered by the frontend. + * The worker thread may need to wake the queue. */ - RING_IDX rx_req_cons_peek; + bool rx_event; /* Given MAX_BUFFER_OFFSET of 4096 the worst case is that each * head/fragment page uses 2 copy operations because it @@ -198,8 +196,6 @@ void xenvif_xenbus_fini(void); int xenvif_schedulable(struct xenvif *vif); -int xenvif_rx_ring_full(struct xenvif *vif); - int xenvif_must_stop_queue(struct xenvif *vif); /* (Un)Map communication rings. */ @@ -211,21 +207,20 @@ int xenvif_map_frontend_rings(struct xenvif *vif, /* Check for SKBs from frontend and schedule backend processing */ void xenvif_check_rx_xenvif(struct xenvif *vif); -/* Queue an SKB for transmission to the frontend */ -void xenvif_queue_tx_skb(struct xenvif *vif, struct sk_buff *skb); -/* Notify xenvif that ring now has space to send an skb to the frontend */ -void xenvif_notify_tx_completion(struct xenvif *vif); - /* Prevent the device from generating any further traffic. */ void xenvif_carrier_off(struct xenvif *vif); -/* Returns number of ring slots required to send an skb to the frontend */ -unsigned int xenvif_count_skb_slots(struct xenvif *vif, struct sk_buff *skb); - int xenvif_tx_action(struct xenvif *vif, int budget); -void xenvif_rx_action(struct xenvif *vif); int xenvif_kthread(void *data); +void xenvif_kick_thread(struct xenvif *vif); + +/* Determine whether the needed number of slots (req) are available, + * and set req_event if not. + */ +bool xenvif_rx_ring_slots_available(struct xenvif *vif, int needed); + +void xenvif_stop_queue(struct xenvif *vif); extern bool separate_tx_rx_irq; -- cgit v1.2.3 From 11b57f90257c1d6a91cee720151b69e0c2020cf6 Mon Sep 17 00:00:00 2001 From: Paul Durrant Date: Wed, 8 Jan 2014 12:41:58 +0000 Subject: xen-netback: stop vif thread spinning if frontend is unresponsive The recent patch to improve guest receive side flow control (ca2f09f2) had a slight flaw in the wait condition for the vif thread in that any remaining skbs in the guest receive side netback internal queue would prevent the thread from sleeping. An unresponsive frontend can lead to a permanently non-empty internal queue and thus the thread will spin. In this case the thread should really sleep until the frontend becomes responsive again. This patch adds an extra flag to the vif which is set if the shared ring is full and cleared when skbs are drained into the shared ring. Thus, if the thread runs, finds the shared ring full and can make no progress the flag remains set. If the flag remains set then the thread will sleep, regardless of a non-empty queue, until the next event from the frontend. Signed-off-by: Paul Durrant Cc: Wei Liu Cc: Ian Campbell Cc: David Vrabel Acked-by: Wei Liu Signed-off-by: David S. Miller --- drivers/net/xen-netback/common.h | 1 + drivers/net/xen-netback/netback.c | 14 +++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) (limited to 'drivers/net/xen-netback/common.h') diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index c955fc39d69a..4c76bcb9a879 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -143,6 +143,7 @@ struct xenvif { char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */ struct xen_netif_rx_back_ring rx; struct sk_buff_head rx_queue; + bool rx_queue_stopped; /* Set when the RX interrupt is triggered by the frontend. * The worker thread may need to wake the queue. */ diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 4f81ac0e2f0a..27385639b6e5 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -476,7 +476,8 @@ static void xenvif_rx_action(struct xenvif *vif) int ret; unsigned long offset; struct skb_cb_overlay *sco; - int need_to_notify = 0; + bool need_to_notify = false; + bool ring_full = false; struct netrx_pending_operations npo = { .copy = vif->grant_copy_op, @@ -508,7 +509,8 @@ static void xenvif_rx_action(struct xenvif *vif) /* If the skb may not fit then bail out now */ if (!xenvif_rx_ring_slots_available(vif, max_slots_needed)) { skb_queue_head(&vif->rx_queue, skb); - need_to_notify = 1; + need_to_notify = true; + ring_full = true; break; } @@ -521,6 +523,8 @@ static void xenvif_rx_action(struct xenvif *vif) BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta)); + vif->rx_queue_stopped = !npo.copy_prod && ring_full; + if (!npo.copy_prod) goto done; @@ -592,8 +596,7 @@ static void xenvif_rx_action(struct xenvif *vif) RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret); - if (ret) - need_to_notify = 1; + need_to_notify |= !!ret; npo.meta_cons += sco->meta_slots_used; dev_kfree_skb(skb); @@ -1724,7 +1727,8 @@ static struct xen_netif_rx_response *make_rx_response(struct xenvif *vif, static inline int rx_work_todo(struct xenvif *vif) { - return !skb_queue_empty(&vif->rx_queue) || vif->rx_event; + return (!skb_queue_empty(&vif->rx_queue) && !vif->rx_queue_stopped) || + vif->rx_event; } static inline int tx_work_todo(struct xenvif *vif) -- cgit v1.2.3