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

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

[Paul Durrant]

Sander Eikelenboom reported an issue with ring overflow in 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().

v3:
 - Added a cap of MAX_SKB_FRAGS to estimate in patch #2

v2:
 - Added BUG_ON() to patch #1
 - Added more explanation to patch #3

[PATCH v3 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 |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 438d0c0..72314c7 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -192,8 +192,8 @@ 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)
+	BUG_ON(size > MAX_BUFFER_OFFSET);
+	if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
 		return true;
 
 	return false;
-- 
1.7.10.4

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

[Sander Eikelenboom]

Friday, March 28, 2014, 12:39:05 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 |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 438d0c0..72314c7 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -192,8 +192,8 @@ 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)
> +       BUG_ON(size > MAX_BUFFER_OFFSET);
> +       if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
>                 return true;
>  
>         return false;

For the whole v3 series:

Reported-By: Sander Eikelenboom <linux@eikelenboom.it>
Tested-By: Sander Eikelenboom <linux@eikelenboom.it>

CC'ed Dave to get his attention since this is a last minute for 3.14.

--
Sander

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

[Ian Campbell]

On Fri, 2014-03-28 at 13:45 +0100, Sander Eikelenboom wrote:
> Friday, March 28, 2014, 12:39:05 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 |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 438d0c0..72314c7 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -192,8 +192,8 @@ 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)
> > +       BUG_ON(size > MAX_BUFFER_OFFSET);
> > +       if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
> >                 return true;
> >  
> >         return false;
> 
> For the whole v3 series:
> 
> Reported-By: Sander Eikelenboom <linux@eikelenboom.it>
> Tested-By: Sander Eikelenboom <linux@eikelenboom.it>
> 
> CC'ed Dave to get his attention since this is a last minute for 3.14.

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

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

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Fri, 28 Mar 2014 12:58:47 +0000

> On Fri, 2014-03-28 at 13:45 +0100, Sander Eikelenboom wrote:
>> Friday, March 28, 2014, 12:39:05 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 |    4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> > index 438d0c0..72314c7 100644
>> > --- a/drivers/net/xen-netback/netback.c
>> > +++ b/drivers/net/xen-netback/netback.c
>> > @@ -192,8 +192,8 @@ 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)
>> > +       BUG_ON(size > MAX_BUFFER_OFFSET);
>> > +       if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
>> >                 return true;
>> >  
>> >         return false;
>> 
>> For the whole v3 series:
>> 
>> Reported-By: Sander Eikelenboom <linux@eikelenboom.it>
>> Tested-By: Sander Eikelenboom <linux@eikelenboom.it>
>> 
>> CC'ed Dave to get his attention since this is a last minute for 3.14.
> 
> All three patches: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ok, we're coming down to the wire so this might not make it, but
I'll queue it up for -stable and get it merged when I can.

Thanks.

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

[Ian Campbell]

On Sat, 2014-03-29 at 18:52 -0400, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Fri, 28 Mar 2014 12:58:47 +0000
> 
> > On Fri, 2014-03-28 at 13:45 +0100, Sander Eikelenboom wrote:
> >> Friday, March 28, 2014, 12:39:05 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 |    4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> >> > index 438d0c0..72314c7 100644
> >> > --- a/drivers/net/xen-netback/netback.c
> >> > +++ b/drivers/net/xen-netback/netback.c
> >> > @@ -192,8 +192,8 @@ 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)
> >> > +       BUG_ON(size > MAX_BUFFER_OFFSET);
> >> > +       if ((offset + size > MAX_BUFFER_OFFSET) && offset && !head)
> >> >                 return true;
> >> >  
> >> >         return false;
> >> 
> >> For the whole v3 series:
> >> 
> >> Reported-By: Sander Eikelenboom <linux@eikelenboom.it>
> >> Tested-By: Sander Eikelenboom <linux@eikelenboom.it>
> >> 
> >> CC'ed Dave to get his attention since this is a last minute for 3.14.
> > 
> > All three patches: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Ok, we're coming down to the wire so this might not make it, but
> I'll queue it up for -stable and get it merged when I can.

Thanks!

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

Unfortunately some frontends aggressively limit the number of requests
they post into the shared ring so to avoid an estimate that is 'too'
pessimal it is capped at MAX_SKB_FRAGS.

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 |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 72314c7..573f3e8 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -493,9 +493,28 @@ 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);
 		}
+
+		/* To avoid the estimate becoming too pessimal for some
+		 * frontends that limit posted rx requests, cap the estimate
+		 * at MAX_SKB_FRAGS.
+		 */
+		if (max_slots_needed > MAX_SKB_FRAGS)
+			max_slots_needed = MAX_SKB_FRAGS;
+
+		/* We may need one more slot for GSO metadata */
 		if (skb_is_gso(skb) &&
 		   (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4 ||
 		    skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6))
-- 
1.7.10.4

[PATCH v3 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 necessarily the case
for GSO packets, because the non-prefix GSO protocol consumes one more ring
slot than meta-slot for the 'extra_info'. 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 573f3e8..cd0bd95 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -482,6 +482,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
@@ -530,8 +532,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