Re: [Xen-devel] xennet: skb rides the rocket: 20 slots

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-01-09 at 17:05 +0000, Konrad Rzeszutek Wilk wrote:
> 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.

Right, I think a corallary to the negotiation is that having agreed a
maximum you are responsible for ensuring you only send out requests
which meet it. IOW if you advertise this feature you also have to be
prepared to resegment if the OS above you gives you something larger
(either too many frags or via compound pages).

e.g. if the dom0 OS can support 16 frags and the domU 18 then netfront
and netback would agree on 16 slots maximum. If the domU OS passes an
SKB which requests more slots to netfront then netfront would have to
segment it before sending.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel