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: Wednesday, August 22, 2012 6:52 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 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.

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel