Re: xennet: skb rides the rocket: 20 slots

[ANNIE LI <annie.li@oracle.com>]

On 2013-1-9 23:08, 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.

I am thinking to do re-fragment in netfront for these skbs like following,

Create a new skb, copy linear data and frag data from original skb into 
this one, and make every frags data size is PAGE_SIZE except for the 
last fragment. It is possible that the last fragment length is less than 
PAGE_SIZE, then free the original skb. The skb packet is large, and 
there will be lots of copys.

struct skbuff *xennet_refrag_skb(skb)
{
    create newskb
    copying data and doing fragmentation
    return newskb
}

.......

if (unlikely(slots>  MAX_SKB_FRAGS + 1)) {
         net_alert_ratelimited(
               "xennet: skb rides the rocket: %d slots\n", slots);
         skb = xennet_refrag_skb(skb);
}
.....

Thanks
Annie
>
>> "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
>
> in both netback and netfront to set the maximum number of frags we can
> handle to 21? If that works with Sander test - then yes, it looks like
> we really need to get this 'feature-max-skb-frags' done.
>