Re: xennet: skb rides the rocket: 20 slots

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jan 09, 2013 at 04:34:01PM +0000, Ian Campbell wrote:
> On Wed, 2013-01-09 at 15:08 +0000, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jan 09, 2013 at 03:10:56PM +0800, ANNIE LI wrote:
> > > 
> > > 
> > > On 2013-1-9 4:55, Sander Eikelenboom wrote:
> > > >>                  if (unlikely(frags>= MAX_SKB_FRAGS)) {
> > > >>                          netdev_dbg(vif->dev, "Too many frags\n");
> > > >>                          return -frags;
> > > >>                  }
> > > >I have added some rate limited warns in this function. However none seems to be triggered while the pv-guest reports the "skb rides the rocket" ..
> > > 
> > > Oh,  yes, "skb rides the rocket" is a protect mechanism in netfront,
> > > and it is not caused by netback checking code, but they all concern
> > > about the same thing(frags >= MAX_SKB_FRAGS ). I thought those
> > > packets were dropped by backend check, sorry for the confusion.
> > > 
> > > In netfront, following code would check whether required slots
> > > exceed MAX_SKB_FRAGS, and drop skbs which does not meet this
> > > requirement directly.
> > > 
> > >         if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
> > >                 net_alert_ratelimited(
> > >                         "xennet: skb rides the rocket: %d slots\n", slots);
> > >                 goto drop;
> > >         }
> > > 
> > > In netback, following code also compared frags with MAX_SKB_FRAGS,
> > > and create error response for netfront which does not meet this
> > > requirment. In this case, netfront will also drop corresponding
> > > skbs.
> > > 
> > >                 if (unlikely(frags >= MAX_SKB_FRAGS)) {
> > >                         netdev_dbg(vif->dev, "Too many frags\n");
> > >                         return -frags;
> > >                 }
> > > 
> > > So it is correct that netback log was not print out because those
> > > packets are drops directly by frontend check, not by backend check.
> > > Without the frontend check, it is likely that netback check would
> > > block these skbs and create error response for netfront.
> > > 
> > > So two ways are available: workaround in netfront for those packets,
> > > doing re-fragment copying, but not sure how copying hurt
> > > performance. Another is to implement in netback, as discussed in
> > 
> > There is already some copying done (the copying of the socket data
> > from userspace to the kernel) - so the extra copy might not be that
> > bad as the data can be in the cache. This would probably be a way
> > to deal with old backends that cannot deal with this new feature-flag.
> 
> Or for any backend which doesn't handle enough slots for the current
> skb. Essentially you need to do a fragmentation in software in the
> frontend.
> 
> For backends which don't support the flag we should just hardcode some
> number (some historically common value) as the default unless
> negotiation says otherwise, no need to copy everything...
> 
> > 
> > > "netchannel vs MAX_SKB_FRAGS". Maybe these two mechanism are all
> > > necessary?
> > 
> > Lets see first if this is indeed the problem. Perhaps a simple debug
> > patch that just does:
> > 
> > 	s/MAX_SKB_FRAGS/DEBUG_MAX_FRAGS/
> > 	#define DEBUG_MAX_FRAGS 21
> 
> Do you mean to do this globally or just in the netfront (or back)
> driver?

Our frontend and backend driver.
> 
> Doing it globally would just be a case of changing the define, without
> the s//, but there would be performance implications to growing the
> shinfo.
> 
> Just changing netfront won't work because there will only be
> MAX_SKB_FRAGS in the skb's shinfo so you won't be able to add any more.

Oh, then doing the negotiation for the "slots" is gated to a maximum
of MAX_SKB_FRAGS. Hmmm.. that presents a problem for this bug as
just negotiating the max_skb_frags feature won't solve the "riding the rocket"
problem.