Re: [PATCH net-next-2.6] macvtap: Add GSO/csum offload support

[Sridhar Samudrala <sri@us.ibm.com>]

On 2/13/2010 9:34 AM, Arnd Bergmann wrote:
> Hi Sridhar,
>
> On Saturday 13 February 2010, Sridhar Samudrala wrote:
>
>    
>> This patch adds GSO/checksum offload support to macvtap driver and applies
>> on top of Arnd's refcnt bugfix.
>> 	http://patchwork.ozlabs.org/patch/45136/
>>      
> Sorry for messing this up by replacing that patch with a different one.
> It shouldn't be hard to rebase this one though, which I'll probably do on Monday.
> Please tell me if you want to do it yourself instead.
>    
No problem. If you get to it before, it is fine with me.
>> @@ -286,6 +288,7 @@ static int macvtap_open(struct inode *inode, struct file *file)
>>   	sock_init_data(&q->sock,&q->sk);
>>   	q->sk.sk_allocation = GFP_ATOMIC; /* for now */
>>   	q->sk.sk_write_space = macvtap_sock_write_space;
>> +	q->flags = IFF_VNET_HDR;
>>
>>   	err = macvtap_set_queue(dev, file, q);
>>   	if (err)
>>      
> Making IFF_VNET_HDR the default probably prevents the driver from working
> with applications that don't known about VNET_HDR, e.g. anything other
> than qemu. I believe qemu always tries setting it though, which would make
> a default value of !IFF_NET_HDR fine.
>    
Yes. It is better to make the default as !IFF_VNET_HDR
> Also, what about IFF_TAP and IFF_NO_PI, should those be always set?
>    
Atleast it is not required for qemu to have these flags set. If we are 
not doing anything different based on
these flags, i felt we don't need to have them.
>    
>> @@ -499,18 +648,14 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>   		return 0;
>>
>>   	case TUNSETOFFLOAD:
>> -		/* let the user check for future flags */
>> -		if (arg&  ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>> -			  TUN_F_TSO_ECN | TUN_F_UFO))
>> -			return -EINVAL;
>> -
>> -		/* TODO: add support for these, so far we don't
>> -			 support any offload */
>> -		if (arg&  (TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>> -			 TUN_F_TSO_ECN | TUN_F_UFO))
>> -			return -EINVAL;
>> -
>> -		return 0;
>> +		q = macvtap_file_get_queue(file);
>> +		if (!q)
>> +			return -ENOLINK;
>> +		ret = 0;
>> +		if (!(q->flags&  IFF_VNET_HDR))
>> +			ret =  -EINVAL;
>> +		macvtap_file_put_queue(q);
>> +		return ret;
>>
>>   	default:
>>   		return -EINVAL;
>>      
> At least the first check needs to be in there, in case we are running with
> new user space that knows additional flags. Moreover, shouldn't we check
> the flags against the capabilities of vlan->lowerdev? I though it would be
> best to report the capabilities of the real hardware to the guest kernel
> so it can do the right thing.
>    
Originally, i also thought we should check these based on the real 
device capabilities. But later i realized,
that it is not really required as we fall back to software offload via 
dev_gso_segment() call in dev_hard_start_xmit()
if the real device doesn't support any of the offloads. So we can 
advertise that all the offloads are supported to
the guest and let host deal with any offloads that are not supported by 
the real device.

Thanks
Sridhar