| Message ID | aa24cb5cdd3bd082d038960efd7d5c9a2ba95d8e.1310601401.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote:
> There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM.
[...]
I think this is still a useful distinction, even the networking core
currently doesn't care about the difference.
Ben.
On Thu, 14 Jul 2011 01:23:29 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote: > > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM. > [...] > > I think this is still a useful distinction, even the networking core > currently doesn't care about the difference. > > Ben. It also is a kernel API change since this part is exposed through ethtool calls. -- 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 Wed, 2011-07-13 at 17:48 -0700, Stephen Hemminger wrote: > On Thu, 14 Jul 2011 01:23:29 +0100 > Ben Hutchings <bhutchings@solarflare.com> wrote: > > > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote: > > > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM. > > [...] > > > > I think this is still a useful distinction, even the networking core > > currently doesn't care about the difference. > > > > Ben. > > It also is a kernel API change since this part is exposed > through ethtool calls. Users of ETHTOOL_{G,S}FEATURES are supposed to use the corresponding string set to map names to bits (and vice versa). It is OK to renumber features, and to remove them if they are no longer implemented. Ben.
On Wed, Jul 13, 2011 at 05:30:37PM -0700, Tom Herbert wrote: > On Wed, Jul 13, 2011 at 5:23 PM, Ben Hutchings <bhutchings@solarflare.com>wrote: > > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote: > > > There are no explicit users, so this is now equivalent to > > NETIF_F_HW_CSUM. > > [...] > > I think this is still a useful distinction, even the networking core > > currently doesn't care about the difference. > Agreed. It seems like this is the only way to distinguish virtual devices > from HW devices (like we did with nocachecopy check). You can't reliably detect virtual devices by this method. No tunnel devices use this flag and it also doesn't detect e.g. IPsec being used on the route (and no-cache copy should be disabled at least for software encryption). That's why its turned off by default and should be enabled only when user knows he will win some pps with it. 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 Thu, Jul 14, 2011 at 01:23:29AM +0100, Ben Hutchings wrote: > On Thu, 2011-07-14 at 02:10 +0200, Michał Mirosław wrote: > > There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM. > [...] > I think this is still a useful distinction, even the networking core > currently doesn't care about the difference. The problem is that all packets now can be redirected from NO_CSUM device to other (or userspace). If some protocols just ignore checksum marking altogether (like SCTP was doing) because of this flag, you get broken packets that are hard to debug. It costs little to stay on the safe side and calculate this additional u32 with checksum information, even if it stays unused most of the time. That's why I propose to remove NO_CSUM flag - to prevent someone to write such a quietly broken code in the future. 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
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Thu, 14 Jul 2011 22:56:23 +0200 > That's why its turned off by default and should be enabled only when user > knows he will win some pps with it. More people are going to lose than win by your change. The nocopy feature helps more real situations than it hurts, the existing default is the best. -- 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 Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Date: Thu, 14 Jul 2011 22:56:23 +0200 > > That's why its turned off by default and should be enabled only when user > > knows he will win some pps with it. > More people are going to lose than win by your change. > > The nocopy feature helps more real situations than it hurts, the > existing default is the best. I see. I still want to remove NO_CSUM (as I explained in other mail), so would you accept replacing it with something more specific to nocache-copy feature? READS_DATA maybe? That could be later added to sk_route_caps whenever it's known for a route there will be need to read packets' data. 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
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Fri, 15 Jul 2011 00:44:45 +0200 > On Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote: >> From: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Date: Thu, 14 Jul 2011 22:56:23 +0200 >> > That's why its turned off by default and should be enabled only when user >> > knows he will win some pps with it. >> More people are going to lose than win by your change. >> >> The nocopy feature helps more real situations than it hurts, the >> existing default is the best. > > I see. I still want to remove NO_CSUM (as I explained in other mail), > so would you accept replacing it with something more specific to > nocache-copy feature? READS_DATA maybe? That could be later added to > sk_route_caps whenever it's known for a route there will be need to > read packets' data. I don't actually see what the problem is. The code wants to conditionalize the nocache-copy feature based upon whether hardware will checksum the packet or not. And that's exactly what it's testing. The reason, of course, is because it doesn't want to enable nocache-copy if the cpu is just going to read the data back into it's caches during the checksum. But that's no reason to change the flag name to have the word "read" instead of "checksum" in it. -- 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 Thu, Jul 14, 2011 at 05:05:06PM -0700, David Miller wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Date: Fri, 15 Jul 2011 00:44:45 +0200 > > > On Thu, Jul 14, 2011 at 02:31:21PM -0700, David Miller wrote: > >> From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> Date: Thu, 14 Jul 2011 22:56:23 +0200 > >> > That's why its turned off by default and should be enabled only when user > >> > knows he will win some pps with it. > >> More people are going to lose than win by your change. > >> > >> The nocopy feature helps more real situations than it hurts, the > >> existing default is the best. > > > > I see. I still want to remove NO_CSUM (as I explained in other mail), > > so would you accept replacing it with something more specific to > > nocache-copy feature? READS_DATA maybe? That could be later added to > > sk_route_caps whenever it's known for a route there will be need to > > read packets' data. > > I don't actually see what the problem is. > > The code wants to conditionalize the nocache-copy feature based upon > whether hardware will checksum the packet or not. > > And that's exactly what it's testing. > > The reason, of course, is because it doesn't want to enable > nocache-copy if the cpu is just going to read the data back into it's > caches during the checksum. But that's no reason to change the > flag name to have the word "read" instead of "checksum" in it. But the real condition is that CPU doesn't read the data. I doesn't matter if reading is to calculate the checksum or parsing it. The change would make this obvious. BTW, 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/ieee802154/fakehard.c b/drivers/ieee802154/fakehard.c index eb0e2cc..73d4531 100644 --- a/drivers/ieee802154/fakehard.c +++ b/drivers/ieee802154/fakehard.c @@ -343,7 +343,7 @@ static void ieee802154_fake_setup(struct net_device *dev) { dev->addr_len = IEEE802154_ADDR_LEN; memset(dev->broadcast, 0xff, IEEE802154_ADDR_LEN); - dev->features = NETIF_F_NO_CSUM; + dev->features = NETIF_F_HW_CSUM; dev->needed_tailroom = 2; /* FCS */ dev->mtu = 127; dev->tx_queue_len = 10; diff --git a/drivers/misc/sgi-xp/xpnet.c b/drivers/misc/sgi-xp/xpnet.c index 42f0673..3fac67a 100644 --- a/drivers/misc/sgi-xp/xpnet.c +++ b/drivers/misc/sgi-xp/xpnet.c @@ -576,7 +576,7 @@ xpnet_init(void) * report an error if the data is not retrievable and the * packet will be dropped. */ - xpnet_device->features = NETIF_F_NO_CSUM; + xpnet_device->features = NETIF_F_HW_CSUM; result = register_netdev(xpnet_device); if (result != 0) { diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index b9eaf5c..90844a9 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4409,7 +4409,7 @@ static void bond_setup(struct net_device *bond_dev) NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_FILTER; - bond_dev->hw_features &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM); + bond_dev->hw_features &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_HW_CSUM); bond_dev->features |= bond_dev->hw_features; } diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index d0f8c7e..3f405b40 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -442,7 +442,7 @@ static void can_setup(struct net_device *dev) /* New-style flags. */ dev->flags = IFF_NOARP; - dev->features = NETIF_F_NO_CSUM; + dev->features = NETIF_F_HW_CSUM; } struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf) diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c index 1b49df6..d8d9b3e 100644 --- a/drivers/net/can/slcan.c +++ b/drivers/net/can/slcan.c @@ -409,7 +409,7 @@ static void slc_setup(struct net_device *dev) /* New-style flags. */ dev->flags = IFF_NOARP; - dev->features = NETIF_F_NO_CSUM; + dev->features = NETIF_F_HW_CSUM; } /****************************************** diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index 39cf9b9..6b5c186 100644 --- a/drivers/net/dummy.c +++ b/drivers/net/dummy.c @@ -134,7 +134,7 @@ static void dummy_setup(struct net_device *dev) dev->flags |= IFF_NOARP; dev->flags &= ~IFF_MULTICAST; dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_TSO; - dev->features |= NETIF_F_NO_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; + dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; random_ether_addr(dev->dev_addr); } diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index 6e82dd3..a3926a4 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -164,7 +164,7 @@ static const struct net_device_ops ifb_netdev_ops = { .ndo_validate_addr = eth_validate_addr, }; -#define IFB_FEATURES (NETIF_F_NO_CSUM | NETIF_F_SG | NETIF_F_FRAGLIST | \ +#define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_FRAGLIST | \ NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6 | \ NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_TX) diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c index 4ce9e5f..b71998d 100644 --- a/drivers/net/loopback.c +++ b/drivers/net/loopback.c @@ -169,7 +169,7 @@ static void loopback_setup(struct net_device *dev) dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | NETIF_F_UFO - | NETIF_F_NO_CSUM + | NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 7f78db7..9d487d9 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -268,7 +268,7 @@ static void veth_setup(struct net_device *dev) dev->features |= NETIF_F_LLTX; dev->destructor = veth_dev_free; - dev->hw_features = NETIF_F_NO_CSUM | NETIF_F_SG | NETIF_F_RXCSUM; + dev->hw_features = NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_RXCSUM; } /* diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index faa415d..ebace1b 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1027,7 +1027,6 @@ struct net_device { #define NETIF_F_SG 1 /* Scatter/gather IO. */ #define NETIF_F_IP_CSUM 2 /* Can checksum TCP/UDP over IPv4. */ -#define NETIF_F_NO_CSUM 4 /* Does not require checksum. F.e. loopack. */ #define NETIF_F_HW_CSUM 8 /* Can checksum all the packets. */ #define NETIF_F_IPV6_CSUM 16 /* Can checksum TCP/UDP over IPV6 */ #define NETIF_F_HIGHDMA 32 /* Can DMA to high memory. */ @@ -1074,7 +1073,7 @@ struct net_device { NETIF_F_TSO6 | NETIF_F_UFO) -#define NETIF_F_GEN_CSUM (NETIF_F_NO_CSUM | NETIF_F_HW_CSUM) +#define NETIF_F_GEN_CSUM (NETIF_F_HW_CSUM) #define NETIF_F_V4_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM) #define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM) #define NETIF_F_ALL_CSUM (NETIF_F_V4_CSUM | NETIF_F_V6_CSUM) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index a24218c..ec107ac 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -81,7 +81,6 @@ * at device setup time. * NETIF_F_HW_CSUM - it is clever device, it is able to checksum * everything. - * NETIF_F_NO_CSUM - loopback or reliable single hop media. * NETIF_F_IP_CSUM - device is dumb. It is able to csum only * TCP/UDP over IPv4. Sigh. Vendors like this * way by an unknown reason. Though, see comment above diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index 32b8f9f..9d607ba 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -344,10 +344,10 @@ void br_dev_setup(struct net_device *dev) dev->priv_flags = IFF_EBRIDGE; dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA | - NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_LLTX | + NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | NETIF_F_LLTX | NETIF_F_NETNS_LOCAL | NETIF_F_HW_VLAN_TX; dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA | - NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | + NETIF_F_GSO_MASK | NETIF_F_HW_CSUM | NETIF_F_HW_VLAN_TX; br->dev = dev; diff --git a/net/core/dev.c b/net/core/dev.c index 7b9e2f4..09ce52c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5218,12 +5218,6 @@ u32 netdev_fix_features(struct net_device *dev, u32 features) features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM); } - if ((features & NETIF_F_NO_CSUM) && - (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) { - netdev_warn(dev, "mixed no checksumming and other settings.\n"); - features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM); - } - /* Fix illegal SG+CSUM combinations. */ if ((features & NETIF_F_SG) && !(features & NETIF_F_ALL_CSUM)) { @@ -6215,17 +6209,13 @@ static int dev_cpu_callback(struct notifier_block *nfb, */ u32 netdev_increment_features(u32 all, u32 one, u32 mask) { - if (mask & NETIF_F_GEN_CSUM) + if (mask & NETIF_F_HW_CSUM) mask |= NETIF_F_ALL_CSUM; mask |= NETIF_F_VLAN_CHALLENGED; all |= one & (NETIF_F_ONE_FOR_ALL|NETIF_F_ALL_CSUM) & mask; all &= one | ~NETIF_F_ALL_FOR_ALL; - /* If device needs checksumming, downgrade to it. */ - if (all & (NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM)) - all &= ~NETIF_F_NO_CSUM; - /* If one device supports hw checksumming, set for all. */ if (all & NETIF_F_GEN_CSUM) all &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM); diff --git a/net/core/ethtool.c b/net/core/ethtool.c index b7c12a6..3b5cee6 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -341,7 +341,7 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GSTRING_LEN] = { /* NETIF_F_SG */ "tx-scatter-gather", /* NETIF_F_IP_CSUM */ "tx-checksum-ipv4", - /* NETIF_F_NO_CSUM */ "tx-checksum-unneeded", + "", /* NETIF_F_HW_CSUM */ "tx-checksum-ip-generic", /* NETIF_F_IPV6_CSUM */ "tx-checksum-ipv6", /* NETIF_F_HIGHDMA */ "highdma",
There are no explicit users, so this is now equivalent to NETIF_F_HW_CSUM. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/ieee802154/fakehard.c | 2 +- drivers/misc/sgi-xp/xpnet.c | 2 +- drivers/net/bonding/bond_main.c | 2 +- drivers/net/can/dev.c | 2 +- drivers/net/can/slcan.c | 2 +- drivers/net/dummy.c | 2 +- drivers/net/ifb.c | 2 +- drivers/net/loopback.c | 2 +- drivers/net/veth.c | 2 +- include/linux/netdevice.h | 3 +-- include/linux/skbuff.h | 1 - net/bridge/br_device.c | 4 ++-- net/core/dev.c | 12 +----------- net/core/ethtool.c | 2 +- 14 files changed, 14 insertions(+), 26 deletions(-)