[PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Palagummi, Siva]

[-- Attachment #1.1: Type: text/plain, Size: 358 bytes --]

Hi,

I ran into an issue where netback driver is crashing with BUG_ON(npo.meta_prod > ARRAY_SIZE(netbk->meta)). It is happening in Intel 10Gbps network when larger mtu values  are used. The problem seems to be  the way the slots are counted. After applying this patch things ran fine in my environment. I request to validate my changes.

Thanks
Siva


[-- Attachment #1.2: Type: text/html, Size: 2233 bytes --]

[-- Attachment #2: netback_slots_counting.patch --]
[-- Type: application/octet-stream, Size: 1637 bytes --]

From: Siva Palagummi <Siva.Palagummi@ca.com>

count variable in xen_netbk_rx_action need to be incremented
correctly to take into account of extra slots required when skb_headlen is 
greater than PAGE_SIZE when larger MTU values are used. Without this change 
BUG_ON(npo.meta_prod > ARRAY_SIZE(netbk->meta)) is causing 
netback thread to exit.

While inspecting the code, it looked like we also need to take care of
incrementing the count variable when gso_size is non zero.

The problem is seen with linux 3.2.2 kernel on Intel 10Gbps network.


Signed-off-by: Siva Palagummi <Siva.Palagummi@ca.com>
---

diff -uprN a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
--- a/drivers/net/xen-netback/netback.c	2012-01-25 19:39:32.000000000 -0500
+++ b/drivers/net/xen-netback/netback.c	2012-08-12 15:50:50.000000000 -0400
@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
 
 		count += nr_frags + 1;
 
+		/*
+		 * The logic here should be somewhat similar to
+		 * xen_netbk_count_skb_slots. In case of larger MTU size,
+		 * skb head length may be more than a PAGE_SIZE. We need to
+		 * consider ring slots consumed by that data. If we do not,
+		 * then within this loop itself we end up consuming more meta
+		 * slots turning the BUG_ON below. With this fix we may end up
+		 * iterating through xen_netbk_rx_action multiple times
+		 * instead of crashing netback thread.
+		 */
+
+
+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
+
+		if (skb_shinfo(skb)->gso_size)
+			count++;
+
 		__skb_queue_tail(&rxq, skb);
 
 		/* Filled the batch queue? */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Jan Beulich]

>>> On 13.08.12 at 02:12, "Palagummi, Siva" <Siva.Palagummi@ca.com> wrote:
>--- a/drivers/net/xen-netback/netback.c	2012-01-25 19:39:32.000000000 -0500
>+++ b/drivers/net/xen-netback/netback.c	2012-08-12 15:50:50.000000000 -0400
>@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> 
> 		count += nr_frags + 1;
> 
>+		/*
>+		 * The logic here should be somewhat similar to
>+		 * xen_netbk_count_skb_slots. In case of larger MTU size,

Is there a reason why you can't simply use that function then?
Afaict it's being used on the very same skb before it gets put on
rx_queue already anyway.

>+		 * skb head length may be more than a PAGE_SIZE. We need to
>+		 * consider ring slots consumed by that data. If we do not,
>+		 * then within this loop itself we end up consuming more meta
>+		 * slots turning the BUG_ON below. With this fix we may end up
>+		 * iterating through xen_netbk_rx_action multiple times
>+		 * instead of crashing netback thread.
>+		 */
>+
>+
>+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

This now over-accounts by one I think (due to the "+ 1" above;
the calculation here really is to replace that increment).

Jan

>+
>+		if (skb_shinfo(skb)->gso_size)
>+			count++;
>+
> 		__skb_queue_tail(&rxq, skb);
> 
> 		/* Filled the batch queue? */

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Palagummi, Siva]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, August 13, 2012 1:58 PM
> To: Palagummi, Siva
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> properly when larger MTU sizes are used
> 
> >>> On 13.08.12 at 02:12, "Palagummi, Siva" <Siva.Palagummi@ca.com>
> wrote:
> >--- a/drivers/net/xen-netback/netback.c	2012-01-25 19:39:32.000000000
> -0500
> >+++ b/drivers/net/xen-netback/netback.c	2012-08-12 15:50:50.000000000
> -0400
> >@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> >
> > 		count += nr_frags + 1;
> >
> >+		/*
> >+		 * The logic here should be somewhat similar to
> >+		 * xen_netbk_count_skb_slots. In case of larger MTU size,
> 
> Is there a reason why you can't simply use that function then?
> Afaict it's being used on the very same skb before it gets put on
> rx_queue already anyway.
> 

I did think about it. But this would mean iterating through similar piece of code twice with additional function calls. netbk_gop_skb-->netbk_gop_frag_copy sequence is actually executing similar code. And also not sure about any other implications. So decided to fix it by adding few lines of code in line.

> >+		 * skb head length may be more than a PAGE_SIZE. We need to
> >+		 * consider ring slots consumed by that data. If we do not,
> >+		 * then within this loop itself we end up consuming more
> meta
> >+		 * slots turning the BUG_ON below. With this fix we may end
> up
> >+		 * iterating through xen_netbk_rx_action multiple times
> >+		 * instead of crashing netback thread.
> >+		 */
> >+
> >+
> >+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> 
> This now over-accounts by one I think (due to the "+ 1" above;
> the calculation here really is to replace that increment).
> 
> Jan
> 
I also wasn't sure about the actual purpose of "+1" above whether it is meant to take care of skb_headlen or non zero gso_size case or some other case.  That's why I left it like that so that I can exit the loop on safer side. If someone who knows this area of code can confirm that we do not need it, I will create a new patch. In my environment I did observe that "count" is always greater than 
actual meta slots produced because of this additional "+1" with my patch. When I took out this extra addition then count is always equal to actual meta slots produced and loop is exiting safely with more meta slots produced under heavy traffic.

Thanks
Siva
 
> >+
> >+		if (skb_shinfo(skb)->gso_size)
> >+			count++;
> >+
> > 		__skb_queue_tail(&rxq, skb);
> >
> > 		/* Filled the batch queue? */
> 

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Ian Campbell]

