[PATCH] xen-netfront: Fix Rx stall during network stress and OOM

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

Fix is to consider the skb allocation failure as an error and in the
event of this error, re-enable polling so that request slots could be
created when memory is available.

Signed-off-by: Vineeth Remanan Pillai <redacted>
---
 drivers/net/xen-netfront.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..8275549 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c

@@ -277,13 +277,14 @@ static struct sk_buff *xennet_alloc_one_rx_buffer(struct netfront_queue *queue)
 }
 
 
-static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
+static int xennet_alloc_rx_buffers(struct netfront_queue *queue)
 {
 	RING_IDX req_prod = queue->rx.req_prod_pvt;
 	int notify;
+	int err = 0;
 
 	if (unlikely(!netif_carrier_ok(queue->info->netdev)))
-		return;
+		return err;
 
 	for (req_prod = queue->rx.req_prod_pvt;
 	     req_prod - queue->rx.rsp_cons < NET_RX_RING_SIZE;

@@ -295,8 +296,10 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 		struct xen_netif_rx_request *req;
 
 		skb = xennet_alloc_one_rx_buffer(queue);
-		if (!skb)
+		if (!skb) {
+			err = -ENOMEM;
 			break;
+		}
 
 		id = xennet_rxidx(req_prod);
 

@@ -321,9 +324,9 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 	queue->rx.req_prod_pvt = req_prod;
 
 	/* Not enough requests? Try again later. */
-	if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
+	if (req_prod - queue->rx.sring->rsp_prod < NET_RX_SLOTS_MIN) {
 		mod_timer(&queue->rx_refill_timer, jiffies + (HZ/10));
-		return;
+		return err;
 	}
 
 	wmb();		/* barrier so backend seens requests */

@@ -331,6 +334,8 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&queue->rx, notify);
 	if (notify)
 		notify_remote_via_irq(queue->rx_irq);
+
+	return err;
 }
 
 static int xennet_open(struct net_device *dev)

@@ -1046,7 +1051,7 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 
 	work_done -= handle_incoming_queue(queue, &rxq);
 
-	xennet_alloc_rx_buffers(queue);
+	err = xennet_alloc_rx_buffers(queue);
 
 	if (work_done < budget) {
 		int more_to_do = 0;

@@ -1054,7 +1059,11 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 		napi_complete(napi);
 
 		RING_FINAL_CHECK_FOR_RESPONSES(&queue->rx, more_to_do);
-		if (more_to_do)
+
+		/* If there is more work to do or could not allocate
+		 * rx buffers, re-enable polling.
+		 */
+		if (more_to_do || err != 0)
 			napi_schedule(napi);
 	}
 

-- 
2.1.2.AMZN

From: Vineeth Remanan Pillai <redacted>
Date: Wed, 11 Jan 2017 23:17:17 +0000

Just polling endlessly in a loop retrying the SKB allocation over and over
again until it succeeds is not very nice behavior.

You already have that refill timer, so please use that to retry instead
of wasting cpu cycles looping in NAPI poll.

Thanks.

On 01/12/2017 12:17 PM, David Miller wrote:

From: Vineeth Remanan Pillai <redacted>
Date: Wed, 11 Jan 2017 23:17:17 +0000

quoted

@@ -1054,7 +1059,11 @@ static int xennet_poll(struct napi_struct *napi, int budget)
  		napi_complete(napi);
  
  		RING_FINAL_CHECK_FOR_RESPONSES(&queue->rx, more_to_do);
-		if (more_to_do)
+
+		/* If there is more work to do or could not allocate
+		 * rx buffers, re-enable polling.
+		 */
+		if (more_to_do || err != 0)
  			napi_schedule(napi);

Just polling endlessly in a loop retrying the SKB allocation over and over
again until it succeeds is not very nice behavior.

You already have that refill timer, so please use that to retry instead
of wasting cpu cycles looping in NAPI poll.

Thanks Dave for the inputs.
On further look, I think I can fix it much simpler by correcting the 
test condition
for minimum slots for pushing requests. Existing test is like this:

<snip>
         /* Not enough requests? Try again later. */
        if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
                 mod_timer(&queue->rx_refill_timer, jiffies + (HZ/10));
                 return;
         }
</snip>

