| Message ID | 1377002752-4622-2-git-send-email-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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.
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 -- 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 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 -- 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
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.
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
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(+)