On Tue, 2012-08-14 at 22:17 +0100, Palagummi, Siva wrote:
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Monday, August 13, 2012 1:58 PM
> > To: Palagummi, Siva
> > Cc: xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > properly when larger MTU sizes are used
> > 
> > >>> On 13.08.12 at 02:12, "Palagummi, Siva" <Siva.Palagummi@ca.com>
> > wrote:
> > >--- a/drivers/net/xen-netback/netback.c	2012-01-25 19:39:32.000000000
> > -0500
> > >+++ b/drivers/net/xen-netback/netback.c	2012-08-12 15:50:50.000000000
> > -0400
> > >@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> > >
> > > 		count += nr_frags + 1;
> > >
> > >+		/*
> > >+		 * The logic here should be somewhat similar to
> > >+		 * xen_netbk_count_skb_slots. In case of larger MTU size,
> > 
> > Is there a reason why you can't simply use that function then?
> > Afaict it's being used on the very same skb before it gets put on
> > rx_queue already anyway.
> > 
> 
> I did think about it. But this would mean iterating through similar
> piece of code twice with additional function calls.
> netbk_gop_skb-->netbk_gop_frag_copy sequence is actually executing
> similar code. And also not sure about any other implications. So
> decided to fix it by adding few lines of code in line.

I wonder if we could cache the result of the call to
xen_netbk_count_skb_slots in xenvif_start_xmit somewhere?

> > >+		 * skb head length may be more than a PAGE_SIZE. We need to
> > >+		 * consider ring slots consumed by that data. If we do not,
> > >+		 * then within this loop itself we end up consuming more
> > meta
> > >+		 * slots turning the BUG_ON below. With this fix we may end
> > up
> > >+		 * iterating through xen_netbk_rx_action multiple times
> > >+		 * instead of crashing netback thread.
> > >+		 */
> > >+
> > >+
> > >+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> > 
> > This now over-accounts by one I think (due to the "+ 1" above;
> > the calculation here really is to replace that increment).
> > 
> > Jan
> > 
> I also wasn't sure about the actual purpose of "+1" above whether it
> is meant to take care of skb_headlen or non zero gso_size case or some
> other case.

I think it's intention was to account for skb_headlen and therefore it
should be replaced.

>   That's why I left it like that so that I can exit the loop on safer
> side. If someone who knows this area of code can confirm that we do
> not need it, I will create a new patch. In my environment I did
> observe that "count" is always greater than 
> actual meta slots produced because of this additional "+1" with my
> patch. When I took out this extra addition then count is always equal
> to actual meta slots produced and loop is exiting safely with more
> meta slots produced under heavy traffic.

I think that's an argument for removing it as well.

The + 1 leading to an early exit seems benign when you think about one
largish skb but imagine if you had 200 small (single page) skbs -- then
you have effectively halved the size of the ring (or at least the
batch).

This:
		/* Filled the batch queue? */
		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
			break;
seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
max_required_rx_slots(vif)? Or maybe the preflight checks from 
xenvif_start_xmit save us from this fate?

Ian.

> 
> Thanks
> Siva
>  
> > >+
> > >+		if (skb_shinfo(skb)->gso_size)
> > >+			count++;
> > >+
> > > 		__skb_queue_tail(&rxq, skb);
> > >
> > > 		/* Filled the batch queue? */
> > 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Palagummi, Siva]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Tuesday, August 21, 2012 6:45 PM
> To: Palagummi, Siva
> Cc: Jan Beulich; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> properly when larger MTU sizes are used
> 
> On Tue, 2012-08-14 at 22:17 +0100, Palagummi, Siva wrote:
> >
> > > -----Original Message-----
> > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > Sent: Monday, August 13, 2012 1:58 PM
> > > To: Palagummi, Siva
> > > Cc: xen-devel@lists.xen.org
> > > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > > properly when larger MTU sizes are used
> > >
> > > >>> On 13.08.12 at 02:12, "Palagummi, Siva" <Siva.Palagummi@ca.com>
> > > wrote:
> > > >--- a/drivers/net/xen-netback/netback.c	2012-01-25
> 19:39:32.000000000
> > > -0500
> > > >+++ b/drivers/net/xen-netback/netback.c	2012-08-12
> 15:50:50.000000000
> > > -0400
> > > >@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> > > >
> > > > 		count += nr_frags + 1;
> > > >
> > > >+		/*
> > > >+		 * The logic here should be somewhat similar to
> > > >+		 * xen_netbk_count_skb_slots. In case of larger MTU
> size,
> > >
> > > Is there a reason why you can't simply use that function then?
> > > Afaict it's being used on the very same skb before it gets put on
> > > rx_queue already anyway.
> > >
> >
> > I did think about it. But this would mean iterating through similar
> > piece of code twice with additional function calls.
> > netbk_gop_skb-->netbk_gop_frag_copy sequence is actually executing
> > similar code. And also not sure about any other implications. So
> > decided to fix it by adding few lines of code in line.
> 
> I wonder if we could cache the result of the call to
> xen_netbk_count_skb_slots in xenvif_start_xmit somewhere?
> 
> > > >+		 * skb head length may be more than a PAGE_SIZE. We
> need to
> > > >+		 * consider ring slots consumed by that data. If we
> do not,
> > > >+		 * then within this loop itself we end up consuming
> more
> > > meta
> > > >+		 * slots turning the BUG_ON below. With this fix we
> may end
> > > up
> > > >+		 * iterating through xen_netbk_rx_action multiple
> times
> > > >+		 * instead of crashing netback thread.
> > > >+		 */
> > > >+
> > > >+
> > > >+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> > >
> > > This now over-accounts by one I think (due to the "+ 1" above;
> > > the calculation here really is to replace that increment).
> > >
> > > Jan
> > >
> > I also wasn't sure about the actual purpose of "+1" above whether it
> > is meant to take care of skb_headlen or non zero gso_size case or
> some
> > other case.
> 
> I think it's intention was to account for skb_headlen and therefore it
> should be replaced.
> 
> >   That's why I left it like that so that I can exit the loop on safer
> > side. If someone who knows this area of code can confirm that we do
> > not need it, I will create a new patch. In my environment I did
> > observe that "count" is always greater than
> > actual meta slots produced because of this additional "+1" with my
> > patch. When I took out this extra addition then count is always equal
> > to actual meta slots produced and loop is exiting safely with more
> > meta slots produced under heavy traffic.
> 
> I think that's an argument for removing it as well.
> 
> The + 1 leading to an early exit seems benign when you think about one
> largish skb but imagine if you had 200 small (single page) skbs -- then
> you have effectively halved the size of the ring (or at least the
> batch).
> 
I agree and that's what even I have observed where xen_netbk_kick_thread is getting invoked to finish work in next iteration.  I will create a patch replacing "+1" with DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE).


> This:
> 		/* Filled the batch queue? */
> 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> 			break;
> seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> max_required_rx_slots(vif)? Or maybe the preflight checks from
> xenvif_start_xmit save us from this fate?
> 
> Ian.

You are right Ian. The intention of this check seems to be to ensure that enough slots are still left prior to picking up next skb. But instead of invoking max_required_rx_slots with already received skb, we may have to do skb_peek and invoke max_required_rx_slots on skb that we are about to dequeue. Is there any better way?

Thanks
Siva


> 
> >
> > Thanks
> > Siva
> >
> > > >+
> > > >+		if (skb_shinfo(skb)->gso_size)
> > > >+			count++;
> > > >+
> > > > 		__skb_queue_tail(&rxq, skb);
> > > >
> > > > 		/* Filled the batch queue? */
> > >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Ian Campbell]

On Wed, 2012-08-22 at 14:14 +0100, Palagummi, Siva wrote:
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> > Sent: Tuesday, August 21, 2012 6:45 PM
> > To: Palagummi, Siva
> > Cc: Jan Beulich; xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > properly when larger MTU sizes are used
> > 
> > On Tue, 2012-08-14 at 22:17 +0100, Palagummi, Siva wrote:
> > >
> > > > -----Original Message-----
> > > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > > Sent: Monday, August 13, 2012 1:58 PM
> > > > To: Palagummi, Siva
> > > > Cc: xen-devel@lists.xen.org
> > > > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > > > properly when larger MTU sizes are used
> > > >
> > > > >>> On 13.08.12 at 02:12, "Palagummi, Siva" <Siva.Palagummi@ca.com>
> > > > wrote:
> > > > >--- a/drivers/net/xen-netback/netback.c	2012-01-25
> > 19:39:32.000000000
> > > > -0500
> > > > >+++ b/drivers/net/xen-netback/netback.c	2012-08-12
> > 15:50:50.000000000
> > > > -0400
> > > > >@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> > > > >
> > > > > 		count += nr_frags + 1;
> > > > >
> > > > >+		/*
> > > > >+		 * The logic here should be somewhat similar to
> > > > >+		 * xen_netbk_count_skb_slots. In case of larger MTU
> > size,
> > > >
> > > > Is there a reason why you can't simply use that function then?
> > > > Afaict it's being used on the very same skb before it gets put on
> > > > rx_queue already anyway.
> > > >
> > >
> > > I did think about it. But this would mean iterating through similar
> > > piece of code twice with additional function calls.
> > > netbk_gop_skb-->netbk_gop_frag_copy sequence is actually executing
> > > similar code. And also not sure about any other implications. So
> > > decided to fix it by adding few lines of code in line.
> > 
> > I wonder if we could cache the result of the call to
> > xen_netbk_count_skb_slots in xenvif_start_xmit somewhere?
> > 
> > > > >+		 * skb head length may be more than a PAGE_SIZE. We
> > need to
> > > > >+		 * consider ring slots consumed by that data. If we
> > do not,
> > > > >+		 * then within this loop itself we end up consuming
> > more
> > > > meta
> > > > >+		 * slots turning the BUG_ON below. With this fix we
> > may end
> > > > up
> > > > >+		 * iterating through xen_netbk_rx_action multiple
> > times
> > > > >+		 * instead of crashing netback thread.
> > > > >+		 */
> > > > >+
> > > > >+
> > > > >+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> > > >
> > > > This now over-accounts by one I think (due to the "+ 1" above;
> > > > the calculation here really is to replace that increment).
> > > >
> > > > Jan
> > > >
> > > I also wasn't sure about the actual purpose of "+1" above whether it
> > > is meant to take care of skb_headlen or non zero gso_size case or
> > some
> > > other case.
> > 
> > I think it's intention was to account for skb_headlen and therefore it
> > should be replaced.
> > 
> > >   That's why I left it like that so that I can exit the loop on safer
> > > side. If someone who knows this area of code can confirm that we do
> > > not need it, I will create a new patch. In my environment I did
> > > observe that "count" is always greater than
> > > actual meta slots produced because of this additional "+1" with my
> > > patch. When I took out this extra addition then count is always equal
> > > to actual meta slots produced and loop is exiting safely with more
> > > meta slots produced under heavy traffic.
> > 
> > I think that's an argument for removing it as well.
> > 
> > The + 1 leading to an early exit seems benign when you think about one
> > largish skb but imagine if you had 200 small (single page) skbs -- then
> > you have effectively halved the size of the ring (or at least the
> > batch).
> > 
> I agree and that's what even I have observed where xen_netbk_kick_thread is getting invoked to finish work in next iteration.  I will create a patch replacing "+1" with DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE).
> 
> 
> > This:
> > 		/* Filled the batch queue? */
> > 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > 			break;
> > seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> > max_required_rx_slots(vif)? Or maybe the preflight checks from
> > xenvif_start_xmit save us from this fate?
> > 
> > Ian.
> 
> You are right Ian. The intention of this check seems to be to ensure
> that enough slots are still left prior to picking up next skb. But
> instead of invoking max_required_rx_slots with already received skb,
> we may have to do skb_peek and invoke max_required_rx_slots on skb
> that we are about to dequeue. Is there any better way?

max_required_rx_slots doesn't take an skb as an argument, just a vif. It
returns the worst case number of slots for any skb on that vif.

Ian.

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Palagummi, Siva]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Wednesday, August 22, 2012 6:52 PM
> To: Palagummi, Siva
> Cc: Jan Beulich; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> properly when larger MTU sizes are used
> 
> On Wed, 2012-08-22 at 14:14 +0100, Palagummi, Siva wrote:
> >
> > > -----Original Message-----
> > > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> > > Sent: Tuesday, August 21, 2012 6:45 PM
> > > To: Palagummi, Siva
> > > Cc: Jan Beulich; xen-devel@lists.xen.org
> > > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > > properly when larger MTU sizes are used
> > >
> > > On Tue, 2012-08-14 at 22:17 +0100, Palagummi, Siva wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > > > Sent: Monday, August 13, 2012 1:58 PM
> > > > > To: Palagummi, Siva
> > > > > Cc: xen-devel@lists.xen.org
> > > > > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring
> slots
> > > > > properly when larger MTU sizes are used
> > > > >
> > > > > >>> On 13.08.12 at 02:12, "Palagummi, Siva"
> <Siva.Palagummi@ca.com>
> > > > > wrote:
> > > > > >--- a/drivers/net/xen-netback/netback.c	2012-01-25
> > > 19:39:32.000000000
> > > > > -0500
> > > > > >+++ b/drivers/net/xen-netback/netback.c	2012-08-12
> > > 15:50:50.000000000
> > > > > -0400
> > > > > >@@ -623,6 +623,24 @@ static void xen_netbk_rx_action(struct x
> > > > > >
> > > > > > 		count += nr_frags + 1;
> > > > > >
> > > > > >+		/*
> > > > > >+		 * The logic here should be somewhat similar to
> > > > > >+		 * xen_netbk_count_skb_slots. In case of larger MTU
> > > size,
> > > > >
> > > > > Is there a reason why you can't simply use that function then?
> > > > > Afaict it's being used on the very same skb before it gets put
> on
> > > > > rx_queue already anyway.
> > > > >
> > > >
> > > > I did think about it. But this would mean iterating through
> similar
> > > > piece of code twice with additional function calls.
> > > > netbk_gop_skb-->netbk_gop_frag_copy sequence is actually
> executing
> > > > similar code. And also not sure about any other implications. So
> > > > decided to fix it by adding few lines of code in line.
> > >
> > > I wonder if we could cache the result of the call to
> > > xen_netbk_count_skb_slots in xenvif_start_xmit somewhere?
> > >
> > > > > >+		 * skb head length may be more than a PAGE_SIZE. We
> > > need to
> > > > > >+		 * consider ring slots consumed by that data. If we
> > > do not,
> > > > > >+		 * then within this loop itself we end up consuming
> > > more
> > > > > meta
> > > > > >+		 * slots turning the BUG_ON below. With this fix we
> > > may end
> > > > > up
> > > > > >+		 * iterating through xen_netbk_rx_action multiple
> > > times
> > > > > >+		 * instead of crashing netback thread.
> > > > > >+		 */
> > > > > >+
> > > > > >+
> > > > > >+		count += DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> > > > >
> > > > > This now over-accounts by one I think (due to the "+ 1" above;
> > > > > the calculation here really is to replace that increment).
> > > > >
> > > > > Jan
> > > > >
> > > > I also wasn't sure about the actual purpose of "+1" above whether
> it
> > > > is meant to take care of skb_headlen or non zero gso_size case or
> > > some
> > > > other case.
> > >
> > > I think it's intention was to account for skb_headlen and therefore
> it
> > > should be replaced.
> > >
> > > >   That's why I left it like that so that I can exit the loop on
> safer
> > > > side. If someone who knows this area of code can confirm that we
> do
> > > > not need it, I will create a new patch. In my environment I did
> > > > observe that "count" is always greater than
> > > > actual meta slots produced because of this additional "+1" with
> my
> > > > patch. When I took out this extra addition then count is always
> equal
> > > > to actual meta slots produced and loop is exiting safely with
> more
> > > > meta slots produced under heavy traffic.
> > >
> > > I think that's an argument for removing it as well.
> > >
> > > The + 1 leading to an early exit seems benign when you think about
> one
> > > largish skb but imagine if you had 200 small (single page) skbs --
> then
> > > you have effectively halved the size of the ring (or at least the
> > > batch).
> > >
> > I agree and that's what even I have observed where
> xen_netbk_kick_thread is getting invoked to finish work in next
> iteration.  I will create a patch replacing "+1" with
> DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE).
> >
> >
> > > This:
> > > 		/* Filled the batch queue? */
> > > 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > > 			break;
> > > seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> > > max_required_rx_slots(vif)? Or maybe the preflight checks from
> > > xenvif_start_xmit save us from this fate?
> > >
> > > Ian.
> >
> > You are right Ian. The intention of this check seems to be to ensure
> > that enough slots are still left prior to picking up next skb. But
> > instead of invoking max_required_rx_slots with already received skb,
> > we may have to do skb_peek and invoke max_required_rx_slots on skb
> > that we are about to dequeue. Is there any better way?
> 
> max_required_rx_slots doesn't take an skb as an argument, just a vif.
> It
> returns the worst case number of slots for any skb on that vif.
> 
> Ian.

That’s true. What I meant is to peek to next skb and get vif from that structure to invoke max_required_rx_slots. Don't you think we need to do like that?

Thanks
Siva

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Ian Campbell]

On Wed, 2012-08-22 at 14:30 +0100, Palagummi, Siva wrote:
> > >
> > >
> > > > This:
> > > > 		/* Filled the batch queue? */
> > > > 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > > > 			break;
> > > > seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> > > > max_required_rx_slots(vif)? Or maybe the preflight checks from
> > > > xenvif_start_xmit save us from this fate?
> > > >
> > > > Ian.
> > >
> > > You are right Ian. The intention of this check seems to be to ensure
> > > that enough slots are still left prior to picking up next skb. But
> > > instead of invoking max_required_rx_slots with already received skb,
> > > we may have to do skb_peek and invoke max_required_rx_slots on skb
> > > that we are about to dequeue. Is there any better way?
> > 
> > max_required_rx_slots doesn't take an skb as an argument, just a vif.
> > It
> > returns the worst case number of slots for any skb on that vif.
> > 
> > Ian.
> 
> That’s true. What I meant is to peek to next skb and get vif from that
> structure to invoke max_required_rx_slots. Don't you think we need to
> do like that?

Do you mean something other than max_required_rx_slots? Because the
prototype of that function is
        static int max_required_rx_slots(struct xenvif *vif)
i.e. it doesn't need an skb.

I think it is acceptable to check for the worst case number of slots.
That's what we do e.g. in xen_netbk_rx_ring_full.

Using skb_peek might work too though, assuming all the locking etc is ok
-- this is a private queue so I think it is probably ok. Rather than
calculating the number of slots in xen_netbk_rx_action you probably want
to remember the value from the call to xen_netbk_count_skb_slots in
start_xmit. Perhaps by stashing it in skb->cb? (see NETFRONT_SKB_CB for
an example of how to do this)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Palagummi, Siva]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Wednesday, August 22, 2012 7:10 PM
> To: Palagummi, Siva
> Cc: Jan Beulich; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> properly when larger MTU sizes are used
> 
> On Wed, 2012-08-22 at 14:30 +0100, Palagummi, Siva wrote:
> > > >
> > > >
> > > > > This:
> > > > > 		/* Filled the batch queue? */
> > > > > 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > > > > 			break;
> > > > > seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> > > > > max_required_rx_slots(vif)? Or maybe the preflight checks from
> > > > > xenvif_start_xmit save us from this fate?
> > > > >
> > > > > Ian.
> > > >
> > > > You are right Ian. The intention of this check seems to be to
> ensure
> > > > that enough slots are still left prior to picking up next skb.
> But
> > > > instead of invoking max_required_rx_slots with already received
> skb,
> > > > we may have to do skb_peek and invoke max_required_rx_slots on
> skb
> > > > that we are about to dequeue. Is there any better way?
> > >
> > > max_required_rx_slots doesn't take an skb as an argument, just a
> vif.
> > > It
> > > returns the worst case number of slots for any skb on that vif.
> > >
> > > Ian.
> >
> > That’s true. What I meant is to peek to next skb and get vif from
> that
> > structure to invoke max_required_rx_slots. Don't you think we need to
> > do like that?
> 
> Do you mean something other than max_required_rx_slots? Because the
> prototype of that function is
>         static int max_required_rx_slots(struct xenvif *vif)
> i.e. it doesn't need an skb.
> 
> I think it is acceptable to check for the worst case number of slots.
> That's what we do e.g. in xen_netbk_rx_ring_full
> 
> Using skb_peek might work too though, assuming all the locking etc is
> ok o

I want to use max_required_rx_slots only. So the code will look somewhat like this.

skb=skb_peek(&netbk->rx_queue);
if(skb == NULL)
	break;
vif=netdev_priv(skb->dev);

/*Filled the batch queue?*/
If(count + max_required_rx_slots(vif) >= XEN_NETIF_RX_RING_SIZE)
	break;


> -- this is a private queue so I think it is probably ok. Rather than
> calculating the number of slots in xen_netbk_rx_action you probably
> want
> to remember the value from the call to xen_netbk_count_skb_slots in
> start_xmit. Perhaps by stashing it in skb->cb? (see NETFRONT_SKB_CB for
> an example of how to do this)
> 
> Ian.

Ok. I will look into this as well. This will definitely save some cycles in xen_netbk_rx_action. Apart from that calculations we already discussed should work fine right?

Thanks for all your input Ian.

Siva

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH RFC] xen/netback: Count ring slots properly when larger MTU sizes are used

[Ian Campbell]

On Wed, 2012-08-22 at 15:32 +0100, Palagummi, Siva wrote:
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> > Sent: Wednesday, August 22, 2012 7:10 PM
> > To: Palagummi, Siva
> > Cc: Jan Beulich; xen-devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH RFC] xen/netback: Count ring slots
> > properly when larger MTU sizes are used
> > 
> > On Wed, 2012-08-22 at 14:30 +0100, Palagummi, Siva wrote:
> > > > >
> > > > >
> > > > > > This:
> > > > > > 		/* Filled the batch queue? */
> > > > > > 		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > > > > > 			break;
> > > > > > seems a bit iffy to me too. I wonder if MAX_SKB_FRAGS should be
> > > > > > max_required_rx_slots(vif)? Or maybe the preflight checks from
> > > > > > xenvif_start_xmit save us from this fate?
> > > > > >
> > > > > > Ian.
> > > > >
> > > > > You are right Ian. The intention of this check seems to be to
> > ensure
> > > > > that enough slots are still left prior to picking up next skb.
> > But
> > > > > instead of invoking max_required_rx_slots with already received
> > skb,
> > > > > we may have to do skb_peek and invoke max_required_rx_slots on
> > skb
> > > > > that we are about to dequeue. Is there any better way?
> > > >
> > > > max_required_rx_slots doesn't take an skb as an argument, just a
> > vif.
> > > > It
> > > > returns the worst case number of slots for any skb on that vif.
> > > >
> > > > Ian.
> > >
> > > That’s true. What I meant is to peek to next skb and get vif from
> > that
> > > structure to invoke max_required_rx_slots. Don't you think we need to
> > > do like that?
> > 
> > Do you mean something other than max_required_rx_slots? Because the
> > prototype of that function is
> >         static int max_required_rx_slots(struct xenvif *vif)
> > i.e. it doesn't need an skb.
> > 
> > I think it is acceptable to check for the worst case number of slots.
> > That's what we do e.g. in xen_netbk_rx_ring_full
> > 
> > Using skb_peek might work too though, assuming all the locking etc is
> > ok o
> 
> I want to use max_required_rx_slots only. So the code will look somewhat like this.
> 
> skb=skb_peek(&netbk->rx_queue);
> if(skb == NULL)
> 	break;
> vif=netdev_priv(skb->dev);

Oh, I see why you need the skb now!

> /*Filled the batch queue?*/
> If(count + max_required_rx_slots(vif) >= XEN_NETIF_RX_RING_SIZE)
> 	break;

You need to to finally dequeue the skb here.

> 
> 
> > -- this is a private queue so I think it is probably ok. Rather than
> > calculating the number of slots in xen_netbk_rx_action you probably
> > want
> > to remember the value from the call to xen_netbk_count_skb_slots in
> > start_xmit. Perhaps by stashing it in skb->cb? (see NETFRONT_SKB_CB for
> > an example of how to do this)
> > 
> > Ian.
> 
> Ok. I will look into this as well. This will definitely save some
> cycles in xen_netbk_rx_action. Apart from that calculations we already
> discussed should work fine right?

I think so. Proof in the pudding and all that ;-)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel