[PATCH net 0/3] xen-netback: fix rx slot estimation

[PATCH net 0/3] xen-netback: fix rx slot estimation

[Paul Durrant]

Sander Eikelenboom reported an issue with ring overflow netback in 3.14-rc3.
This turns outo be be because of a bug in the ring slot estimation code.
This patch series fixes the slot estimation, fixes the BUG_ON() that was
supposed to catch the issue that Sander ran into and also makes a small
fix to start_new_rx_buffer().

[PATCH net 1/3] xen-netback: remove pointless clause from if statement

[Paul Durrant]

This patch removes a test in start_new_rx_buffer() that checks whether
a copy operation is less than MAX_BUFFER_OFFSET in length, since
MAX_BUFFER_OFFSET is defined to be PAGE_SIZE and the only caller of
start_new_rx_buffer() already limits copy operations to PAGE_SIZE or less.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Sander Eikelenboom <linux@eikelenboom.it>
---
 drivers/net/xen-netback/netback.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 438d0c0..befc413 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -192,8 +192,7 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head)
 	 * into multiple copies tend to give large frags their
 	 * own buffers as before.
 	 */
-	if ((offset + size > MAX_BUFFER_OFFSET) &&
-	    (size <= MAX_BUFFER_OFFSET) && offset && !head)
+	if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
 		return true;
 
 	return false;
-- 
1.7.10.4

Re: [PATCH net 1/3] xen-netback: remove pointless clause from if statement

[Ian Campbell]

On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote:
> This patch removes a test in start_new_rx_buffer() that checks whether
> a copy operation is less than MAX_BUFFER_OFFSET in length, since
> MAX_BUFFER_OFFSET is defined to be PAGE_SIZE and the only caller of
> start_new_rx_buffer() already limits copy operations to PAGE_SIZE or less.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

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

Although it would have been nice to consider a BUG_ON or ASSERT.

> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sander Eikelenboom <linux@eikelenboom.it>
> ---
>  drivers/net/xen-netback/netback.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 438d0c0..befc413 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -192,8 +192,7 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head)
>  	 * into multiple copies tend to give large frags their
>  	 * own buffers as before.
>  	 */
> -	if ((offset + size > MAX_BUFFER_OFFSET) &&
> -	    (size <= MAX_BUFFER_OFFSET) && offset && !head)
> +	if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
>  		return true;
>  
>  	return false;

Re: [PATCH net 1/3] xen-netback: remove pointless clause from if statement

[Sander Eikelenboom]

Thursday, March 27, 2014, 1:23:06 PM, you wrote:

> This patch removes a test in start_new_rx_buffer() that checks whether
> a copy operation is less than MAX_BUFFER_OFFSET in length, since
> MAX_BUFFER_OFFSET is defined to be PAGE_SIZE and the only caller of
> start_new_rx_buffer() already limits copy operations to PAGE_SIZE or less.

> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sander Eikelenboom <linux@eikelenboom.it>
> ---
>  drivers/net/xen-netback/netback.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 438d0c0..befc413 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -192,8 +192,7 @@ static bool start_new_rx_buffer(int offset, unsigned long size, int head)
>          * into multiple copies tend to give large frags their
>          * own buffers as before.
>          */
> -       if ((offset + size > MAX_BUFFER_OFFSET) &&
> -           (size <= MAX_BUFFER_OFFSET) && offset && !head)
> +       if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
>                 return true;
>  
>         return false;


Will put this through the test now ..

--
Sander

[PATCH net 2/3] xen-netback: worse-case estimate in xenvif_rx_action is underestimating

[Paul Durrant]

The worse-case estimate for skb ring slot usage in xenvif_rx_action()
fails to take fragment page_offset into account. The page_offset does,
however, affect the number of times the fragmentation code calls
start_new_rx_buffer() (i.e. consume another slot) and the worse-case
should assume that will always return true. This patch adds the page_offset
into the DIV_ROUND_UP for each frag.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Sander Eikelenboom <linux@eikelenboom.it>
---
 drivers/net/xen-netback/netback.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index befc413..ac35489 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif)
 						PAGE_SIZE);
 		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
 			unsigned int size;
+			unsigned int offset;
+
 			size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
