[PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes

Hi all,

This patchset aims at allowing dynamically changing a given device
needed_headroom/tailroom space and propagating such events to stacked
devices such as bridges and vlans.

Unless callers use the new helpers (dev_set_headroom/dev_set_tailroom)
there is no functional change introduced.

I tested this with an out of tree Ethernet driver with both VLANs and
bridges and the need for a 64-byte headroom to insert a transmit
status descriptor in front of a SKB.

Since I am not familiar with all subsystems/drivers changing the
needed_headroom/tailroom requirements, I would leave that to them.

Florian Fainelli (3):
  net: add a new NETDEV_CHANGEROOM event type
  net: vlan: handle NETDEV_CHANGEROOM events
  net: bridge: handle NETDEV_CHANGEROOM event

 include/linux/netdevice.h |  3 +++
 net/8021q/vlan.c          |  7 +++++++
 net/bridge/br_if.c        | 32 ++++++++++++++++++++++++++++++++
 net/bridge/br_notify.c    |  5 +++++
 net/bridge/br_private.h   |  2 ++
 net/core/dev.c            | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 95 insertions(+)

-- 
1.8.1.2

The event NETDEV_CHANGEROOM event can be used by devices/subsystems
which need to adjust their needed_headroom and/or needed_tailroom
requirements dynamically. Two helper functions are introduced:

- dev_set_headroom(dev, new_headroom)
- dev_set_taioroom(dev, new_tailroom)

which will notify listeners of such a change.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/netdevice.h |  3 +++
 net/core/dev.c            | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 077363d..2b3e56c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -1652,6 +1652,7 @@ struct packet_offload {
 #define NETDEV_JOIN		0x0014
 #define NETDEV_CHANGEUPPER	0x0015
 #define NETDEV_RESEND_IGMP	0x0016
+#define NETDEV_CHANGEROOM	0x0017 /* needed_headroom/tailroom change */
 
 extern int register_netdevice_notifier(struct notifier_block *nb);
 extern int unregister_netdevice_notifier(struct notifier_block *nb);

@@ -2329,6 +2330,8 @@ extern int		dev_change_net_namespace(struct net_device *,
 						 struct net *, const char *);
 extern int		dev_set_mtu(struct net_device *, int);
 extern void		dev_set_group(struct net_device *, int);
+extern int		dev_set_headroom(struct net_device *, unsigned short);
+extern int		dev_set_tailroom(struct net_device *, unsigned short);
 extern int		dev_set_mac_address(struct net_device *,
 					    struct sockaddr *);
 extern int		dev_change_carrier(struct net_device *,

diff --git a/net/core/dev.c b/net/core/dev.c
index 1ed2b66..be712be 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -4931,6 +4931,52 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
 EXPORT_SYMBOL(dev_set_mtu);
 
 /**
+ *	dev_set_headroom - Change device needed headroom
+ *	@dev: device
+ *	@new_headroom: new headroom size
+ *
+ * 	Change the network device headroom space.
+ */
+int dev_set_headroom(struct net_device *dev, unsigned short new_headroom)
+{
+	if (dev->needed_headroom == new_headroom)
+		return 0;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	dev->needed_headroom = new_headroom;
+
+	call_netdevice_notifiers(NETDEV_CHANGEROOM, dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(dev_set_headroom);
+
+/**
+ *	dev_set_tailroom - Change device needed tailroom
+ *	@dev: device
+ *	@new_tailroom: new tailroom size
+ *
+ * 	Change the network device tailroom space.
+ */
+int dev_set_tailroom(struct net_device *dev, unsigned short new_tailroom)
+{
+	if (dev->needed_tailroom == new_tailroom)
+		return 0;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	dev->needed_tailroom = new_tailroom;
+
+	call_netdevice_notifiers(NETDEV_CHANGEROOM, dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(dev_set_tailroom);
+
+/**
  *	dev_set_group - Change group this device belongs to
  *	@dev: device
  *	@new_group: group this device should belong to

-- 
1.8.1.2

When a device which is a port member of a bridge has
needed_headroom/tailroom requirement changes, we need to walk the list
of bridge members, compute the minimum headroom or tailroom value, and
update the parent bridge device with the new values.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/bridge/br_if.c      | 32 ++++++++++++++++++++++++++++++++
 net/bridge/br_notify.c  |  5 +++++
 net/bridge/br_private.h |  2 ++
 3 files changed, 39 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index aa6c9a8..017622b 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -296,6 +296,38 @@ int br_min_mtu(const struct net_bridge *br)
 	return mtu;
 }
 
+int br_min_headroom(const struct net_bridge *br)
+{
+	const struct net_bridge_port *p;
+	unsigned short needed_headroom = 0;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!needed_headroom ||
+			p->dev->needed_headroom < needed_headroom)
+			needed_headroom = p->dev->needed_headroom;
+	}
+
+	return needed_headroom;
+}
+
+int br_min_tailroom(const struct net_bridge *br)
+{
+	const struct net_bridge_port *p;
+	unsigned short needed_tailroom = 0;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!needed_tailroom ||
+			p->dev->needed_tailroom < needed_tailroom)
+			needed_tailroom = p->dev->needed_tailroom;
+	}
+
+	return needed_tailroom;
+}
+
 /*
  * Recomputes features using slave's features
  */

diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 2998dd1..1c7be7c 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c

@@ -107,6 +107,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		/* Propagate to master device */
 		call_netdevice_notifiers(event, br->dev);
 		break;
+
+	case NETDEV_CHANGEROOM:
+		dev_set_headroom(br->dev, br_min_headroom(br));
+		dev_set_tailroom(br->dev, br_min_tailroom(br));
+		break;
 	}
 
 	/* Events that may cause spanning tree to refresh */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index d41283c..81dc7aa 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -421,6 +421,8 @@ extern int br_add_if(struct net_bridge *br,
 extern int br_del_if(struct net_bridge *br,
 	      struct net_device *dev);
 extern int br_min_mtu(const struct net_bridge *br);
+extern int br_min_headroom(const struct net_bridge *br);
+extern int br_min_tailroom(const struct net_bridge *br);
 extern netdev_features_t br_features_recompute(struct net_bridge *br,
 	netdev_features_t features);
 

-- 
1.8.1.2

Whenever the parent device has needed_headroom/needed_tailroom
requirement changes, the VLAN devices should also be updated to the new
value. Handle the NETDEV_CHANGEROOM events and just set the new headroom
and tailroom requirements to the values of the parent device.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/8021q/vlan.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 61fc573..11a1faa 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c

@@ -466,6 +466,13 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		vlan_group_for_each_dev(grp, i, vlandev)
 			call_netdevice_notifiers(event, vlandev);
 		break;
+
+	case NETDEV_CHANGEROOM:
+		vlan_group_for_each_dev(grp, i, vlandev) {
+			dev_set_headroom(vlandev, dev->needed_headroom);
+			dev_set_tailroom(vlandev, dev->needed_tailroom);
+		}
+		break;
 	}
 
 out:

-- 
1.8.1.2

Tue, Aug 20, 2013 at 02:45:49PM CEST, f.fainelli@gmail.com wrote:

Hi all,

This patchset aims at allowing dynamically changing a given device
needed_headroom/tailroom space and propagating such events to stacked
devices such as bridges and vlans.

You should also add support for other stacked devices, like bonding,
team, macvlan, ovs-datapath, etc.	
	

Unless callers use the new helpers (dev_set_headroom/dev_set_tailroom)
there is no functional change introduced.

I tested this with an out of tree Ethernet driver with both VLANs and
bridges and the need for a 64-byte headroom to insert a transmit
status descriptor in front of a SKB.

Would be nice to add at least one driver which would use your new api.

Thanks,
	Jiri

Since I am not familiar with all subsystems/drivers changing the
needed_headroom/tailroom requirements, I would leave that to them.

Florian Fainelli (3):
 net: add a new NETDEV_CHANGEROOM event type
 net: vlan: handle NETDEV_CHANGEROOM events
 net: bridge: handle NETDEV_CHANGEROOM event

include/linux/netdevice.h |  3 +++
net/8021q/vlan.c          |  7 +++++++
net/bridge/br_if.c        | 32 ++++++++++++++++++++++++++++++++
net/bridge/br_notify.c    |  5 +++++
net/bridge/br_private.h   |  2 ++
net/core/dev.c            | 46 ++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 95 insertions(+)

-- 
1.8.1.2

2013/8/20 Jiri Pirko [off-list ref]:

Tue, Aug 20, 2013 at 02:45:49PM CEST, f.fainelli@gmail.com wrote:

quoted

Hi all,

This patchset aims at allowing dynamically changing a given device
needed_headroom/tailroom space and propagating such events to stacked
devices such as bridges and vlans.

You should also add support for other stacked devices, like bonding,
team, macvlan, ovs-datapath, etc.

Ok, will take a look at these.

quoted

Unless callers use the new helpers (dev_set_headroom/dev_set_tailroom)
there is no functional change introduced.

I tested this with an out of tree Ethernet driver with both VLANs and
bridges and the need for a 64-byte headroom to insert a transmit
status descriptor in front of a SKB.

Would be nice to add at least one driver which would use your new api.

cxgb, niu, ps3_gelic_net and gianfar could probably directly benefit
from this change.

Thanks for your feedback!
--
Florian

Tue, Aug 20, 2013 at 04:24:37PM CEST, f.fainelli@gmail.com wrote:

2013/8/20 Jiri Pirko [off-list ref]:

quoted

Tue, Aug 20, 2013 at 02:45:49PM CEST, f.fainelli@gmail.com wrote:

quoted

Hi all,

This patchset aims at allowing dynamically changing a given device
needed_headroom/tailroom space and propagating such events to stacked
devices such as bridges and vlans.

You should also add support for other stacked devices, like bonding,
team, macvlan, ovs-datapath, etc.

Ok, will take a look at these.

quoted

quoted

Unless callers use the new helpers (dev_set_headroom/dev_set_tailroom)
there is no functional change introduced.

I tested this with an out of tree Ethernet driver with both VLANs and
bridges and the need for a 64-byte headroom to insert a transmit
status descriptor in front of a SKB.

Would be nice to add at least one driver which would use your new api.

cxgb, niu, ps3_gelic_net and gianfar could probably directly benefit
from this change.

Great, please add use of your new api to one of these and add that patch
to your set. Thanks!

Thanks for your feedback!
--
Florian

On Tue, 2013-08-20 at 13:45 +0100, Florian Fainelli wrote:

 /**
+ *	dev_set_headroom - Change device needed headroom
+ *	@dev: device
+ *	@new_headroom: new headroom size
+ *
+ * 	Change the network device headroom space.
+ */
+int dev_set_headroom(struct net_device *dev, unsigned short new_headroom)

It seems that you need to invoke these under RTNL, might be worth
documenting that.

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient? I suppose you could make them 'int' arguments and reserve -1
for no changes, or just require both new values to be given (if doing
this at all.)

johannes

2013/8/20 Johannes Berg [off-list ref]:

On Tue, 2013-08-20 at 13:45 +0100, Florian Fainelli wrote:

quoted

 /**
+ *   dev_set_headroom - Change device needed headroom
+ *   @dev: device
+ *   @new_headroom: new headroom size
+ *
+ *   Change the network device headroom space.
+ */
+int dev_set_headroom(struct net_device *dev, unsigned short new_headroom)

It seems that you need to invoke these under RTNL, might be worth
documenting that.

Good point, yes.

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient?

I have mixed feelings about this. I do not expect changing the
headroom/tailroom to be in a hot-path, and we would need to have a
name such as dev_set_head_and_tailroom() or something that clearly
states that it operates on both quantities. Looking at the subsystems
and drivers, there are quite a lot of users which only set one or the
other, occasionaly both before registration.

I suppose you could make them 'int' arguments and reserve -1
for no changes, or just require both new values to be given (if doing
this at all.)

What I like about keeping them separate is that we can use the
"native" storage type that is used in struct net_device, and have
compile-time checking of this.
-- 
Florian

Tue, Aug 20, 2013 at 05:30:48PM CEST, f.fainelli@gmail.com wrote:

2013/8/20 Johannes Berg [off-list ref]:

quoted

On Tue, 2013-08-20 at 13:45 +0100, Florian Fainelli wrote:

quoted

 /**
+ *   dev_set_headroom - Change device needed headroom
+ *   @dev: device
+ *   @new_headroom: new headroom size
+ *
+ *   Change the network device headroom space.
+ */
+int dev_set_headroom(struct net_device *dev, unsigned short new_headroom)

It seems that you need to invoke these under RTNL, might be worth
documenting that.

Good point, yes.

quoted

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient?

I have mixed feelings about this. I do not expect changing the
headroom/tailroom to be in a hot-path, and we would need to have a
name such as dev_set_head_and_tailroom() or something that clearly
states that it operates on both quantities. Looking at the subsystems
and drivers, there are quite a lot of users which only set one or the
other, occasionaly both before registration.

quoted

I suppose you could make them 'int' arguments and reserve -1

Ugh, -1, I don't like this. I think that they should be set separate. Not
real need to do it in one function.

quoted

for no changes, or just require both new values to be given (if doing
this at all.)

What I like about keeping them separate is that we can use the
"native" storage type that is used in struct net_device, and have
compile-time checking of this.
-- 
Florian

On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:

quoted

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient?

I have mixed feelings about this. I do not expect changing the
headroom/tailroom to be in a hot-path, and we would need to have a
name such as dev_set_head_and_tailroom() or something that clearly
states that it operates on both quantities. Looking at the subsystems
and drivers, there are quite a lot of users which only set one or the
other, occasionaly both before registration.

No, it shouldn't be on a path that has any performance impact at all,
that's true.

quoted

I suppose you could make them 'int' arguments and reserve -1
for no changes, or just require both new values to be given (if doing
this at all.)

What I like about keeping them separate is that we can use the
"native" storage type that is used in struct net_device, and have
compile-time checking of this.

Makes sense.

I was really more thinking about the notifier complexity.

Right now, you can potentially blow up your iterations - for example if
you have a vlan on a bridge:
 * driver sets headroom (or tailroom)
 * this iterates all netdevs, including the bridge
 * bridge calls the function again, and while iterating iterates again,
then
   going into the vlan
   (is it even valid to iterate while iterating?)
 * vlan calls it again and it iterates again, doing nothing this time

So now you've iterated the netdevs many times...

johannes

2013/8/20 Johannes Berg [off-list ref]:

On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:

quoted

quoted

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient?

I have mixed feelings about this. I do not expect changing the
headroom/tailroom to be in a hot-path, and we would need to have a
name such as dev_set_head_and_tailroom() or something that clearly
states that it operates on both quantities. Looking at the subsystems
and drivers, there are quite a lot of users which only set one or the
other, occasionaly both before registration.

No, it shouldn't be on a path that has any performance impact at all,
that's true.

quoted

quoted

I suppose you could make them 'int' arguments and reserve -1
for no changes, or just require both new values to be given (if doing
this at all.)

What I like about keeping them separate is that we can use the
"native" storage type that is used in struct net_device, and have
compile-time checking of this.

Makes sense.

I was really more thinking about the notifier complexity.

Right now, you can potentially blow up your iterations - for example if
you have a vlan on a bridge:
 * driver sets headroom (or tailroom)
 * this iterates all netdevs, including the bridge
 * bridge calls the function again, and while iterating iterates again,
then
   going into the vlan
   (is it even valid to iterate while iterating?)
 * vlan calls it again and it iterates again, doing nothing this time

So now you've iterated the netdevs many times...

That's right, although I am not sure if we can really do something
about it, the network device stacking in that scheme, is just complex
by nature. Fortunately at some point we stop notifying since
dev->headroom == new_headroom. I am pretty sure the same applies
already for NETDEV_CHANGEADDR and NETDEV_CHANGEMTU events today.
-- 
Florian