* [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes
@ 2013-08-20 12:45 Florian Fainelli
2013-08-20 12:45 ` [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type Florian Fainelli
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 12:45 UTC (permalink / raw)
To: netdev
Cc: amwang, jiri, stephen, kaber, davem, vyasevic, johannes,
eric.dumazet, Florian Fainelli
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 12:45 [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Florian Fainelli
@ 2013-08-20 12:45 ` Florian Fainelli
2013-08-20 15:16 ` Johannes Berg
2013-08-20 12:45 ` [PATCH 2/3] net: vlan: handle NETDEV_CHANGEROOM events Florian Fainelli
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 12:45 UTC (permalink / raw)
To: netdev
Cc: amwang, jiri, stephen, kaber, davem, vyasevic, johannes,
eric.dumazet, Florian Fainelli
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] net: vlan: handle NETDEV_CHANGEROOM events
2013-08-20 12:45 [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Florian Fainelli
2013-08-20 12:45 ` [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type Florian Fainelli
@ 2013-08-20 12:45 ` Florian Fainelli
2013-08-20 12:45 ` [PATCH 3/3] net: bridge: handle NETDEV_CHANGEROOM event Florian Fainelli
2013-08-20 13:29 ` [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Jiri Pirko
3 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 12:45 UTC (permalink / raw)
To: netdev
Cc: amwang, jiri, stephen, kaber, davem, vyasevic, johannes,
eric.dumazet, Florian Fainelli
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] net: bridge: handle NETDEV_CHANGEROOM event
2013-08-20 12:45 [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Florian Fainelli
2013-08-20 12:45 ` [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type Florian Fainelli
2013-08-20 12:45 ` [PATCH 2/3] net: vlan: handle NETDEV_CHANGEROOM events Florian Fainelli
@ 2013-08-20 12:45 ` Florian Fainelli
2013-08-20 13:29 ` [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Jiri Pirko
3 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 12:45 UTC (permalink / raw)
To: netdev
Cc: amwang, jiri, stephen, kaber, davem, vyasevic, johannes,
eric.dumazet, Florian Fainelli
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes
2013-08-20 12:45 [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Florian Fainelli
` (2 preceding siblings ...)
2013-08-20 12:45 ` [PATCH 3/3] net: bridge: handle NETDEV_CHANGEROOM event Florian Fainelli
@ 2013-08-20 13:29 ` Jiri Pirko
2013-08-20 14:24 ` Florian Fainelli
3 siblings, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2013-08-20 13:29 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, amwang, stephen, kaber, davem, vyasevic, johannes,
eric.dumazet
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
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes
2013-08-20 13:29 ` [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Jiri Pirko
@ 2013-08-20 14:24 ` Florian Fainelli
2013-08-20 14:27 ` Jiri Pirko
0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 14:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, amwang, Stephen Hemminger, kaber, David Miller, vyasevic,
Johannes Berg, Eric Dumazet
2013/8/20 Jiri Pirko <jiri@resnulli.us>:
> 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.
Ok, will take a look at these.
>
>>
>>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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes
2013-08-20 14:24 ` Florian Fainelli
@ 2013-08-20 14:27 ` Jiri Pirko
0 siblings, 0 replies; 12+ messages in thread
From: Jiri Pirko @ 2013-08-20 14:27 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, amwang, Stephen Hemminger, kaber, David Miller, vyasevic,
Johannes Berg, Eric Dumazet
Tue, Aug 20, 2013 at 04:24:37PM CEST, f.fainelli@gmail.com wrote:
>2013/8/20 Jiri Pirko <jiri@resnulli.us>:
>> 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.
>
>Ok, will take a look at these.
>
>>
>>>
>>>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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 12:45 ` [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type Florian Fainelli
@ 2013-08-20 15:16 ` Johannes Berg
2013-08-20 15:30 ` Florian Fainelli
0 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2013-08-20 15:16 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, amwang, jiri, stephen, kaber, davem, vyasevic,
eric.dumazet
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 15:16 ` Johannes Berg
@ 2013-08-20 15:30 ` Florian Fainelli
2013-08-20 16:10 ` Jiri Pirko
2013-08-20 16:35 ` Johannes Berg
0 siblings, 2 replies; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 15:30 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, amwang, Jiri Pirko, Stephen Hemminger, kaber,
David Miller, vyasevic, Eric Dumazet
2013/8/20 Johannes Berg <johannes@sipsolutions.net>:
> 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.
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 15:30 ` Florian Fainelli
@ 2013-08-20 16:10 ` Jiri Pirko
2013-08-20 16:35 ` Johannes Berg
1 sibling, 0 replies; 12+ messages in thread
From: Jiri Pirko @ 2013-08-20 16:10 UTC (permalink / raw)
To: Florian Fainelli
Cc: Johannes Berg, netdev, amwang, Stephen Hemminger, kaber,
David Miller, vyasevic, Eric Dumazet
Tue, Aug 20, 2013 at 05:30:48PM CEST, f.fainelli@gmail.com wrote:
>2013/8/20 Johannes Berg <johannes@sipsolutions.net>:
>> 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.
>
>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
Ugh, -1, I don't like this. I think that they should be set separate. Not
real need to do it in one function.
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 15:30 ` Florian Fainelli
2013-08-20 16:10 ` Jiri Pirko
@ 2013-08-20 16:35 ` Johannes Berg
2013-08-20 17:30 ` Florian Fainelli
1 sibling, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2013-08-20 16:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, amwang, Jiri Pirko, Stephen Hemminger, kaber,
David Miller, vyasevic, Eric Dumazet
On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:
> > 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.
> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type
2013-08-20 16:35 ` Johannes Berg
@ 2013-08-20 17:30 ` Florian Fainelli
0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2013-08-20 17:30 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, amwang, Jiri Pirko, Stephen Hemminger, kaber,
David Miller, vyasevic, Eric Dumazet
2013/8/20 Johannes Berg <johannes@sipsolutions.net>:
> On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:
>
>> > 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.
>
>> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-08-20 17:30 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-20 12:45 [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Florian Fainelli
2013-08-20 12:45 ` [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type Florian Fainelli
2013-08-20 15:16 ` Johannes Berg
2013-08-20 15:30 ` Florian Fainelli
2013-08-20 16:10 ` Jiri Pirko
2013-08-20 16:35 ` Johannes Berg
2013-08-20 17:30 ` Florian Fainelli
2013-08-20 12:45 ` [PATCH 2/3] net: vlan: handle NETDEV_CHANGEROOM events Florian Fainelli
2013-08-20 12:45 ` [PATCH 3/3] net: bridge: handle NETDEV_CHANGEROOM event Florian Fainelli
2013-08-20 13:29 ` [PATCH 0/3] net: propagate dynamic needed_headroom/tailroom changes Jiri Pirko
2013-08-20 14:24 ` Florian Fainelli
2013-08-20 14:27 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).