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

[Arnd Bergmann <arnd@arndb.de>]

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.

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

Also, what about IFF_TAP and IFF_NO_PI, should those be always set?

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

	Arnd