Re: xennet: skb rides the rocket: 20 slots

[Ian Campbell <Ian.Campbell@citrix.com>]

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?

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.

Ian.