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 15:32 +0100, Palagummi, Siva wrote:
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > Sent: Wednesday, August 22, 2012 7:10 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: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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel