net: enable dynamic lro disabling for vlans

net: enable dynamic lro disabling for vlans

[Neil Horman]

Hey there-
	Noted recently that, while physical devices have lro disabled when
attached to a bridge, vlan devices do not.  This is because the vlan netdev has
no get/set flags method in its ethtool_ops struct. This series adds those
methods as passthrough calls to the underlying physical devices, so that whe
dev_disable_lro is called on a vlan device, the physical device underneath has
lro properly disabled.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net

[PATCH 1/2] net: add dev_ethtool_set_flags helper

[Neil Horman]

Add a helper to check for presence of a set_flags method in an ethtool_ops
structure and conditionally call it

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net
---
 include/linux/netdevice.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ca333e7..243781c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2624,6 +2624,13 @@ static inline u32 dev_ethtool_get_flags(struct net_device *dev)
 	return dev->ethtool_ops->get_flags(dev);
 }
 
+static inline int dev_ethtool_set_flags(struct net_device *dev, u32 flags)
+{
+	if (!dev->ethtool_ops || !dev->ethtool_ops->set_flags)
+		return 0;
+	return dev->ethtool_ops->set_flags(dev, flags);
+}
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* netdev_printk helpers, similar to dev_printk */
-- 
1.7.5.1

[PATCH 2/2] net: add passthrough ethtool commands for get/set flags to vlan dev

[Neil Horman]

Add ethtool command to the vlan driver so that when dev_disable_lro is called on
a vlan device, the disablement can be correctly passed down to the underlying
hardware

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net
---
 net/8021q/vlan_dev.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index f247f5b..611839b 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -648,10 +648,24 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st
 	return stats;
 }
 
+static u32 vlan_ethtool_get_flags (struct net_device *vdev)
+{
+	const struct vlan_dev_info *vlan = vlan_dev_info(vdev);
+	return dev_ethtool_get_flags(vlan->real_dev);
+}
+
+static int vlan_ethtool_set_flags(struct net_device *vdev, u32 flags)
+{
+	const struct vlan_dev_info *vlan = vlan_dev_info(vdev);
+	return dev_ethtool_set_flags(vlan->real_dev, flags);
+}
+
 static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_settings	        = vlan_ethtool_get_settings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
+	.get_flags		= vlan_ethtool_get_flags,
+	.set_flags		= vlan_ethtool_set_flags,
 };
 
 static const struct net_device_ops vlan_netdev_ops = {
-- 
1.7.5.1

Re: net: enable dynamic lro disabling for vlans

[Ben Hutchings]

On Tue, 2011-05-24 at 13:15 -0400, Neil Horman wrote:
> Hey there-
> 	Noted recently that, while physical devices have lro disabled when
> attached to a bridge, vlan devices do not.

Good point.

> This is because the vlan netdev has
> no get/set flags method in its ethtool_ops struct. This series adds those
> methods as passthrough calls to the underlying physical devices, so that whe
> dev_disable_lro is called on a vlan device, the physical device underneath has
> lro properly disabled.

But I don't think this is correct.

The get_flags() result should be masked with vlan_features.  And
set_flags() shouldn't be allowed; the VLAN device should normally follow
the parent device and not the other way round.  I think
dev_disable_lro() needs to handle VLAN devices explicitly, instead.

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: net: enable dynamic lro disabling for vlans

[Neil Horman]

On Tue, May 24, 2011 at 10:22:49AM -0700, Ben Hutchings wrote:
> On Tue, 2011-05-24 at 13:15 -0400, Neil Horman wrote:
> > Hey there-
> > 	Noted recently that, while physical devices have lro disabled when
> > attached to a bridge, vlan devices do not.
> 
> Good point.
> 
> > This is because the vlan netdev has
> > no get/set flags method in its ethtool_ops struct. This series adds those
> > methods as passthrough calls to the underlying physical devices, so that whe
> > dev_disable_lro is called on a vlan device, the physical device underneath has
> > lro properly disabled.
> 
> But I don't think this is correct.
> 
> The get_flags() result should be masked with vlan_features.  And
> set_flags() shouldn't be allowed; the VLAN device should normally follow
> the parent device and not the other way round.  I think
I'm not sure thats entirely true (see vlan_ethtool_get_settings, it does a
passthrough to the physical device just like I'm doing, although you're correct
we don't do any passthrough of the set methods).

> dev_disable_lro() needs to handle VLAN devices explicitly, instead.
> 
I suppose that will work just as well as this, and likely will be more concise.
I'll respin shortly.

Best
Neil

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

net: enable dynamic lro disabling for vlans (v2)

[Neil Horman]

Noted recently that, while physical devices have lro disabled when
attached to a bridge, vlan devices do not.  This is because the vlan netdev has
no get/set flags method in its ethtool_ops struct. This series teaches
dev_disable_lro to recognize vlan interfaces and operate on the underlying
physical device instead.

Change notes:
v2) Rewrite of my origional solution As per Ben H.- instead of passing ethtool
command from the vlan device to the physical device, instead teach
dev_disable_lro to recognize vlans and operate on the underlying physical device
directly.  This lets us avoid the various oddities of operating with ethtool on
a vlan device, and is more consice to boot.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net
CC: bhutchings@solarflare.com

[PATCH 1/2] net: move is_vlan_dev into public header file (v2)

[Neil Horman]

Migrate is_vlan_dev() to if_vlan.h so that core networkig can use it

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net
CC: bhutchings@solarflare.com
---
 include/linux/if_vlan.h |    5 +++++
 net/8021q/vlan.h        |    5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 290bd8a..dc01681 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -110,6 +110,11 @@ static inline void vlan_group_set_device(struct vlan_group *vg,
 	array[vlan_id % VLAN_GROUP_ARRAY_PART_LEN] = dev;
 }
 
+static inline int is_vlan_dev(struct net_device *dev)
+{
+        return dev->priv_flags & IFF_802_1Q_VLAN;
+}
+
 #define vlan_tx_tag_present(__skb)	((__skb)->vlan_tci & VLAN_TAG_PRESENT)
 #define vlan_tx_tag_get(__skb)		((__skb)->vlan_tci & ~VLAN_TAG_PRESENT)
 
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index c3408de..9da07e3 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -118,11 +118,6 @@ extern void vlan_netlink_fini(void);
 
 extern struct rtnl_link_ops vlan_link_ops;
 
-static inline int is_vlan_dev(struct net_device *dev)
-{
-	return dev->priv_flags & IFF_802_1Q_VLAN;
-}
-
 extern int vlan_net_id;
 
 struct proc_dir_entry;
-- 
1.7.5.1

Re: [PATCH 1/2] net: move is_vlan_dev into public header file (v2)

[Joe Perches]

On Tue, 2011-05-24 at 14:31 -0400, Neil Horman wrote:
> Migrate is_vlan_dev() to if_vlan.h so that core networkig can use it
[]
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
[]
> @@ -110,6 +110,11 @@ static inline void vlan_group_set_device(struct vlan_group *vg,
>  	array[vlan_id % VLAN_GROUP_ARRAY_PART_LEN] = dev;
>  }
>  
> +static inline int is_vlan_dev(struct net_device *dev)
> +{
> +        return dev->priv_flags & IFF_802_1Q_VLAN;
> +}

perhaps:

static bool is_vlan_dev(const struct net_device *dev)
{
	return !!(dev->priv_flags & IFF_802_1Q_VLAN);
}

Re: [PATCH 1/2] net: move is_vlan_dev into public header file (v2)

[Neil Horman]

On Tue, May 24, 2011 at 11:36:35AM -0700, Joe Perches wrote:
> On Tue, 2011-05-24 at 14:31 -0400, Neil Horman wrote:
> > Migrate is_vlan_dev() to if_vlan.h so that core networkig can use it
> []
> > diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> []
> > @@ -110,6 +110,11 @@ static inline void vlan_group_set_device(struct vlan_group *vg,
> >  	array[vlan_id % VLAN_GROUP_ARRAY_PART_LEN] = dev;
> >  }
> >  
> > +static inline int is_vlan_dev(struct net_device *dev)
> > +{
> > +        return dev->priv_flags & IFF_802_1Q_VLAN;
> > +}
> 
> perhaps:
> 
> static bool is_vlan_dev(const struct net_device *dev)
> {
> 	return !!(dev->priv_flags & IFF_802_1Q_VLAN);
> }
> 
I migrated this directly out of vlan.h as is.  I suppose we could change it, but
I'm not sure I see a need to do so immediately. All callers of this function
already work with it properly as defined currently (not that they wouldn't
otherwise).

Perhaps we could do a separate patch to fix this up as well as other common test
functions (br_port_exists, is_valid_iface, macvlan_port_exists,
netif_is_bond_slave, etc), all just return the result of a bitwise and
currently)

Neil

Re: [PATCH 1/2] net: move is_vlan_dev into public header file (v2)

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 24 May 2011 14:31:08 -0400

> Migrate is_vlan_dev() to if_vlan.h so that core networkig can use it
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

[PATCH 2/2] net: make dev_disable_lro use physical device if passed a vlan dev (v2)

[Neil Horman]

If the device passed into dev_disable_lro is a vlan, then repoint the dev
poniter so that we actually modify the underlying physical device.

Signed-of-by: Neil Horman <nhorman@tuxdriver.com>
CC: davem@davemloft.net
CC: bhutchings@solarflare.com
---
 net/core/dev.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d945379..da16c6a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1308,6 +1308,13 @@ void dev_disable_lro(struct net_device *dev)
 {
 	u32 flags;
 
+	/*
+ 	 * If we're trying to disable lro on a vlan device
+ 	 * use the underlying physical device instead
+ 	 */
+	if (is_vlan_dev(dev))
+		dev = vlan_dev_real_dev(dev);
+
 	if (dev->ethtool_ops && dev->ethtool_ops->get_flags)
 		flags = dev->ethtool_ops->get_flags(dev);
 	else
-- 
1.7.5.1

Re: [PATCH 2/2] net: make dev_disable_lro use physical device if passed a vlan dev (v2)

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 24 May 2011 14:31:09 -0400

> If the device passed into dev_disable_lro is a vlan, then repoint the dev
> poniter so that we actually modify the underlying physical device.
> 
> Signed-of-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Re: [PATCH 2/2] net: make dev_disable_lro use physical device if passed a vlan dev (v2)

[Ben Hutchings]

On Tue, 2011-05-24 at 14:31 -0400, Neil Horman wrote:
> If the device passed into dev_disable_lro is a vlan, then repoint the dev
> poniter so that we actually modify the underlying physical device.
[...]

Thanks Neil, this looks good.

There seems to be a slightly weird bug remaining, in that NETIF_F_LRO
may remain set on the VLAN device.  But that's really cosmetic and
should go away in 2.6.40 along with the old ethtool operations for
offload setting.

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: [PATCH 2/2] net: make dev_disable_lro use physical device if passed a vlan dev (v2)

[Neil Horman]

On Sat, May 28, 2011 at 04:11:03AM +0100, Ben Hutchings wrote:
> On Tue, 2011-05-24 at 14:31 -0400, Neil Horman wrote:
> > If the device passed into dev_disable_lro is a vlan, then repoint the dev
> > poniter so that we actually modify the underlying physical device.
> [...]
> 
> Thanks Neil, this looks good.
> 
> There seems to be a slightly weird bug remaining, in that NETIF_F_LRO
> may remain set on the VLAN device.  But that's really cosmetic and
> should go away in 2.6.40 along with the old ethtool operations for
> offload setting.
> 
> Ben.
> 
Copy that, thanks Ben.
Neil

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