[PATCH net v3 0/2] xen-netback: napi fixes

[PATCH net v3 0/2] xen-netback: napi fixes

[Paul Durrant]

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

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

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

[PATCH net v3 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.

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 v3 2/2] xen-netback: napi: don't prematurely request a tx event

[Malcolm Crossley]

On 10/12/13 13:39, Paul Durrant wrote:
> 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.
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;
>   

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

[Ian Campbell]

On Tue, 2013-12-10 at 13:39 +0000, Paul Durrant wrote:
> 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.

Like I said last time:

Can you add a reminder of when/where the ring event pointer is
eventually advanced now please.

> 
> 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;
>  

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

[Ian Campbell]

On Tue, 2013-12-10 at 13:39 +0000, Paul Durrant wrote:
> 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.

Like I said last time:

Can you add a reminder of when/where the ring event pointer is
eventually advanced now please.

> 
> 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;
>  

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

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 11 December 2013 09:44
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel
> Subject: Re: [PATCH net v3 2/2] xen-netback: napi: don't prematurely
> request a tx event
> 
> On Tue, 2013-12-10 at 13:39 +0000, Paul Durrant wrote:
> > 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.
> 
> Like I said last time:
> 
> Can you add a reminder of when/where the ring event pointer is
> eventually advanced now please.
> 

Sorry, I missed that. Will do.

  Paul

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

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 11 December 2013 09:44
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel
> Subject: Re: [PATCH net v3 2/2] xen-netback: napi: don't prematurely
> request a tx event
> 
> On Tue, 2013-12-10 at 13:39 +0000, Paul Durrant wrote:
> > 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.
> 
> Like I said last time:
> 
> Can you add a reminder of when/where the ring event pointer is
> eventually advanced now please.
> 

Sorry, I missed that. Will do.

  Paul

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

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Malcolm Crossley
> Sent: 10 December 2013 17:47
> To: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH net v3 2/2] xen-netback: napi: don't
> prematurely request a tx event
> 
> On 10/12/13 13:39, Paul Durrant wrote:
> > 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.
> Reported-by: Malcolm Crossley <malcolm.crossley@citrix.com>

Apologies. Indeed I should have added that to my commit comment.

  Paul

> > 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;
> >
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel