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

["Palagummi, Siva" <Siva.Palagummi@xxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Tuesday, August 21, 2012 6:45 PM
> To: Palagummi, Siva
> Cc: Jan Beulich; xen-devel@xxxxxxxxxxxxx
> 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@xxxxxxxx]
> > > Sent: Monday, August 13, 2012 1:58 PM
> > > To: Palagummi, Siva
> > > Cc: xen-devel@xxxxxxxxxxxxx
> > > 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@xxxxxx>
> > > 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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> 

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