| Message ID | 20110517142943.GA926@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote: > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote: > > tun->set_features are meant to limit not force the features. > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > --- > > drivers/net/tun.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 74e9405..f77c6d0 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -458,7 +458,7 @@ static u32 tun_net_fix_features(struct net_device *dev, u32 features) > > { > > struct tun_struct *tun = netdev_priv(dev); > > > > - return (features & tun->set_features) | (features & ~TUN_USER_FEATURES); > > + return features & (tun->set_features | ~TUN_USER_FEATURES); > > } > > > > static const struct net_device_ops tun_netdev_ops = { > > -- > > 1.7.2.5 > > One thing that this will do though: previously, if > ethtool disables offloads, then an application enables > them, the application will have the last say. > With this patch, the most conservative approach wins. > Right? Exactly. On device creation, wanted_features default to all offloads enabled, so unless an admin changes the flags, the application controls what is enabled. This matters only when using persistent tun/tap and admin and user are two different people. If the admin is using queues and doesn't want to handle e.g. TSO packets (I'm not sure if they are properly accounted in all queuing disciplines), then the feature should not be enabled by user. > If we want to have the existing behaviour > I think the following would do this (untested). What do you think? > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 74e9405..1d6c7bc 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg) > return -EINVAL; > > tun->set_features = features; > + tun->dev->features &= TUN_USER_FEATURES; > + tun->dev->features |= (features & TUN_USER_FEATURES); > netdev_update_features(tun->dev); tun->dev->features will be recalculated by netdev_update_features() anyway. For this to work as you described it would need to alter wanted_features. I don't like the idea that something other than one of ethtool_ops is changing this field, as it then becomes something else that what the admin wants (even if that is not what he gets). 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 Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote: > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote: > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote: > > > tun->set_features are meant to limit not force the features. > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > --- > > > drivers/net/tun.c | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > index 74e9405..f77c6d0 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -458,7 +458,7 @@ static u32 tun_net_fix_features(struct net_device *dev, u32 features) > > > { > > > struct tun_struct *tun = netdev_priv(dev); > > > > > > - return (features & tun->set_features) | (features & ~TUN_USER_FEATURES); > > > + return features & (tun->set_features | ~TUN_USER_FEATURES); > > > } > > > > > > static const struct net_device_ops tun_netdev_ops = { > > > -- > > > 1.7.2.5 > > > > One thing that this will do though: previously, if > > ethtool disables offloads, then an application enables > > them, the application will have the last say. > > With this patch, the most conservative approach wins. > > Right? > > Exactly. > > On device creation, wanted_features default to all offloads > enabled, so unless an admin changes the flags, the application controls > what is enabled. This matters only when using persistent tun/tap and > admin and user are two different people. If the admin is using queues > and doesn't want to handle e.g. TSO packets (I'm not sure if they are > properly accounted in all queuing disciplines), then the feature should > not be enabled by user. > > > If we want to have the existing behaviour > > I think the following would do this (untested). What do you think? > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 74e9405..1d6c7bc 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg) > > return -EINVAL; > > > > tun->set_features = features; > > + tun->dev->features &= TUN_USER_FEATURES; > > + tun->dev->features |= (features & TUN_USER_FEATURES); > > netdev_update_features(tun->dev); > > tun->dev->features will be recalculated by netdev_update_features() > anyway. For this to work as you described it would need to alter > wanted_features. I don't like the idea that something other than one > of ethtool_ops is changing this field, as it then becomes something > else that what the admin wants (even if that is not what he gets). > > Best Regards, > Michał Mirosław Yes, with virtualization admin and the app are two different people usually. The device doesn't have to be persistent though I think - what limits this to persistent devices? I agree this behaviour seems more consistent, I just hope this change does not break any setups.
On Tue, May 17, 2011 at 05:54:28PM +0300, Michael S. Tsirkin wrote: > On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote: > > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote: > > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote: > > > > tun->set_features are meant to limit not force the features. [...] > > > One thing that this will do though: previously, if > > > ethtool disables offloads, then an application enables > > > them, the application will have the last say. > > > With this patch, the most conservative approach wins. > > > Right? > > > > Exactly. > > > > On device creation, wanted_features default to all offloads > > enabled, so unless an admin changes the flags, the application controls > > what is enabled. This matters only when using persistent tun/tap and > > admin and user are two different people. If the admin is using queues > > and doesn't want to handle e.g. TSO packets (I'm not sure if they are > > properly accounted in all queuing disciplines), then the feature should > > not be enabled by user. [...] > Yes, with virtualization admin and the app are two different people > usually. The device doesn't have to be persistent though I think - > what limits this to persistent devices? Hmm. Nothing really. I just forgot about the virtualization case. You usually will change the offloads just after device creation unless you're testing or debugging something. > I agree this behaviour seems more consistent, I just hope this change > does not break any setups. The only effect would be some performance drop on cases, where admin turned off the offloads and they stay like that regardless of what user part does. 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 Tue, May 17, 2011 at 05:00:29PM +0200, Michał Mirosław wrote: > On Tue, May 17, 2011 at 05:54:28PM +0300, Michael S. Tsirkin wrote: > > On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote: > > > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote: > > > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote: > > > > > tun->set_features are meant to limit not force the features. > [...] > > > > One thing that this will do though: previously, if > > > > ethtool disables offloads, then an application enables > > > > them, the application will have the last say. > > > > With this patch, the most conservative approach wins. > > > > Right? > > > > > > Exactly. > > > > > > On device creation, wanted_features default to all offloads > > > enabled, so unless an admin changes the flags, the application controls > > > what is enabled. This matters only when using persistent tun/tap and > > > admin and user are two different people. If the admin is using queues > > > and doesn't want to handle e.g. TSO packets (I'm not sure if they are > > > properly accounted in all queuing disciplines), then the feature should > > > not be enabled by user. > [...] > > Yes, with virtualization admin and the app are two different people > > usually. The device doesn't have to be persistent though I think - > > what limits this to persistent devices? > > Hmm. Nothing really. I just forgot about the virtualization case. You > usually will change the offloads just after device creation unless you're > testing or debugging something. That's true. kvm invokes a user script after creating device but just before configuring it, if there might be a problem it's likely only because of something such a script might do (which used to be harmless). My gut feeling is this is unlikely. > > I agree this behaviour seems more consistent, I just hope this change > > does not break any setups. > > The only effect would be some performance drop on cases, where admin turned > off the offloads and they stay like that regardless of what user part does. > > Best Regards, > Michał Mirosław The performance drop is actually quite drastic :), but yes it will keep going, which is a good thing.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 74e9405..1d6c7bc 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg) return -EINVAL; tun->set_features = features; + tun->dev->features &= TUN_USER_FEATURES; + tun->dev->features |= (features & TUN_USER_FEATURES); netdev_update_features(tun->dev); return 0;