From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: xennet: skb rides the rocket: 20 slots Date: Wed, 9 Jan 2013 12:05:30 -0500 Message-ID: <20130109170530.GC20683@phenom.dumpdata.com> References: <72958707.20130104172854@eikelenboom.it> <1357556115.7989.13.camel@zakaz.uk.xensource.com> <50EB8091.90705@oracle.com> <323202711.20130108215503@eikelenboom.it> <50ED1800.1080208@oracle.com> <20130109150850.GI18395@phenom.dumpdata.com> <1357749241.7989.280.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1357749241.7989.280.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Sander Eikelenboom , ANNIE LI , xen-devel List-Id: xen-devel@lists.xenproject.org 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.