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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-08-22 at 14:14 +0100, Palagummi, Siva wrote:
> 
> > -----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?

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.


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