[RFC PATCH] net: fold dev_disable_lro() into netdev_fix_features()

[RFC PATCH] net: fold dev_disable_lro() into netdev_fix_features()

[Michał Mirosław]

This moves checks that device is forwarding from bridge, IPv4 and IPv6
code into netdev_fix_features(). As a side effect, after device is no longer
forwarding it gets LRO back. This also means that user is not allowed to
enable LRO after device is put to forwarding mode.

This patch depends on removal of discrete offload setting ethtool ops.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/netdevice.h |    1 -
 net/bridge/br_if.c        |    6 +++---
 net/core/dev.c            |   41 +++++++++++++++++++++--------------------
 net/ipv4/devinet.c        |   20 +++++++++-----------
 net/ipv6/addrconf.c       |    7 +++----
 5 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7be3ca2..3a8c21d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1627,7 +1627,6 @@ extern struct net_device	*__dev_get_by_name(struct net *net, const char *name);
 extern int		dev_alloc_name(struct net_device *dev, const char *name);
 extern int		dev_open(struct net_device *dev);
 extern int		dev_close(struct net_device *dev);
-extern void		dev_disable_lro(struct net_device *dev);
 extern int		dev_queue_xmit(struct sk_buff *skb);
 extern int		register_netdevice(struct net_device *dev);
 extern void		unregister_netdevice_queue(struct net_device *dev,
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..62aab1e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 	br_netpoll_disable(p);
 
 	call_rcu(&p->rcu, destroy_nbp_rcu);
+
+	netdev_update_features(dev);
 }
 
 /* called with RTNL */
@@ -368,11 +370,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	dev->priv_flags |= IFF_BRIDGE_PORT;
 
-	dev_disable_lro(dev);
-
 	list_add_rcu(&p->list, &br->port_list);
 
-	netdev_update_features(br->dev);
+	netdev_update_features(dev);
 
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
diff --git a/net/core/dev.c b/net/core/dev.c
index 7193499..3d646c9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -132,6 +132,7 @@
 #include <trace/events/skb.h>
 #include <linux/pci.h>
 #include <linux/inetdevice.h>
+#include <net/addrconf.h>
 #include <linux/cpu_rmap.h>
 
 #include "net-sysfs.h"
@@ -1294,26 +1295,6 @@ int dev_close(struct net_device *dev)
 EXPORT_SYMBOL(dev_close);
 
 
-/**
- *	dev_disable_lro - disable Large Receive Offload on a device
- *	@dev: device
- *
- *	Disable Large Receive Offload (LRO) on a net device.  Must be
- *	called under RTNL.  This is needed if received packets may be
- *	forwarded to another interface.
- */
-void dev_disable_lro(struct net_device *dev)
-{
-	dev->wanted_features &= ~NETIF_F_LRO;
-	netdev_update_features(dev);
-
-	if (unlikely(dev->features & NETIF_F_LRO))
-		netdev_WARN(dev, "failed to disable LRO!\n");
-
-}
-EXPORT_SYMBOL(dev_disable_lro);
-
-
 static int dev_boot_phase = 1;
 
 /**
@@ -5239,6 +5220,26 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		}
 	}
 
+	if (features & NETIF_F_LRO) {
+		struct in_device *in4_dev;
+		struct inet6_dev *in6_dev;
+
+		/* disable LRO for bridge ports */
+		if (dev->priv_flags & IFF_BRIDGE_PORT) {
+			netdev_info(dev, "Disabling LRO for bridge port.\n");
+			features &= NETIF_F_LRO;
+		} else /* ... or when forwarding IPv4 */
+		if (((in4_dev = __in_dev_get_rtnl(dev))) &&
+		    IN_DEV_CONF_GET(in4_dev, FORWARDING)) {
+			netdev_info(dev, "Disabling LRO for IPv4 router port.\n");
+			features &= NETIF_F_LRO;
+		} else /* ... or when forwarding IPv6 */
+		if (((in6_dev = __in6_dev_get(dev))) && in6_dev->cnf.forwarding) {
+			netdev_info(dev, "Disabling LRO for IPv6 router port.\n");
+			features &= NETIF_F_LRO;
+		}
+	}
+
 	return features;
 }
 EXPORT_SYMBOL(netdev_fix_features);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index cd9ca08..e9c0557 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -245,8 +245,6 @@ static struct in_device *inetdev_init(struct net_device *dev)
 	in_dev->arp_parms = neigh_parms_alloc(dev, &arp_tbl);
 	if (!in_dev->arp_parms)
 		goto out_kfree;
-	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
-		dev_disable_lro(dev);
 	/* Reference in_dev->dev */
 	dev_hold(dev);
 	/* Account for reference dev->ip_ptr (below) */
@@ -259,6 +257,8 @@ static struct in_device *inetdev_init(struct net_device *dev)
 
 	/* we can receive as soon as ip_ptr is set -- do this last */
 	rcu_assign_pointer(dev->ip_ptr, in_dev);
+
+	netdev_update_features(dev);
 out:
 	return in_dev;
 out_kfree:
@@ -1475,14 +1475,12 @@ static void inet_forward_change(struct net *net)
 	IPV4_DEVCONF_DFLT(net, FORWARDING) = on;
 
 	for_each_netdev(net, dev) {
-		struct in_device *in_dev;
-		if (on)
-			dev_disable_lro(dev);
-		rcu_read_lock();
-		in_dev = __in_dev_get_rcu(dev);
-		if (in_dev)
+		struct in_device *in_dev = __in_dev_get_rtnl(dev);
+
+		if (in_dev) {
 			IN_DEV_CONF_SET(in_dev, FORWARDING, on);
-		rcu_read_unlock();
+			netdev_update_features(in_dev->dev);
+		}
 	}
 }
 
@@ -1527,11 +1525,11 @@ static int devinet_sysctl_forward(ctl_table *ctl, int write,
 			}
 			if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
 				inet_forward_change(net);
-			} else if (*valp) {
+			} else {
 				struct ipv4_devconf *cnf = ctl->extra1;
 				struct in_device *idev =
 					container_of(cnf, struct in_device, cnf);
-				dev_disable_lro(idev->dev);
+				netdev_update_features(idev->dev);
 			}
 			rtnl_unlock();
 			rt_cache_flush(net, 0);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f2f9b2e..d1344ac 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -370,8 +370,6 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 		kfree(ndev);
 		return NULL;
 	}
-	if (ndev->cnf.forwarding)
-		dev_disable_lro(dev);
 	/* We refer to the device */
 	dev_hold(dev);
 
@@ -435,6 +433,7 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 	addrconf_sysctl_register(ndev);
 	/* protected by rtnl_lock */
 	rcu_assign_pointer(dev->ip6_ptr, ndev);
+	netdev_update_features(dev);
 
 	/* Join all-node multicast group */
 	ipv6_dev_mc_inc(dev, &in6addr_linklocal_allnodes);
@@ -469,8 +468,6 @@ static void dev_forward_change(struct inet6_dev *idev)
 	if (!idev)
 		return;
 	dev = idev->dev;
-	if (idev->cnf.forwarding)
-		dev_disable_lro(dev);
 	if (dev && (dev->flags & IFF_MULTICAST)) {
 		if (idev->cnf.forwarding)
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
@@ -486,6 +483,8 @@ static void dev_forward_change(struct inet6_dev *idev)
 		else
 			addrconf_leave_anycast(ifa);
 	}
+
+	netdev_update_features(dev);
 }
 
 
-- 
1.7.2.5

Re: [RFC PATCH] net: fold dev_disable_lro() into netdev_fix_features()

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Sat,  7 May 2011 13:48:02 +0200 (CEST)

> This moves checks that device is forwarding from bridge, IPv4 and IPv6
> code into netdev_fix_features(). As a side effect, after device is no longer
> forwarding it gets LRO back. This also means that user is not allowed to
> enable LRO after device is put to forwarding mode.
> 
> This patch depends on removal of discrete offload setting ethtool ops.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

We need to keep the check in the protocols because we don't want to
be testing protocol specific device state in generic code like
net/core/dev.c

Re: [RFC PATCH] net: fold dev_disable_lro() into netdev_fix_features()

[Michał Mirosław]

On Mon, May 09, 2011 at 12:08:11PM -0700, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Sat,  7 May 2011 13:48:02 +0200 (CEST)
> > This moves checks that device is forwarding from bridge, IPv4 and IPv6
> > code into netdev_fix_features(). As a side effect, after device is no longer
> > forwarding it gets LRO back. This also means that user is not allowed to
> > enable LRO after device is put to forwarding mode.
> We need to keep the check in the protocols because we don't want to
> be testing protocol specific device state in generic code like
> net/core/dev.c

I modified it to use priv_flags that mirror the protocol-internal state.
Patch follows.

Best Regards,
Michał Mirosław

[RFC PATCH v2] net: fold dev_disable_lro() into netdev_fix_features()

[Michał Mirosław]

This implements checks for forwarding mode in netdev_fix_features() using
dev->priv_flags bits. As a side effect, after device is no longer
forwarding it gets LRO back. This also means that user is not allowed to
enable LRO after device is put to forwarding mode.

This patch depends on removal of discrete offload setting ethtool ops.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: remove protocol-specific checks from core code

 include/linux/if.h        |    5 +++++
 include/linux/netdevice.h |    1 -
 net/bridge/br_if.c        |    6 +++---
 net/core/dev.c            |   25 +++++--------------------
 net/ipv4/devinet.c        |   30 +++++++++++++++++++-----------
 net/ipv6/addrconf.c       |   17 +++++++++++++----
 6 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/include/linux/if.h b/include/linux/if.h
index 3bc63e6..1aef6a7 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -76,6 +76,11 @@
 #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 					 * datapath port */
+#define IFF_FORWARDING_IPV4	0x10000	/* IPv4 router port */
+#define IFF_FORWARDING_IPV6	0x20000	/* IPv6 router port */
+
+#define IFF_LRO_FORBIDDEN (IFF_BRIDGE_PORT | \
+		IFF_FORWARDING_IPV4 | IFF_FORWARDING_IPV6)
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
 #define IF_GET_PROTO	0x0002
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 41972b9..f698730 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1628,7 +1628,6 @@ extern struct net_device	*__dev_get_by_name(struct net *net, const char *name);
 extern int		dev_alloc_name(struct net_device *dev, const char *name);
 extern int		dev_open(struct net_device *dev);
 extern int		dev_close(struct net_device *dev);
-extern void		dev_disable_lro(struct net_device *dev);
 extern int		dev_queue_xmit(struct sk_buff *skb);
 extern int		register_netdevice(struct net_device *dev);
 extern void		unregister_netdevice_queue(struct net_device *dev,
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..568a9dd 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 	br_netpoll_disable(p);
 
 	call_rcu(&p->rcu, destroy_nbp_rcu);
+
+	netdev_update_features(dev);
 }
 
 /* called with RTNL */
@@ -368,11 +370,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	dev->priv_flags |= IFF_BRIDGE_PORT;
 
-	dev_disable_lro(dev);
-
 	list_add_rcu(&p->list, &br->port_list);
 
-	netdev_update_features(br->dev);
+	netdev_change_features(dev);
 
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
diff --git a/net/core/dev.c b/net/core/dev.c
index 491b8eb..b70b6c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1296,26 +1296,6 @@ int dev_close(struct net_device *dev)
 EXPORT_SYMBOL(dev_close);
 
 
-/**
- *	dev_disable_lro - disable Large Receive Offload on a device
- *	@dev: device
- *
- *	Disable Large Receive Offload (LRO) on a net device.  Must be
- *	called under RTNL.  This is needed if received packets may be
- *	forwarded to another interface.
- */
-void dev_disable_lro(struct net_device *dev)
-{
-	dev->wanted_features &= ~NETIF_F_LRO;
-	netdev_update_features(dev);
-
-	if (unlikely(dev->features & NETIF_F_LRO))
-		netdev_WARN(dev, "failed to disable LRO!\n");
-
-}
-EXPORT_SYMBOL(dev_disable_lro);
-
-
 static int dev_boot_phase = 1;
 
 /**
@@ -5241,6 +5221,11 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		}
 	}
 
+	if (features & NETIF_F_LRO && dev->priv_flags & IFF_LRO_FORBIDDEN) {
+		netdev_info(dev, "Disabling LRO for forwarding interface.\n");
+		features &= NETIF_F_LRO;
+	}
+
 	return features;
 }
 EXPORT_SYMBOL(netdev_fix_features);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 0d4a184..9552c68 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -221,6 +221,7 @@ void in_dev_finish_destroy(struct in_device *idev)
 	printk(KERN_DEBUG "in_dev_finish_destroy: %p=%s\n",
 	       idev, dev ? dev->name : "NIL");
 #endif
+	dev->priv_flags &= ~IFF_FORWARDING_IPV4;
 	dev_put(dev);
 	if (!idev->dead)
 		pr_err("Freeing alive in_device %p\n", idev);
@@ -229,6 +230,16 @@ void in_dev_finish_destroy(struct in_device *idev)
 }
 EXPORT_SYMBOL(in_dev_finish_destroy);
 
+static void mark_ipv4_forwarding(struct net_device *dev, bool on)
+{
+	if (on)
+		dev->priv_flags |= IFF_FORWARDING_IPV4;
+	else
+		dev->priv_flags &= ~IFF_FORWARDING_IPV4;
+
+	netdev_update_features(dev);
+}
+
 static struct in_device *inetdev_init(struct net_device *dev)
 {
 	struct in_device *in_dev;
@@ -245,8 +256,7 @@ static struct in_device *inetdev_init(struct net_device *dev)
 	in_dev->arp_parms = neigh_parms_alloc(dev, &arp_tbl);
 	if (!in_dev->arp_parms)
 		goto out_kfree;
-	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
-		dev_disable_lro(dev);
+	mark_ipv4_forwarding(dev, IPV4_DEVCONF(in_dev->cnf, FORWARDING));
 	/* Reference in_dev->dev */
 	dev_hold(dev);
 	/* Account for reference dev->ip_ptr (below) */
@@ -1475,14 +1485,12 @@ static void inet_forward_change(struct net *net)
 	IPV4_DEVCONF_DFLT(net, FORWARDING) = on;
 
 	for_each_netdev(net, dev) {
-		struct in_device *in_dev;
-		if (on)
-			dev_disable_lro(dev);
-		rcu_read_lock();
-		in_dev = __in_dev_get_rcu(dev);
-		if (in_dev)
+		struct in_device *in_dev = __in_dev_get_rtnl(dev);
+
+		if (in_dev) {
 			IN_DEV_CONF_SET(in_dev, FORWARDING, on);
-		rcu_read_unlock();
+			mark_ipv4_forwarding(in_dev->dev, on);
+		}
 	}
 }
 
@@ -1527,11 +1535,11 @@ static int devinet_sysctl_forward(ctl_table *ctl, int write,
 			}
 			if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
 				inet_forward_change(net);
-			} else if (*valp) {
+			} else {
 				struct ipv4_devconf *cnf = ctl->extra1;
 				struct in_device *idev =
 					container_of(cnf, struct in_device, cnf);
-				dev_disable_lro(idev->dev);
+				mark_ipv4_forwarding(idev->dev, *valp);
 			}
 			rtnl_unlock();
 			rt_cache_flush(net, 0);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f2f9b2e..7e0e8fa 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -333,6 +333,7 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 #ifdef NET_REFCNT_DEBUG
 	printk(KERN_DEBUG "in6_dev_finish_destroy: %s\n", dev ? dev->name : "NIL");
 #endif
+	dev->priv_flags &= ~IFF_FORWARDING_IPV6;
 	dev_put(dev);
 	if (!idev->dead) {
 		pr_warning("Freeing alive inet6 device %p\n", idev);
@@ -344,6 +345,16 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 
 EXPORT_SYMBOL(in6_dev_finish_destroy);
 
+static void mark_ipv6_forwarding(struct net_device *dev, bool on)
+{
+	if (on)
+		dev->priv_flags |= IFF_FORWARDING_IPV6;
+	else
+		dev->priv_flags &= ~IFF_FORWARDING_IPV6;
+
+	netdev_update_features(dev);
+}
+
 static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 {
 	struct inet6_dev *ndev;
@@ -370,8 +381,7 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 		kfree(ndev);
 		return NULL;
 	}
-	if (ndev->cnf.forwarding)
-		dev_disable_lro(dev);
+	mark_ipv6_forwarding(dev, ndev->cnf.forwarding);
 	/* We refer to the device */
 	dev_hold(dev);
 
@@ -469,8 +479,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 	if (!idev)
 		return;
 	dev = idev->dev;
-	if (idev->cnf.forwarding)
-		dev_disable_lro(dev);
+	mark_ipv6_forwarding(dev, idev->cnf.forwarding);
 	if (dev && (dev->flags & IFF_MULTICAST)) {
 		if (idev->cnf.forwarding)
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
-- 
1.7.2.5

Re: [RFC PATCH v2] net: fold dev_disable_lro() into netdev_fix_features()

[Ben Hutchings]

On Thu, 2011-05-12 at 18:06 +0200, Michał Mirosław wrote:
> This implements checks for forwarding mode in netdev_fix_features() using
> dev->priv_flags bits. As a side effect, after device is no longer
> forwarding it gets LRO back. This also means that user is not allowed to
> enable LRO after device is put to forwarding mode.
> 
> This patch depends on removal of discrete offload setting ethtool ops.

This is nice, but:

[...]
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
>  	br_netpoll_disable(p);
>  
>  	call_rcu(&p->rcu, destroy_nbp_rcu);
> +
> +	netdev_update_features(dev);
>  }
>  
>  /* called with RTNL */
> @@ -368,11 +370,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	dev->priv_flags |= IFF_BRIDGE_PORT;
>  
> -	dev_disable_lro(dev);
> -
>  	list_add_rcu(&p->list, &br->port_list);
>  
> -	netdev_update_features(br->dev);
> +	netdev_change_features(dev);
>  
>  	spin_lock_bh(&br->lock);
>  	changed_addr = br_stp_recalculate_bridge_id(br);

Why netdev_change_features() here?  I thought that was primarily for use
when vlan_features may have been changed.

[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -5241,6 +5221,11 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
>  		}
>  	}
>  
> +	if (features & NETIF_F_LRO && dev->priv_flags & IFF_LRO_FORBIDDEN) {
> +		netdev_info(dev, "Disabling LRO for forwarding interface.\n");
> +		features &= NETIF_F_LRO;
[...]

Mising '~'.

Ben.

-- 
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: [RFC PATCH v2] net: fold dev_disable_lro() into netdev_fix_features()

[Michał Mirosław]

On Thu, May 12, 2011 at 05:29:07PM +0100, Ben Hutchings wrote:
> On Thu, 2011-05-12 at 18:06 +0200, Michał Mirosław wrote:
> > This implements checks for forwarding mode in netdev_fix_features() using
> > dev->priv_flags bits. As a side effect, after device is no longer
> > forwarding it gets LRO back. This also means that user is not allowed to
> > enable LRO after device is put to forwarding mode.
> > 
> > This patch depends on removal of discrete offload setting ethtool ops.
> 
> This is nice, but:
> 
> [...]
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
> >  	br_netpoll_disable(p);
> >  
> >  	call_rcu(&p->rcu, destroy_nbp_rcu);
> > +
> > +	netdev_update_features(dev);
> >  }
> >  
> >  /* called with RTNL */
> > @@ -368,11 +370,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
> >  
> >  	dev->priv_flags |= IFF_BRIDGE_PORT;
> >  
> > -	dev_disable_lro(dev);
> > -
> >  	list_add_rcu(&p->list, &br->port_list);
> >  
> > -	netdev_update_features(br->dev);
> > +	netdev_change_features(dev);
> >  
> >  	spin_lock_bh(&br->lock);
> >  	changed_addr = br_stp_recalculate_bridge_id(br);
> Why netdev_change_features() here?  I thought that was primarily for use
> when vlan_features may have been changed.

Because this recalculates port's features and then cascades to bridge's
device. I could have written:

	netdev_update_features(dev);
	netdev_update_features(br->dev);

But that might cause redundant recalculations for br->dev (first through
notifier calls). I'll add a comment here.

> 
> [...]
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> [...]
> > @@ -5241,6 +5221,11 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
> >  		}
> >  	}
> >  
> > +	if (features & NETIF_F_LRO && dev->priv_flags & IFF_LRO_FORBIDDEN) {
> > +		netdev_info(dev, "Disabling LRO for forwarding interface.\n");
> > +		features &= NETIF_F_LRO;
> [...]
> 
> Mising '~'.

Fixed.

Best Regards,
Michał Mirosław

[RFC PATCH v3] net: fold dev_disable_lro() into netdev_fix_features()

[Michał Mirosław]

This implements checks for forwarding mode in netdev_fix_features() using
dev->priv_flags bits. As a side effect, after device is no longer
forwarding it gets LRO back. This also means that user is not allowed to
enable LRO after device is put to forwarding mode.

This patch depends on removal of discrete offload setting ethtool ops.

v2: remove protocol-specific checks from core code
v3: add comment and missing negation

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/if.h        |    5 +++++
 include/linux/netdevice.h |    1 -
 net/bridge/br_if.c        |    9 ++++++---
 net/core/dev.c            |   25 +++++--------------------
 net/ipv4/devinet.c        |   30 +++++++++++++++++++-----------
 net/ipv6/addrconf.c       |   17 +++++++++++++----
 6 files changed, 48 insertions(+), 39 deletions(-)

diff --git a/include/linux/if.h b/include/linux/if.h
index 3bc63e6..1aef6a7 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -76,6 +76,11 @@
 #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 					 * datapath port */
+#define IFF_FORWARDING_IPV4	0x10000	/* IPv4 router port */
+#define IFF_FORWARDING_IPV6	0x20000	/* IPv6 router port */
+
+#define IFF_LRO_FORBIDDEN (IFF_BRIDGE_PORT | \
+		IFF_FORWARDING_IPV4 | IFF_FORWARDING_IPV6)
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
 #define IF_GET_PROTO	0x0002
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 41972b9..f698730 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1628,7 +1628,6 @@ extern struct net_device	*__dev_get_by_name(struct net *net, const char *name);
 extern int		dev_alloc_name(struct net_device *dev, const char *name);
 extern int		dev_open(struct net_device *dev);
 extern int		dev_close(struct net_device *dev);
-extern void		dev_disable_lro(struct net_device *dev);
 extern int		dev_queue_xmit(struct sk_buff *skb);
 extern int		register_netdevice(struct net_device *dev);
 extern void		unregister_netdevice_queue(struct net_device *dev,
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..31ba100 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 	br_netpoll_disable(p);
 
 	call_rcu(&p->rcu, destroy_nbp_rcu);
+
+	netdev_update_features(dev);
 }
 
 /* called with RTNL */
@@ -368,11 +370,12 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	dev->priv_flags |= IFF_BRIDGE_PORT;
 
-	dev_disable_lro(dev);
-
 	list_add_rcu(&p->list, &br->port_list);
 
-	netdev_update_features(br->dev);
+	/* possibly disable LRO and force recalculation of bridge dev's
+	 * features through NETDEV_FEAT_CHANGE notifier call.
+	 */
+	netdev_change_features(dev);
 
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
diff --git a/net/core/dev.c b/net/core/dev.c
index 491b8eb..7c7df7c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1296,26 +1296,6 @@ int dev_close(struct net_device *dev)
 EXPORT_SYMBOL(dev_close);
 
 
-/**
- *	dev_disable_lro - disable Large Receive Offload on a device
- *	@dev: device
- *
- *	Disable Large Receive Offload (LRO) on a net device.  Must be
- *	called under RTNL.  This is needed if received packets may be
- *	forwarded to another interface.
- */
-void dev_disable_lro(struct net_device *dev)
-{
-	dev->wanted_features &= ~NETIF_F_LRO;
-	netdev_update_features(dev);
-
-	if (unlikely(dev->features & NETIF_F_LRO))
-		netdev_WARN(dev, "failed to disable LRO!\n");
-
-}
-EXPORT_SYMBOL(dev_disable_lro);
-
-
 static int dev_boot_phase = 1;
 
 /**
@@ -5241,6 +5221,11 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		}
 	}
 
+	if (features & NETIF_F_LRO && dev->priv_flags & IFF_LRO_FORBIDDEN) {
+		netdev_info(dev, "Disabling LRO for forwarding interface.\n");
+		features &= ~NETIF_F_LRO;
+	}
+
 	return features;
 }
 EXPORT_SYMBOL(netdev_fix_features);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 0d4a184..9552c68 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -221,6 +221,7 @@ void in_dev_finish_destroy(struct in_device *idev)
 	printk(KERN_DEBUG "in_dev_finish_destroy: %p=%s\n",
 	       idev, dev ? dev->name : "NIL");
 #endif
+	dev->priv_flags &= ~IFF_FORWARDING_IPV4;
 	dev_put(dev);
 	if (!idev->dead)
 		pr_err("Freeing alive in_device %p\n", idev);
@@ -229,6 +230,16 @@ void in_dev_finish_destroy(struct in_device *idev)
 }
 EXPORT_SYMBOL(in_dev_finish_destroy);
 
+static void mark_ipv4_forwarding(struct net_device *dev, bool on)
+{
+	if (on)
+		dev->priv_flags |= IFF_FORWARDING_IPV4;
+	else
+		dev->priv_flags &= ~IFF_FORWARDING_IPV4;
+
+	netdev_update_features(dev);
+}
+
 static struct in_device *inetdev_init(struct net_device *dev)
 {
 	struct in_device *in_dev;
@@ -245,8 +256,7 @@ static struct in_device *inetdev_init(struct net_device *dev)
 	in_dev->arp_parms = neigh_parms_alloc(dev, &arp_tbl);
 	if (!in_dev->arp_parms)
 		goto out_kfree;
-	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
-		dev_disable_lro(dev);
+	mark_ipv4_forwarding(dev, IPV4_DEVCONF(in_dev->cnf, FORWARDING));
 	/* Reference in_dev->dev */
 	dev_hold(dev);
 	/* Account for reference dev->ip_ptr (below) */
@@ -1475,14 +1485,12 @@ static void inet_forward_change(struct net *net)
 	IPV4_DEVCONF_DFLT(net, FORWARDING) = on;
 
 	for_each_netdev(net, dev) {
-		struct in_device *in_dev;
-		if (on)
-			dev_disable_lro(dev);
-		rcu_read_lock();
-		in_dev = __in_dev_get_rcu(dev);
-		if (in_dev)
+		struct in_device *in_dev = __in_dev_get_rtnl(dev);
+
+		if (in_dev) {
 			IN_DEV_CONF_SET(in_dev, FORWARDING, on);
-		rcu_read_unlock();
+			mark_ipv4_forwarding(in_dev->dev, on);
+		}
 	}
 }
 
@@ -1527,11 +1535,11 @@ static int devinet_sysctl_forward(ctl_table *ctl, int write,
 			}
 			if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
 				inet_forward_change(net);
-			} else if (*valp) {
+			} else {
 				struct ipv4_devconf *cnf = ctl->extra1;
 				struct in_device *idev =
 					container_of(cnf, struct in_device, cnf);
-				dev_disable_lro(idev->dev);
+				mark_ipv4_forwarding(idev->dev, *valp);
 			}
 			rtnl_unlock();
 			rt_cache_flush(net, 0);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f2f9b2e..7e0e8fa 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -333,6 +333,7 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 #ifdef NET_REFCNT_DEBUG
 	printk(KERN_DEBUG "in6_dev_finish_destroy: %s\n", dev ? dev->name : "NIL");
 #endif
+	dev->priv_flags &= ~IFF_FORWARDING_IPV6;
 	dev_put(dev);
 	if (!idev->dead) {
 		pr_warning("Freeing alive inet6 device %p\n", idev);
@@ -344,6 +345,16 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 
 EXPORT_SYMBOL(in6_dev_finish_destroy);
 
+static void mark_ipv6_forwarding(struct net_device *dev, bool on)
+{
+	if (on)
+		dev->priv_flags |= IFF_FORWARDING_IPV6;
+	else
+		dev->priv_flags &= ~IFF_FORWARDING_IPV6;
+
+	netdev_update_features(dev);
+}
+
 static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 {
 	struct inet6_dev *ndev;
@@ -370,8 +381,7 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 		kfree(ndev);
 		return NULL;
 	}
-	if (ndev->cnf.forwarding)
-		dev_disable_lro(dev);
+	mark_ipv6_forwarding(dev, ndev->cnf.forwarding);
 	/* We refer to the device */
 	dev_hold(dev);
 
@@ -469,8 +479,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 	if (!idev)
 		return;
 	dev = idev->dev;
-	if (idev->cnf.forwarding)
-		dev_disable_lro(dev);
+	mark_ipv6_forwarding(dev, idev->cnf.forwarding);
 	if (dev && (dev->flags & IFF_MULTICAST)) {
 		if (idev->cnf.forwarding)
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
-- 
1.7.2.5

Re: [RFC PATCH v2] net: fold dev_disable_lro() into netdev_fix_features()

[Stephen Hemminger]

On Thu, 12 May 2011 17:29:07 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> >  	dev->priv_flags |= IFF_BRIDGE_PORT;
> >  
> > -	dev_disable_lro(dev);
> > -
> >  	list_add_rcu(&p->list, &br->port_list);
> >  
> > -	netdev_update_features(br->dev);
> > +	netdev_change_features(dev);
> >  
> >  	spin_lock_bh(&br->lock);
> >  	changed_addr = br_stp_recalculate_bridge_id(br);  
> 
> Why netdev_change_features() here?  I thought that was primarily for use
> when vlan_features may have been changed.

Setting IFF_BRIDGE_PORT in priv_flags causes change_features
to disable LRO.

--