Actually the above check counts more than the newly created request slots
as it counts from rsp_cons. The actual count should be the difference 
between
new req_prod and old req_prod(in the queue). If skbs cannot be created, 
this
count remains small and hence we would schedule the timer. So the fix 
could be:

         /* Not enough requests? Try again later. */
-       if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
+       if (req_prod - queue->rx.sring->req_prod < NET_RX_SLOTS_MIN) {


I have done some initial testing to verify the fix. Will send out v2 
patch after couple
more round of testing.

Thanks,
Vineeth

From: Vineeth Remanan Pillai <redacted>

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

The issue is with the retry logic which schedules the timer if the
created slots are less than NET_RX_SLOTS_MIN. The count of new request
slots to be pushed are calculated as a difference between new req_prod
and rsp_cons which could be more than the actual slots, if there are
unconsumed responses.

The fix is to calculate the count of newly created slots as the
difference between new req_prod and old req_prod.

Signed-off-by: Vineeth Remanan Pillai <redacted>
---
Changes in v2:
	- Removed the old implementation of enabling polling on
	  skb allocation error.
	- Corrected the refill timer logic to schedule when newly
	  created slots since last push is less than NET_RX_SLOTS_MIN.

 drivers/net/xen-netfront.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..2c7c29f 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c

@@ -321,7 +321,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 	queue->rx.req_prod_pvt = req_prod;
 
 	/* Not enough requests? Try again later. */
-	if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
+	if (req_prod - queue->rx.sring->req_prod < NET_RX_SLOTS_MIN) {
 		mod_timer(&queue->rx_refill_timer, jiffies + (HZ/10));
 		return;
 	}

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On 13/01/17 18:55, Remanan Pillai wrote:

From: Vineeth Remanan Pillai <redacted>

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

The issue is with the retry logic which schedules the timer if the
created slots are less than NET_RX_SLOTS_MIN. The count of new request
slots to be pushed are calculated as a difference between new req_prod
and rsp_cons which could be more than the actual slots, if there are
unconsumed responses.

The fix is to calculate the count of newly created slots as the
difference between new req_prod and old req_prod.

Signed-off-by: Vineeth Remanan Pillai <redacted>

Reviewed-by: Juergen Gross <jgross@suse.com>


Thanks,

Juergen

On 01/15/2017 10:24 PM, Juergen Gross wrote:

On 13/01/17 18:55, Remanan Pillai wrote:

quoted

From: Vineeth Remanan Pillai <redacted>

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

The issue is with the retry logic which schedules the timer if the
created slots are less than NET_RX_SLOTS_MIN. The count of new request
slots to be pushed are calculated as a difference between new req_prod
and rsp_cons which could be more than the actual slots, if there are
unconsumed responses.

The fix is to calculate the count of newly created slots as the
difference between new req_prod and old req_prod.

Signed-off-by: Vineeth Remanan Pillai <redacted>

Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks Juergen.

David,

Could you please pick up this change for net-next if there no more 
concerns..

Many Thanks,
Vineeth

Thanks,

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

This v2 never made it into patchwork.  I don't know why, so please resend it to
netdev with the accumulated reviewed-by etc. tags added.

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On 01/18/2017 12:10 PM, David Miller wrote:

This v2 never made it into patchwork.  I don't know why, so please resend it to
netdev with the accumulated reviewed-by etc. tags added.

Thanks.

Sorry about that. Will resend as a separate thread right away.

Thanks


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

From: Vineeth Remanan Pillai <redacted>
Date: Wed, 18 Jan 2017 09:02:17 -0800

On 01/15/2017 10:24 PM, Juergen Gross wrote:

quoted

On 13/01/17 18:55, Remanan Pillai wrote:

quoted

From: Vineeth Remanan Pillai <redacted>

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

The issue is with the retry logic which schedules the timer if the
created slots are less than NET_RX_SLOTS_MIN. The count of new request
slots to be pushed are calculated as a difference between new req_prod
and rsp_cons which could be more than the actual slots, if there are
unconsumed responses.

The fix is to calculate the count of newly created slots as the
difference between new req_prod and old req_prod.

Signed-off-by: Vineeth Remanan Pillai <redacted>

Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks Juergen.

David,

Could you please pick up this change for net-next if there no more
concerns..

Why would I pick up "this change", if the author of the patch has
stated that he will resubmit the change implemented differently based
upon my feedback?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel