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 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);

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