From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH net-next-2.6] macvtap: Add GSO/csum offload support Date: Sat, 13 Feb 2010 18:34:00 +0100 Message-ID: <201002131834.00612.arnd@arndb.de> References: <1266013667.6105.18.camel@w-sridhar.beaverton.ibm.com> <1266044316.10419.3.camel@w-sridhar.beaverton.ibm.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Cc: David Miller , Herbert Xu , netdev To: Sridhar Samudrala Return-path: Received: from moutng.kundenserver.de ([212.227.17.8]:56723 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754175Ab0BMReJ (ORCPT ); Sat, 13 Feb 2010 12:34:09 -0500 In-Reply-To: <1266044316.10419.3.camel@w-sridhar.beaverton.ibm.com> Sender: netdev-owner@vger.kernel.org List-ID: 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