| Message ID | 1484176637-2869-1-git-send-email-vineethp@amazon.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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
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 >
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?
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.
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
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); }
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(-)