-			max_slots_needed += DIV_ROUND_UP(size, PAGE_SIZE);
+			offset = skb_shinfo(skb)->frags[i].page_offset;
+
+			/* For a worse-case estimate we need to factor in
+			 * the fragment page offset as this will affect the
+			 * number of times xenvif_gop_frag_copy() will
+			 * call start_new_rx_buffer().
+			 */
+			max_slots_needed += DIV_ROUND_UP(offset + size,
+							 PAGE_SIZE);
 		}
 		if (skb_is_gso(skb) &&
 		   (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 ||
-- 
1.7.10.4

Re: [PATCH net 2/3] xen-netback: worse-case estimate in xenvif_rx_action is underestimating

[Ian Campbell]

On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote:
> The worse-case estimate for skb ring slot usage in xenvif_rx_action()
> fails to take fragment page_offset into account. The page_offset does,
> however, affect the number of times the fragmentation code calls
> start_new_rx_buffer() (i.e. consume another slot) and the worse-case
> should assume that will always return true. This patch adds the page_offset
> into the DIV_ROUND_UP for each frag.

At least for the copying mode wasn't the idea that you would copy to the
start of the page, so the offset wasn't relevant? IOW is the real issue
that start_new_rx_buffer is/was too aggressive?

Now that we do mapping though I suspect the offset becomes relevant
again here and there is a 1:1 mapping from slots to frags again.

(I could have sworn David V got rid of all this precalculating stuff.)

> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sander Eikelenboom <linux@eikelenboom.it>
> ---
>  drivers/net/xen-netback/netback.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index befc413..ac35489 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif)
>  						PAGE_SIZE);
>  		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>  			unsigned int size;
> +			unsigned int offset;
> +
>  			size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
> -			max_slots_needed += DIV_ROUND_UP(size, PAGE_SIZE);
> +			offset = skb_shinfo(skb)->frags[i].page_offset;
> +
> +			/* For a worse-case estimate we need to factor in
> +			 * the fragment page offset as this will affect the
> +			 * number of times xenvif_gop_frag_copy() will
> +			 * call start_new_rx_buffer().
> +			 */
> +			max_slots_needed += DIV_ROUND_UP(offset + size,
> +							 PAGE_SIZE);
>  		}
>  		if (skb_is_gso(skb) &&
>  		   (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 ||

RE: [PATCH net 2/3] xen-netback: worse-case estimate in xenvif_rx_action is underestimating

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 27 March 2014 12:28
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Sander
> Eikelenboom
> Subject: Re: [PATCH net 2/3] xen-netback: worse-case estimate in
> xenvif_rx_action is underestimating
> 
> On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote:
> > The worse-case estimate for skb ring slot usage in xenvif_rx_action()
> > fails to take fragment page_offset into account. The page_offset does,
> > however, affect the number of times the fragmentation code calls
> > start_new_rx_buffer() (i.e. consume another slot) and the worse-case
> > should assume that will always return true. This patch adds the page_offset
> > into the DIV_ROUND_UP for each frag.
> 
> At least for the copying mode wasn't the idea that you would copy to the
> start of the page, so the offset wasn't relevant? IOW is the real issue
> that start_new_rx_buffer is/was too aggressive?
> 
> Now that we do mapping though I suspect the offset becomes relevant
> again here and there is a 1:1 mapping from slots to frags again.
> 

We're always in copying mode. This is guest receive side :-)

> (I could have sworn David V got rid of all this precalculating stuff.)
> 

He did modify it. I got rid of it in favour of the best-case and worse-case estimations.

  Paul

> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Sander Eikelenboom <linux@eikelenboom.it>
> > ---
> >  drivers/net/xen-netback/netback.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> netback/netback.c
> > index befc413..ac35489 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -492,8 +492,18 @@ static void xenvif_rx_action(struct xenvif *vif)
> >  						PAGE_SIZE);
> >  		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> >  			unsigned int size;
> > +			unsigned int offset;
> > +
> >  			size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
> > -			max_slots_needed += DIV_ROUND_UP(size,
> PAGE_SIZE);
> > +			offset = skb_shinfo(skb)->frags[i].page_offset;
> > +
> > +			/* For a worse-case estimate we need to factor in
> > +			 * the fragment page offset as this will affect the
> > +			 * number of times xenvif_gop_frag_copy() will
> > +			 * call start_new_rx_buffer().
> > +			 */
> > +			max_slots_needed += DIV_ROUND_UP(offset + size,
> > +							 PAGE_SIZE);
> >  		}
> >  		if (skb_is_gso(skb) &&
> >  		   (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 ||
> 

[PATCH net 3/3] xen-netback: BUG_ON in xenvif_rx_action() not catching overflow

[Paul Durrant]

The BUG_ON to catch ring overflow in xenvif_rx_action() makes the assumption
that meta_slots_used == ring slots used. This is not the case for GSO
packets. This patch changes the test to actually check ring slots.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Sander Eikelenboom <linux@eikelenboom.it>
---
 drivers/net/xen-netback/netback.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index ac35489..ae7351f 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -481,6 +481,8 @@ static void xenvif_rx_action(struct xenvif *vif)
 
 	while ((skb = skb_dequeue(&vif->rx_queue)) != NULL) {
 		RING_IDX max_slots_needed;
+		RING_IDX old_req_cons;
+		RING_IDX ring_slots_used;
 		int i;
 
 		/* We need a cheap worse case estimate for the number of
@@ -520,8 +522,12 @@ static void xenvif_rx_action(struct xenvif *vif)
 			vif->rx_last_skb_slots = 0;
 
 		sco = (struct skb_cb_overlay *)skb->cb;
+
+		old_req_cons = vif->rx.req_cons;
 		sco->meta_slots_used = xenvif_gop_skb(skb, &npo);
-		BUG_ON(sco->meta_slots_used > max_slots_needed);
+		ring_slots_used = vif->rx.req_cons - old_req_cons;
+
+		BUG_ON(ring_slots_used > max_slots_needed);
 
 		__skb_queue_tail(&rxq, skb);
 	}
-- 
1.7.10.4

Re: [PATCH net 3/3] xen-netback: BUG_ON in xenvif_rx_action() not catching overflow

[Ian Campbell]

On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote:
> The BUG_ON to catch ring overflow in xenvif_rx_action() makes the assumption
> that meta_slots_used == ring slots used. This is not the case for GSO
> packets.

Can you explain why not here please.

>  This patch changes the test to actually check ring slots.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sander Eikelenboom <linux@eikelenboom.it>
> ---
>  drivers/net/xen-netback/netback.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index ac35489..ae7351f 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -481,6 +481,8 @@ static void xenvif_rx_action(struct xenvif *vif)
>  
>  	while ((skb = skb_dequeue(&vif->rx_queue)) != NULL) {
>  		RING_IDX max_slots_needed;
> +		RING_IDX old_req_cons;
> +		RING_IDX ring_slots_used;
>  		int i;
>  
>  		/* We need a cheap worse case estimate for the number of
> @@ -520,8 +522,12 @@ static void xenvif_rx_action(struct xenvif *vif)
>  			vif->rx_last_skb_slots = 0;
>  
>  		sco = (struct skb_cb_overlay *)skb->cb;
> +
> +		old_req_cons = vif->rx.req_cons;
>  		sco->meta_slots_used = xenvif_gop_skb(skb, &npo);
> -		BUG_ON(sco->meta_slots_used > max_slots_needed);
> +		ring_slots_used = vif->rx.req_cons - old_req_cons;
> +
> +		BUG_ON(ring_slots_used > max_slots_needed);
>  
>  		__skb_queue_tail(&rxq, skb);
>  	}

RE: [PATCH net 3/3] xen-netback: BUG_ON in xenvif_rx_action() not catching overflow

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 27 March 2014 12:28
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Sander
> Eikelenboom
> Subject: Re: [PATCH net 3/3] xen-netback: BUG_ON in xenvif_rx_action()
> not catching overflow
> 
> On Thu, 2014-03-27 at 12:23 +0000, Paul Durrant wrote:
> > The BUG_ON to catch ring overflow in xenvif_rx_action() makes the
> assumption
> > that meta_slots_used == ring slots used. This is not the case for GSO
> > packets.
> 
> Can you explain why not here please.
>

Sure. I'll add an explanation.

  Paul
 
> >  This patch changes the test to actually check ring slots.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Sander Eikelenboom <linux@eikelenboom.it>
> > ---
> >  drivers/net/xen-netback/netback.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> netback/netback.c
> > index ac35489..ae7351f 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -481,6 +481,8 @@ static void xenvif_rx_action(struct xenvif *vif)
> >
> >  	while ((skb = skb_dequeue(&vif->rx_queue)) != NULL) {
> >  		RING_IDX max_slots_needed;
> > +		RING_IDX old_req_cons;
> > +		RING_IDX ring_slots_used;
> >  		int i;
> >
> >  		/* We need a cheap worse case estimate for the number of
> > @@ -520,8 +522,12 @@ static void xenvif_rx_action(struct xenvif *vif)
> >  			vif->rx_last_skb_slots = 0;
> >
> >  		sco = (struct skb_cb_overlay *)skb->cb;
> > +
> > +		old_req_cons = vif->rx.req_cons;
> >  		sco->meta_slots_used = xenvif_gop_skb(skb, &npo);
> > -		BUG_ON(sco->meta_slots_used > max_slots_needed);
> > +		ring_slots_used = vif->rx.req_cons - old_req_cons;
> > +
> > +		BUG_ON(ring_slots_used > max_slots_needed);
> >
> >  		__skb_queue_tail(&rxq, skb);
> >  	}
>