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

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

Thursday, January 10, 2013, 12:22:22 PM, you wrote:



> 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.
>>


What i'm wondering about (without any knowledge about the innerworkings of the 
network stack):
- Why can't netfront handle the largers packets in the first place ?
- Would it be right to do something like xennet_get_responses() does for rx:

        int max = MAX_SKB_FRAGS + (rx->status <= RX_COPY_THRESHOLD);
        if (unlikely(frags > max)) {
            net_warn_ratelimited("%s: Too many frags max:%d, frags:%d 
\n",skb->dev->name, max, frags);
                err = -E2BIG;
        }
  Instead of dropping the packet ?
  Don't know if this propagates via network protocols and will cause clients to 
reduce the packetsize and by that way avoid the need to copying in the first 
place ?


--
Sander


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