tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

BTW, I just noticed that net-next spits out
many of the following when I run any VMs:

tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x401b4849 -> 0x40004040
device msttap0 entered promiscuous mode
br0: Dropping NETIF_F_GSO since no SG feature.
br0: port 1(msttap0) entering forwarding state
br0: port 1(msttap0) entering forwarding state
tap0: Features changed: 0x40004040 -> 0x40024849
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x40024849 -> 0x40004040
br0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x40004040 -> 0x401b4849
tap0: Dropping NETIF_F_SG since no checksum feature.
tap0: Dropping NETIF_F_GSO since no SG feature.
tap0: Features changed: 0x401b4849 -> 0x40004040
br0: Dropping NETIF_F_GSO since no SG feature.

My problem is not primarily with warnings:

will that linearize all packets and disable GSO
for tap and bridge? If yes it can't be good
for performance...

-- 
MST

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
> BTW, I just noticed that net-next spits out
> many of the following when I run any VMs:
> 
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Features changed: 0x401b4849 -> 0x40004040
> device msttap0 entered promiscuous mode
> br0: Dropping NETIF_F_GSO since no SG feature.
> br0: port 1(msttap0) entering forwarding state
> br0: port 1(msttap0) entering forwarding state
> tap0: Features changed: 0x40004040 -> 0x40024849
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Features changed: 0x40024849 -> 0x40004040
> br0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Features changed: 0x40004040 -> 0x401b4849
> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Features changed: 0x401b4849 -> 0x40004040
> br0: Dropping NETIF_F_GSO since no SG feature.
> 
> My problem is not primarily with warnings:
> 
> will that linearize all packets and disable GSO
> for tap and bridge? If yes it can't be good
> for performance...

I think so.  So the question is why is checksum off?

Cheers,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

On Thu, May 05, 2011 at 08:34:15AM +1000, Herbert Xu wrote:
> On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
> > BTW, I just noticed that net-next spits out
> > many of the following when I run any VMs:
> > 
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Features changed: 0x401b4849 -> 0x40004040
> > device msttap0 entered promiscuous mode
> > br0: Dropping NETIF_F_GSO since no SG feature.
> > br0: port 1(msttap0) entering forwarding state
> > br0: port 1(msttap0) entering forwarding state
> > tap0: Features changed: 0x40004040 -> 0x40024849
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Features changed: 0x40024849 -> 0x40004040
> > br0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Features changed: 0x40004040 -> 0x401b4849
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Features changed: 0x401b4849 -> 0x40004040
> > br0: Dropping NETIF_F_GSO since no SG feature.
> > 
> > My problem is not primarily with warnings:
> > 
> > will that linearize all packets and disable GSO
> > for tap and bridge? If yes it can't be good
> > for performance...
> I think so.  So the question is why is checksum off?

Whatever application is creating the tap0 device is not calling
ioctl(TUNSETOFFLOAD) with TUN_F_CSUM set. This is userspace bug/feature
exposed by recent changes to netdev features handling.

Best Regards,
Michał Mirosław

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

On Thu, May 05, 2011 at 01:28:54AM +0200, Michał Mirosław wrote:
>
> Whatever application is creating the tap0 device is not calling
> ioctl(TUNSETOFFLOAD) with TUN_F_CSUM set. This is userspace bug/feature
> exposed by recent changes to netdev features handling.

OK, then user-space needs to be fixed.

Cheers,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

On Thu, May 05, 2011 at 01:28:54AM +0200, Michał Mirosław wrote:
> On Thu, May 05, 2011 at 08:34:15AM +1000, Herbert Xu wrote:
> > On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
> > > BTW, I just noticed that net-next spits out
> > > many of the following when I run any VMs:
> > > 
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Features changed: 0x401b4849 -> 0x40004040
> > > device msttap0 entered promiscuous mode
> > > br0: Dropping NETIF_F_GSO since no SG feature.
> > > br0: port 1(msttap0) entering forwarding state
> > > br0: port 1(msttap0) entering forwarding state
> > > tap0: Features changed: 0x40004040 -> 0x40024849
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Features changed: 0x40024849 -> 0x40004040
> > > br0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Features changed: 0x40004040 -> 0x401b4849
> > > tap0: Dropping NETIF_F_SG since no checksum feature.
> > > tap0: Dropping NETIF_F_GSO since no SG feature.
> > > tap0: Features changed: 0x401b4849 -> 0x40004040
> > > br0: Dropping NETIF_F_GSO since no SG feature.
> > > 
> > > My problem is not primarily with warnings:
> > > 
> > > will that linearize all packets and disable GSO
> > > for tap and bridge? If yes it can't be good
> > > for performance...
> > I think so.  So the question is why is checksum off?
> 
> Whatever application is creating the tap0 device is not calling
> ioctl(TUNSETOFFLOAD) with TUN_F_CSUM set. This is userspace bug/feature
> exposed by recent changes to netdev features handling.
> 
> Best Regards,
> Michał Mirosław

No, I think it's not a bug in userspace. Here is what it does:

    /* Check if our kernel supports TUNSETOFFLOAD */
    if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) {
        return;
    }

    if (csum) {
        offload |= TUN_F_CSUM;
        if (tso4)
            offload |= TUN_F_TSO4;
        if (tso6)
            offload |= TUN_F_TSO6;
        if ((tso4 || tso6) && ecn)
            offload |= TUN_F_TSO_ECN;
        if (ufo)
            offload |= TUN_F_UFO;
    }

    if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
        offload &= ~TUN_F_UFO;
        if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
            fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n",
                    strerror(errno));
        }
    }

The first call is just to check that ioctl works.
When checking for ioctl in this way, userspace clears checksum.
This will clear SG and thus GSO; later userspace enables checksum.
checksum is on but SG is by now disabled so GSO gets cleared again too.


It's also likely a problem that
userspace can trigger warnings in log for what used to be
a legal way to check for ioctl and/or disable checksum offloading,
but that is more minor.

-- 
MST

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Shan Wei]

Michael S. Tsirkin wrote, at 05/05/2011 04:44 PM:
> On Thu, May 05, 2011 at 01:28:54AM +0200, Michał Mirosław wrote:
>> On Thu, May 05, 2011 at 08:34:15AM +1000, Herbert Xu wrote:
>>> On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
>>>> BTW, I just noticed that net-next spits out
>>>> many of the following when I run any VMs:
>>>>
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Features changed: 0x401b4849 -> 0x40004040
>>>> device msttap0 entered promiscuous mode
>>>> br0: Dropping NETIF_F_GSO since no SG feature.
>>>> br0: port 1(msttap0) entering forwarding state
>>>> br0: port 1(msttap0) entering forwarding state
>>>> tap0: Features changed: 0x40004040 -> 0x40024849
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Features changed: 0x40024849 -> 0x40004040
>>>> br0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Features changed: 0x40004040 -> 0x401b4849
>>>> tap0: Dropping NETIF_F_SG since no checksum feature.
>>>> tap0: Dropping NETIF_F_GSO since no SG feature.
>>>> tap0: Features changed: 0x401b4849 -> 0x40004040
>>>> br0: Dropping NETIF_F_GSO since no SG feature.
>>>>
>>>> My problem is not primarily with warnings:
>>>>
>>>> will that linearize all packets and disable GSO
>>>> for tap and bridge? If yes it can't be good
>>>> for performance...
>>> I think so.  So the question is why is checksum off?
>>
>> Whatever application is creating the tap0 device is not calling
>> ioctl(TUNSETOFFLOAD) with TUN_F_CSUM set. This is userspace bug/feature
>> exposed by recent changes to netdev features handling.
>>
>> Best Regards,
>> Michał Mirosław
> 
> No, I think it's not a bug in userspace. Here is what it does:
> 
>     /* Check if our kernel supports TUNSETOFFLOAD */
>     if (ioctl(fd, TUNSETOFFLOAD, 0) != 0 && errno == EINVAL) {
>         return;
>     }
> 
>     if (csum) {
>         offload |= TUN_F_CSUM;
>         if (tso4)
>             offload |= TUN_F_TSO4;
>         if (tso6)
>             offload |= TUN_F_TSO6;
>         if ((tso4 || tso6) && ecn)
>             offload |= TUN_F_TSO_ECN;
>         if (ufo)
>             offload |= TUN_F_UFO;
>     }
> 
>     if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
>         offload &= ~TUN_F_UFO;
>         if (ioctl(fd, TUNSETOFFLOAD, offload) != 0) {
>             fprintf(stderr, "TUNSETOFFLOAD ioctl() failed: %s\n",
>                     strerror(errno));
>         }
>     }
> 
> The first call is just to check that ioctl works.
> When checking for ioctl in this way, userspace clears checksum.
> This will clear SG and thus GSO; later userspace enables checksum.
> checksum is on but SG is by now disabled so GSO gets cleared again too.
> 
> 
> It's also likely a problem that
> userspace can trigger warnings in log for what used to be
> a legal way to check for ioctl and/or disable checksum offloading,
> but that is more minor.

Maybe it's a kernel bug. Can you try following changes?

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().


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)) {


-- 
Best Regards
-----
Shan Wei

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
> BTW, I just noticed that net-next spits out
> many of the following when I run any VMs:
[...]
> tap0: Features changed: 0x40004040 -> 0x401b4849

Before this message, userspace called ioctl(TIOCSETOFFLOAD)
turning offloads on.

> tap0: Dropping NETIF_F_SG since no checksum feature.
> tap0: Dropping NETIF_F_GSO since no SG feature.
> tap0: Features changed: 0x401b4849 -> 0x40004040

And then it probably called ioctl(TIOCSETOFFLOAD) again, disabling them.

Best Regards,
Michał Mirosław

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Shan Wei]

Michał Mirosław wrote, at 05/05/2011 11:26 PM:
> On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
>> BTW, I just noticed that net-next spits out
>> many of the following when I run any VMs:
> [...]
>> tap0: Features changed: 0x40004040 -> 0x401b4849
> 
> Before this message, userspace called ioctl(TIOCSETOFFLOAD)
> turning offloads on.
> 
>> tap0: Dropping NETIF_F_SG since no checksum feature.
>> tap0: Dropping NETIF_F_GSO since no SG feature.
>> tap0: Features changed: 0x401b4849 -> 0x40004040
> 
> And then it probably called ioctl(TIOCSETOFFLOAD) again, disabling them.

[263958.167861] tap1: Dropping NETIF_F_SG since no checksum feature.
[263958.167866] tap1: Dropping NETIF_F_GSO since no SG feature.
[263958.167871] tap1: Features changed: 0x401b4849 -> 0x40004040

See same warning message using tunctl to create a tap device on net-next tree.
But on RHEL6, no warning message.

strace shows no TUNSETOFFLOAD to be used.
Seems that checksum feature is not set when creating tap0 interface.

#strace tunctl -u root -t tap1
open("/dev/net/tun", O_RDWR)            = 3
ioctl(3, TUNSETIFF, 0x7fff789ada10)     = 0
ioctl(3, TUNSETOWNER, 0)                = 0
ioctl(3, TUNSETPERSIST, 0x1)            = 0
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 0), ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f66aeed9000
write(1, "Set 'tap1' persistent and owned "..., 41Set 'tap1' persistent and owned by uid 0
) = 41
exit_group(0)                           = ?


-- 
Best Regards
-----
Shan Wei

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

On Thu, May 05, 2011 at 05:26:44PM +0200, Michał Mirosław wrote:
> On Wed, May 04, 2011 at 09:18:14PM +0300, Michael S. Tsirkin wrote:
> > BTW, I just noticed that net-next spits out
> > many of the following when I run any VMs:
> [...]
> > tap0: Features changed: 0x40004040 -> 0x401b4849
> 
> Before this message, userspace called ioctl(TIOCSETOFFLOAD)
> turning offloads on.
> 
> > tap0: Dropping NETIF_F_SG since no checksum feature.
> > tap0: Dropping NETIF_F_GSO since no SG feature.
> > tap0: Features changed: 0x401b4849 -> 0x40004040
> 
> And then it probably called ioctl(TIOCSETOFFLOAD) again, disabling them.
> 
> Best Regards,
> Michał Mirosław


I'd have to look at this some more, but I know qemu-kvm
supports offloads with current kernels.
There seems to be some kernel behaviour change here.

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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.

-- 
MST

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[David Miller]

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.

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michael S. Tsirkin]

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?

-- 
MST

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Herbert Xu]

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,
-- 
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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

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

Re: tap/bridge: Dropping NETIF_F_GSO/NETIF_F_SG

[Michał Mirosław]

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

[PATCH] net: tuntap: Fix tun_net_fix_features()

[Michał Mirosław]

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

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michael S. Tsirkin]

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?
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);
 
 	return 0;

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michał Mirosław]

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

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michał Mirosław]

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

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [PATCH] net: tuntap: Fix tun_net_fix_features()

[Michael S. Tsirkin]

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>

Not sure when I'll find the time to test this but
just looking at the code, the patch makes sense
to me. So:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

We probably want this in -stable too?

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

[RESENT PATCH] net: tuntap: Fix tun_net_fix_features()

[Michał Mirosław]

tun->set_features are meant to limit not force the features.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Acked-by: Michael S. Tsirkin <mst@redhat.com>

---
 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 4dab85e..56981c2 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

Re: [RESENT PATCH] net: tuntap: Fix tun_net_fix_features()

[Ben Hutchings]

On Mon, 2011-06-20 at 21:14 +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>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
>  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 4dab85e..56981c2 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);

(A & B) | (A & C) <=> A & (B | C)

This is a cosmetic change, not a fix.  Maybe you think it is clearer but
then you should say that in the commit message.

Ben.

>  }
>  
>  static const struct net_device_ops tun_netdev_ops = {

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RESENT PATCH] net: tuntap: Fix tun_net_fix_features()

[Michał Mirosław]

On Mon, Jun 20, 2011 at 08:25:06PM +0100, Ben Hutchings wrote:
> On Mon, 2011-06-20 at 21:14 +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>
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> >  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 4dab85e..56981c2 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);
> 
> (A & B) | (A & C) <=> A & (B | C)
> 
> This is a cosmetic change, not a fix.  Maybe you think it is clearer but
> then you should say that in the commit message.

Hmm. Noise through my braincells, I suppose. Let's just drop this patch.

Best Regards,
Michał Mirosław