[PATCH net v4 0/2] napi fixes

[PATCH net v4 0/2] napi fixes

[Paul Durrant]

This 2 patch series is essentially v4 of "xen-netback: fix abuse of napi
budget".

v4:
- Add missing 'Reported-by' to patch 2 comment
- Add explanation of where the ring event pointer is update to patch 2
  comment

v3:
- Break into two patches
- Don't lose the work_to_do value in xenvif_build_tx_gops

v2:
- More elaborate description of the problem and the fix in the comment.
- Modified the check in xenvif_build_tx_gops.

[PATCH net v4 1/2] xen-netback: napi: fix abuse of budget

[Paul Durrant]

netback seems to be somewhat confused about the napi budget parameter. The
parameter is supposed to limit the number of skbs processed in each poll,
but netback has this confused with grant operations.

This patch fixes that, properly limiting the work done in each poll. Note
that this limit makes sure we do not process any more data from the shared
ring than we intend to pass back from the poll. This is important to
prevent tx_queue potentially growing without bound.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 43341b8..83b4e5b 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1351,14 +1351,15 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
 	return false;
 }
 
-static unsigned xenvif_tx_build_gops(struct xenvif *vif)
+static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget)
 {
 	struct gnttab_copy *gop = vif->tx_copy_ops, *request_gop;
 	struct sk_buff *skb;
 	int ret;
 
 	while ((nr_pending_reqs(vif) + XEN_NETBK_LEGACY_SLOTS_MAX
-		< MAX_PENDING_REQS)) {
+		< MAX_PENDING_REQS) &&
+	       (skb_queue_len(&vif->tx_queue) < budget)) {
 		struct xen_netif_tx_request txreq;
 		struct xen_netif_tx_request txfrags[XEN_NETBK_LEGACY_SLOTS_MAX];
 		struct page *page;
@@ -1520,14 +1521,13 @@ static unsigned xenvif_tx_build_gops(struct xenvif *vif)
 }
 
 
-static int xenvif_tx_submit(struct xenvif *vif, int budget)
+static int xenvif_tx_submit(struct xenvif *vif)
 {
 	struct gnttab_copy *gop = vif->tx_copy_ops;
 	struct sk_buff *skb;
 	int work_done = 0;
 
-	while (work_done < budget &&
-	       (skb = __skb_dequeue(&vif->tx_queue)) != NULL) {
+	while ((skb = __skb_dequeue(&vif->tx_queue)) != NULL) {
 		struct xen_netif_tx_request *txp;
 		u16 pending_idx;
 		unsigned data_len;
@@ -1602,14 +1602,14 @@ int xenvif_tx_action(struct xenvif *vif, int budget)
 	if (unlikely(!tx_work_todo(vif)))
 		return 0;
 
-	nr_gops = xenvif_tx_build_gops(vif);
+	nr_gops = xenvif_tx_build_gops(vif, budget);
 
 	if (nr_gops == 0)
 		return 0;
 
 	gnttab_batch_copy(vif->tx_copy_ops, nr_gops);
 
-	work_done = xenvif_tx_submit(vif, nr_gops);
+	work_done = xenvif_tx_submit(vif);
 
 	return work_done;
 }
-- 
1.7.10.4

[PATCH net v4 2/2] xen-netback: napi: don't prematurely request a tx event

[Paul Durrant]

This patch changes the RING_FINAL_CHECK_FOR_REQUESTS in
xenvif_build_tx_gops to a check for RING_HAS_UNCONSUMED_REQUESTS as the
former call has the side effect of advancing the ring event pointer and
therefore inviting another interrupt from the frontend before the napi
poll has actually finished, thereby defeating the point of napi.

The event pointer is updated by RING_FINAL_CHECK_FOR_REQUESTS in
xenvif_poll, the napi poll function, if the work done is less than the
budget i.e. when actually transitioning back to interrupt mode.

Reported-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 83b4e5b..fb512a9 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1381,7 +1381,7 @@ static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget)
 			continue;
 		}
 
-		RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, work_to_do);
+		work_to_do = RING_HAS_UNCONSUMED_REQUESTS(&vif->tx);
 		if (!work_to_do)
 			break;
 
-- 
1.7.10.4

Re: [PATCH net v4 0/2] napi fixes

[David Miller]

From: Paul Durrant <paul.durrant@citrix.com>
Date: Wed, 11 Dec 2013 10:57:14 +0000

> This 2 patch series is essentially v4 of "xen-netback: fix abuse of napi
> budget".

Does everyone feel like their feedback was addressed and this series
is good to go?

Thanks.

Re: [Xen-devel] [PATCH net v4 0/2] napi fixes

[Ian Campbell]

On Wed, 2013-12-11 at 16:49 -0500, David Miller wrote:
> From: Paul Durrant <paul.durrant@citrix.com>
> Date: Wed, 11 Dec 2013 10:57:14 +0000
> 
> > This 2 patch series is essentially v4 of "xen-netback: fix abuse of napi
> > budget".
> 
> Does everyone feel like their feedback was addressed and this series
> is good to go?

I'm happy -- 

Acked-by: Ian Campbell <ian.campbell@citrix.com>

for both.

Thanks,

Ian.

Re: [PATCH net v4 0/2] napi fixes

[David Miller]

From: Paul Durrant <paul.durrant@citrix.com>
Date: Wed, 11 Dec 2013 10:57:14 +0000

> This 2 patch series is essentially v4 of "xen-netback: fix abuse of napi
> budget".
> 
> v4:
> - Add missing 'Reported-by' to patch 2 comment
> - Add explanation of where the ring event pointer is update to patch 2
>   comment
> 
> v3:
> - Break into two patches
> - Don't lose the work_to_do value in xenvif_build_tx_gops
> 
> v2:
> - More elaborate description of the problem and the fix in the comment.
> - Modified the check in xenvif_build_tx_gops.

These look fine to me, applied, thanks.