| Message ID | 4DC26F33.8010700@cn.fujitsu.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, May 05, 2011 at 05:34:43PM +0800, Shan Wei wrote: > > TUN_F_TSO4, TUN_F_TSO6, TUN_F_TSO_ECN, TUN_F_UFO these features are > depend on NETIF_F_SG. If NETIF_F_SG is not set, these features are not be > enabled and warnings are printed in netdev_fix_features(). No, when the user turns off checksum offload everything should be turned off as well. However, when it's turned on, we shouldn't enable everything automatically. Cheers,
On Thu, May 05, 2011 at 08:05:06PM +1000, Herbert Xu wrote: > On Thu, May 05, 2011 at 05:34:43PM +0800, Shan Wei wrote: > > > > TUN_F_TSO4, TUN_F_TSO6, TUN_F_TSO_ECN, TUN_F_UFO these features are > > depend on NETIF_F_SG. If NETIF_F_SG is not set, these features are not be > > enabled and warnings are printed in netdev_fix_features(). > > No, when the user turns off checksum offload everything should > be turned off as well. However, when it's turned on, we shouldn't > enable everything automatically. > > Cheers, So how is NETIF_F_SG supposed to be enabled then? In upstream kernels userspace can disable checksum offloading then re-enable and get SG set back. userspace came to depend on this behaviour so I think changing this is a regression. > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 10:32:10AM +0300, Michael S. Tsirkin wrote: > > So how is NETIF_F_SG supposed to be enabled then? It should either be enabled at device creation time, or whatever user-space entity managing the device creation should enable it along with checksumming and anything else applicable. > In upstream kernels userspace can disable checksum offloading then > re-enable and get SG set back. userspace came to depend on this > behaviour so I think changing this is a regression. Can you point me to the relevant code in the upstream kernel? I'm not aware of any automatic SG enabling for network devices in general when you enable checksum offloading. Cheers,
On Mon, May 16, 2011 at 06:07:02PM +1000, Herbert Xu wrote: > On Mon, May 16, 2011 at 10:32:10AM +0300, Michael S. Tsirkin wrote: > > > > So how is NETIF_F_SG supposed to be enabled then? > > It should either be enabled at device creation time, or whatever > user-space entity managing the device creation should enable it > along with checksumming and anything else applicable. There's no interface for userspace to enable it: userspace only has an ioctl to enable/disable checksum offloading. SG is an implementation detail. > > In upstream kernels userspace can disable checksum offloading then > > re-enable and get SG set back. userspace came to depend on this > > behaviour so I think changing this is a regression. > > Can you point me to the relevant code in the upstream kernel? > I'm not aware of any automatic SG enabling for network devices > in general when you enable checksum offloading. > > Cheers, I think what happens _SG is enabled at device creation time and then upstream just keeps it on always, even when user clears CSUM. With net-next code changed so that _SG gets cleared when CSUM 'gets cleared. But then it does not get reenabled when CSUM gets reenabled. > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 06:07:02PM +1000, Herbert Xu wrote: > On Mon, May 16, 2011 at 10:32:10AM +0300, Michael S. Tsirkin wrote: > > > > So how is NETIF_F_SG supposed to be enabled then? > > It should either be enabled at device creation time, or whatever > user-space entity managing the device creation should enable it > along with checksumming and anything else applicable. > > > In upstream kernels userspace can disable checksum offloading then > > re-enable and get SG set back. userspace came to depend on this > > behaviour so I think changing this is a regression. > > Can you point me to the relevant code in the upstream kernel? > I'm not aware of any automatic SG enabling for network devices > in general when you enable checksum offloading. > > Cheers, By the way with kvm we let an unpriveledged application control netdev flags through and ioctl. It's probably not a good idea to print out stuff on flag change unconditionally as that will let that application fill up the system log. > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 11:18:41AM +0300, Michael S. Tsirkin wrote: > > There's no interface for userspace to enable it: userspace > only has an ioctl to enable/disable checksum offloading. > SG is an implementation detail. Yes there is: ethtool -K Cheers,
On Mon, May 16, 2011 at 07:38:22PM +1000, Herbert Xu wrote: > On Mon, May 16, 2011 at 11:18:41AM +0300, Michael S. Tsirkin wrote: > > > > There's no interface for userspace to enable it: userspace > > only has an ioctl to enable/disable checksum offloading. > > SG is an implementation detail. > > Yes there is: ethtool -K > > Cheers, Ok. You are right of course, I was confused. So it's just an info message: in the end if userspace DTRT features get enabled properly. Functionally, there is no regression. Both bridge and tun trigger the messages ATM. I note that dev.c takes pains to avoid these messages: Avoid warning from netdev_fix_features() for GSO without SG Should all drivers do this: clear _SG if they clear checksum? What about _GSO? If we start doing this, this spills info about flag dependencies out to drivers.
On Mon, May 16, 2011 at 12:48:21PM +0300, Michael S. Tsirkin wrote: > > Both bridge and tun trigger the messages ATM. I note that > dev.c takes pains to avoid these messages: > Avoid warning from netdev_fix_features() for GSO without SG > > Should all drivers do this: clear _SG if they clear checksum? What about _GSO? > If we start doing this, this spills info about flag dependencies out to drivers. Drivers don't need to do this at all since the network stack will automatically clear SG and related features if checksum is disabled. Cheers,
2011/5/16 Michael S. Tsirkin <mst@redhat.com>: > On Mon, May 16, 2011 at 07:38:22PM +1000, Herbert Xu wrote: >> On Mon, May 16, 2011 at 11:18:41AM +0300, Michael S. Tsirkin wrote: >> > >> > There's no interface for userspace to enable it: userspace >> > only has an ioctl to enable/disable checksum offloading. >> > SG is an implementation detail. >> Yes there is: ethtool -K > Ok. You are right of course, I was confused. > > So it's just an info message: in the end if userspace DTRT features > get enabled properly. Functionally, there is no regression. I proposed changing those messages to DEBUG level some time ago.Those dependencies are constant, so they could be moved to Documentation/networking/. > Both bridge and tun trigger the messages ATM. I note that > dev.c takes pains to avoid these messages: > Avoid warning from netdev_fix_features() for GSO without SG > > Should all drivers do this: clear _SG if they clear checksum? What about _GSO? > If we start doing this, this spills info about flag dependencies out to drivers. Drivers should not care about those dependencies. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 08:43:10PM +1000, Herbert Xu wrote: > On Mon, May 16, 2011 at 12:48:21PM +0300, Michael S. Tsirkin wrote: > > > > Both bridge and tun trigger the messages ATM. I note that > > dev.c takes pains to avoid these messages: > > Avoid warning from netdev_fix_features() for GSO without SG > > > > Should all drivers do this: clear _SG if they clear checksum? What about _GSO? > > If we start doing this, this spills info about flag dependencies out to drivers. > > Drivers don't need to do this at all since the network stack > will automatically clear SG and related features if checksum > is disabled. > > Cheers, Yes, but then we get annoying info messages every time. Change them all to debug or remove completely? > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 02:21:33PM +0300, Michael S. Tsirkin wrote: > > Yes, but then we get annoying info messages every time. > Change them all to debug or remove completely? If you are getting warning messages then it probably means that your driver is doing something wrong. Cheers,
2011/5/16 Herbert Xu <herbert@gondor.apana.org.au>: > On Mon, May 16, 2011 at 02:21:33PM +0300, Michael S. Tsirkin wrote: >> Yes, but then we get annoying info messages every time. >> Change them all to debug or remove completely? > If you are getting warning messages then it probably means that > your driver is doing something wrong. In this case - no. Those messages inform that driver supports more feature combinations than network core and the feature set is reduced because of that. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 02:24:19PM +0200, Michał Mirosław wrote: > > In this case - no. Those messages inform that driver supports more > feature combinations than network core and the feature set is reduced > because of that. The driver should never even claim to support SG/TSO/UFO if it does not support checksum. That is the point of the warning. Cheers,
From: Herbert Xu <herbert@gondor.hengli.com.au> Date: Tue, 17 May 2011 08:46:58 +1000 > On Mon, May 16, 2011 at 02:24:19PM +0200, Michał Mirosław wrote: >> >> In this case - no. Those messages inform that driver supports more >> feature combinations than network core and the feature set is reduced >> because of that. > > The driver should never even claim to support SG/TSO/UFO if it > does not support checksum. That is the point of the warning. Well the check has to exist somewhere. Currently userspace can configure tun/tap into whatever set of offloads it likes. We're warning when the user asks for something that needs to be corrected. So the only thing you can suggest is to duplicate these changes in the tun/tap driver. But if we do that, and error on bad combinations instead of fixing them up, we know from this discussion that existing virtualization setups and tools are going to stop working. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2011 at 07:06:15PM -0400, David Miller wrote: > > Well the check has to exist somewhere. > > Currently userspace can configure tun/tap into whatever set > of offloads it likes. > > We're warning when the user asks for something that needs to be > corrected. So the only thing you can suggest is to duplicate these > changes in the tun/tap driver. > > But if we do that, and error on bad combinations instead of fixing > them up, we know from this discussion that existing virtualization > setups and tools are going to stop working. Yeah the tun driver is simply busted. We should never have allowed user-space to tweak the feature bits like this. Instead they should have gone through the ethtool interface like everyone else, or at least use the same underlying calls as ethtool. Actually, I think we can still do that, and apply the same rules as ethtool with respect to automatically turning things on/off. AFAICS the current set_offload in tun.c does not call anything that verifies/fixes up the settings. If you change the feature bits after registering the tun device it may never get fixed up at all. Allowing an unprivileged user to tweak feature bits directly with no verification is just wrong. Cheers,
On Tue, May 17, 2011 at 09:45:38AM +1000, Herbert Xu wrote: > On Mon, May 16, 2011 at 07:06:15PM -0400, David Miller wrote: > > > > Well the check has to exist somewhere. > > > > Currently userspace can configure tun/tap into whatever set > > of offloads it likes. > > > > We're warning when the user asks for something that needs to be > > corrected. So the only thing you can suggest is to duplicate these > > changes in the tun/tap driver. > > > > But if we do that, and error on bad combinations instead of fixing > > them up, we know from this discussion that existing virtualization > > setups and tools are going to stop working. > > Yeah the tun driver is simply busted. We should never have allowed > user-space to tweak the feature bits like this. Instead they should > have gone through the ethtool interface like everyone else, or at > least use the same underlying calls as ethtool. > > Actually, I think we can still do that, and apply the same rules > as ethtool with respect to automatically turning things on/off. > > AFAICS the current set_offload in tun.c does not call anything > that verifies/fixes up the settings. If you change the feature > bits after registering the tun device it may never get fixed up > at all. Hmm, we get the warnings about bits dropped on each set_offload call: netdev_update_features is called, that calls netdev_fix_features No? > Allowing an unprivileged user to tweak feature bits directly with > no verification is just wrong. > > Cheers, But we do verify bits, and only allow the user to tweak these ones: #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \ NETIF_F_TSO6|NETIF_F_UFO) No? > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2011 at 08:18:45AM +0300, Michael S. Tsirkin wrote: > > Hmm, we get the warnings about bits dropped on each set_offload > call: > netdev_update_features is called, > that calls netdev_fix_features > > No? I presume this has changed recently as the current Linus tree does not contain a call to netdev_update_features, it only calls netdev_features_change. If so then this does ensure that the feature bits will be sane. I still think the warnings should be retained at their previous level in the call paths other than set_offload. All we have to do is add a parameter to netdev_update_features and co. Cheers,
On Tue, May 17, 2011 at 03:24:01PM +1000, Herbert Xu wrote: > On Tue, May 17, 2011 at 08:18:45AM +0300, Michael S. Tsirkin wrote: > > > > Hmm, we get the warnings about bits dropped on each set_offload > > call: > > netdev_update_features is called, > > that calls netdev_fix_features > > > > No? > > I presume this has changed recently as the current Linus tree > does not contain a call to netdev_update_features, it only calls > netdev_features_change. > > If so then this does ensure that the feature bits will be sane. > > I still think the warnings should be retained at their previous > level in the call paths other than set_offload. All we have to > do is add a parameter to netdev_update_features and co. > > Cheers, You mean like bool quiet?
On Tue, May 17, 2011 at 08:48:23AM +0300, Michael S. Tsirkin wrote: > > You mean like bool quiet? Yes when tun calls netdev_update_features we expect it to contain bogus combinations so all warnings should disappear. Everybody should still need maintain sanity. Cheers,
2011/5/17 Herbert Xu <herbert@gondor.apana.org.au>: > On Mon, May 16, 2011 at 02:24:19PM +0200, Michał Mirosław wrote: >> In this case - no. Those messages inform that driver supports more >> feature combinations than network core and the feature set is reduced >> because of that. > The driver should never even claim to support SG/TSO/UFO if it > does not support checksum. That is the point of the warning. Not really. The dependency of SG on checksum is in network core code only, not in the hardware. For TSO/UFO they imply hw checksumming for the respective protocol, but still theres no point in duplicating features dependencies in driver code. In the tun/tap case, by using TUNSETOFFLOAD userspace advertises that it is willing to receive unchecksummed/TSOed packets from kernel (it should use TUN_VNET_HDR for this to work correctly unless it doesn't forward the packets). After the conversion, those offloads can be disabled by using ethtool even if userspace does support them. Previously you were able to push offloaded packets to apllications not prepared for them. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 17 maja 2011 10:08 użytkownik Michał Mirosław <mirqus@gmail.com> napisał: > In the tun/tap case, by using TUNSETOFFLOAD userspace advertises that > it is willing to receive unchecksummed/TSOed packets from kernel (it > should use TUN_VNET_HDR for this to work correctly unless it doesn't > forward the packets). After the conversion, those offloads can be > disabled by using ethtool even if userspace does support them. > Previously you were able to push offloaded packets to apllications not > prepared for them. And there is a bug in the logic implementing this. I'll send a fix in a minute. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 0636f70..eea92e0 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1178,7 +1178,7 @@ static int set_offload(struct tun_struct *tun, unsigned long arg) u32 features = 0; if (arg & TUN_F_CSUM) { - features |= NETIF_F_HW_CSUM; + features |= NETIF_F_HW_CSUM | NETIF_F_SG; arg &= ~TUN_F_CSUM; if (arg & (TUN_F_TSO4|TUN_F_TSO6)) {