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

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

[Vineeth Remanan Pillai]

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 <vineethp@amazon.com>
---
 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

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

[David Miller]

From: Vineeth Remanan Pillai <vineethp@amazon.com>
Date: Wed, 11 Jan 2017 23:17:17 +0000

> @@ -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.

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

[Vineeth Remanan Pillai]

On 01/12/2017 12:17 PM, David Miller wrote:
> From: Vineeth Remanan Pillai <vineethp@amazon.com>
> Date: Wed, 11 Jan 2017 23:17:17 +0000
>
>> @@ -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

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

[Juergen Gross]

On 13/01/17 18:55, Remanan Pillai wrote:
> From: Vineeth Remanan Pillai <vineethp@amazon.com>
> 
> 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 <vineethp@amazon.com>

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


Thanks,

Juergen

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

[Vineeth Remanan Pillai]

On 01/15/2017 10:24 PM, Juergen Gross wrote:
> On 13/01/17 18:55, Remanan Pillai wrote:
>> From: Vineeth Remanan Pillai <vineethp@amazon.com>
>>
>> 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 <vineethp@amazon.com>
> 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
>

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

[David Miller]

From: Vineeth Remanan Pillai <vineethp@amazon.com>
Date: Wed, 18 Jan 2017 09:02:17 -0800

> 
> On 01/15/2017 10:24 PM, Juergen Gross wrote:
>> On 13/01/17 18:55, Remanan Pillai wrote:
>>> From: Vineeth Remanan Pillai <vineethp@amazon.com>
>>>
>>> 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 <vineethp@amazon.com>
>> 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?

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

[David Miller]

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.

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

[Vineeth Remanan Pillai]

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