WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have c

To: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] xen: netfront: Drop GSO SKBs which do not have csum_blank.
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Mon, 24 Jan 2011 09:55:40 -0800
Cc: "netdev@xxxxxxxxxxxxxxx" <netdev@xxxxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 24 Jan 2011 09:56:26 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1295689392.3693.153.camel@xxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1294233811-28123-1-git-send-email-ian.campbell@xxxxxxxxxx> <4D3A2BD2.5030802@xxxxxxxx> <1295689392.3693.153.camel@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.7
On 01/22/2011 01:43 AM, Ian Campbell wrote:
> On Sat, 2011-01-22 at 00:58 +0000, Jeremy Fitzhardinge wrote: 
>> On 01/05/2011 05:23 AM, Ian Campbell wrote:
>>> The Linux network stack expects all GSO SKBs to have ip_summed ==
>>> CHECKSUM_PARTIAL (which implies that the frame contains a partial
>>> checksum) and the Xen network ring protocol similarly expects an SKB
>>> which has GSO set to also have NETRX_csum_blank (which also implies a
>>> partial checksum). Therefore drop such frames on receive otherwise
>>> they will trigger the warning in skb_gso_segment.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>> Cc: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
>>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
>>> Cc: netdev@xxxxxxxxxxxxxxx
>>> ---
>>>  drivers/net/xen-netfront.c |    5 +++++
>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>>> index cdbeec9..8b8c480 100644
>>> --- a/drivers/net/xen-netfront.c
>>> +++ b/drivers/net/xen-netfront.c
>>> @@ -836,6 +836,11 @@ static int handle_incoming_queue(struct net_device 
>>> *dev,
>>>                             dev->stats.rx_errors++;
>>>                             continue;
>>>                     }
>>> +           } else if (skb_is_gso(skb)) {
>>> +                   kfree_skb(skb);
>>> +                   packets_dropped++;
>>> +                   dev->stats.rx_errors++;
>>> +                   continue;
>> This looks redundant; why not something like:
>>
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index 47e6a71..c1b8f64 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
>> @@ -852,13 +852,12 @@ static int handle_incoming_queue(struct net_device 
>> *dev,
>>              /* Ethernet work: Delayed to here as it peeks the header. */
>>              skb->protocol = eth_type_trans(skb, dev);
>>  
>> -            if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> -                    if (skb_checksum_setup(skb)) {
>> -                            kfree_skb(skb);
>> -                            packets_dropped++;
>> -                            dev->stats.rx_errors++;
>> -                            continue;
>> -                    }
>> +            if (skb->ip_summed != CHECKSUM_PARTIAL ||
>> +                skb_checksum_setup(skb)) {
> That drops non-partial skbs. However they are fine unless they also
> claim to be gso.
>
> Perhaps you meant "skb->ip_summed == CHECKSUM_PARTIAL && !
> skb_checksum_setup(skb)" which I think works but doesn't allow us to
> correctly chain the gso check onto the else.

No, I didn't mean to drop the skb_is_gso() test.  But still, the if()s
can be folded to share the same body.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel