| Message ID | 20200420075426.31462-2-maorg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support to get xmit slave | expand |
Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >Add new ndo to get the xmit slave of master device. >User should release the slave when it's not longer needed. >When slave selection method is based on hash, then the user can ask to >get the xmit slave assume all the slaves can transmit by setting the >LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. > >Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >--- > include/linux/netdevice.h | 3 +++ > include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >index 130a668049ab..e8852f3ad0b6 100644 >--- a/include/linux/netdevice.h >+++ b/include/linux/netdevice.h >@@ -1389,6 +1389,9 @@ struct net_device_ops { > struct netlink_ext_ack *extack); > int (*ndo_del_slave)(struct net_device *dev, > struct net_device *slave_dev); >+ struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >+ struct sk_buff *skb, >+ u16 flags); Please adjust the name to: ndo_get_lag_xmit_slave > netdev_features_t (*ndo_fix_features)(struct net_device *dev, > netdev_features_t features); > int (*ndo_set_features)(struct net_device *dev, >diff --git a/include/net/lag.h b/include/net/lag.h >index 95b880e6fdde..c43b035989c4 100644 >--- a/include/net/lag.h >+++ b/include/net/lag.h >@@ -6,6 +6,38 @@ > #include <linux/if_team.h> > #include <net/bonding.h> > >+enum lag_get_slaves_flags { >+ LAG_FLAGS_HASH_ALL_SLAVES = 1<<0 Enum name and the values should be in sync. Also sync with the ndo name. Why exactly do you need these flags? Do you anticipate more of them? A simple bool arg to the ndo would do I believe. Can be changed later if needed. >+}; >+ >+/** >+ * master_xmit_slave_get - Get the xmit slave of master device >+ * @skb: The packet >+ * @flags: lag_get_slaves_flags >+ * >+ * This can be called from any context and does its own locking. >+ * The returned handle has the usage count incremented and the caller must >+ * use dev_put() to release it when it is no longer needed. >+ * %NULL is returned if no slave is found. >+ */ >+ >+static inline >+struct net_device *master_xmit_get_slave(struct net_device *master_dev, Please honor the namespace: net_lag_get_xmit_slave Also, just "struct net_device *dev" would be enough. >+ struct sk_buff *skb, >+ u16 flags) >+{ >+ const struct net_device_ops *ops = master_dev->netdev_ops; >+ struct net_device *slave = NULL; "slave_dev" please. Just check the ndo here and return NULL if it is not defined. That way you avoid unnecessary NULL initialization and rcu lock. >+ >+ rcu_read_lock(); >+ if (ops->ndo_xmit_get_slave) >+ slave = ops->ndo_xmit_get_slave(master_dev, skb, flags); >+ if (slave) >+ dev_hold(slave); >+ rcu_read_unlock(); >+ return slave; >+} >+ > static inline bool net_lag_port_dev_txable(const struct net_device *port_dev) > { > if (netif_is_team_port(port_dev)) >-- >2.17.2 >
On 4/20/20 8:01 AM, Jiri Pirko wrote: > Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >> Add new ndo to get the xmit slave of master device. >> User should release the slave when it's not longer needed. >> When slave selection method is based on hash, then the user can ask to >> get the xmit slave assume all the slaves can transmit by setting the >> LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. >> >> Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >> --- >> include/linux/netdevice.h | 3 +++ >> include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 130a668049ab..e8852f3ad0b6 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1389,6 +1389,9 @@ struct net_device_ops { >> struct netlink_ext_ack *extack); >> int (*ndo_del_slave)(struct net_device *dev, >> struct net_device *slave_dev); >> + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >> + struct sk_buff *skb, >> + u16 flags); > > Please adjust the name to: > ndo_get_lag_xmit_slave I disagree. There are multiple master devices and no reason to have a LAG specific get_slave.
Mon, Apr 20, 2020 at 07:29:15PM CEST, dsahern@gmail.com wrote: >On 4/20/20 8:01 AM, Jiri Pirko wrote: >> Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >>> Add new ndo to get the xmit slave of master device. >>> User should release the slave when it's not longer needed. >>> When slave selection method is based on hash, then the user can ask to >>> get the xmit slave assume all the slaves can transmit by setting the >>> LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. >>> >>> Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >>> --- >>> include/linux/netdevice.h | 3 +++ >>> include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>> index 130a668049ab..e8852f3ad0b6 100644 >>> --- a/include/linux/netdevice.h >>> +++ b/include/linux/netdevice.h >>> @@ -1389,6 +1389,9 @@ struct net_device_ops { >>> struct netlink_ext_ack *extack); >>> int (*ndo_del_slave)(struct net_device *dev, >>> struct net_device *slave_dev); >>> + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >>> + struct sk_buff *skb, >>> + u16 flags); >> >> Please adjust the name to: >> ndo_get_lag_xmit_slave > >I disagree. There are multiple master devices and no reason to have a >LAG specific get_slave. Do you have usecase for any other non-lag master type device? Note the ndo name can change whenever needed. I think the name should reflect the usage.
On 4/20/20 11:41 AM, Jiri Pirko wrote: >> >> I disagree. There are multiple master devices and no reason to have a >> LAG specific get_slave. > > Do you have usecase for any other non-lag master type device? > Note the ndo name can change whenever needed. I think the name should > reflect the usage. > right now, no. But nothing about the current need is LAG specific, so don't make it seem like it is LAG specific with the name.
Mon, Apr 20, 2020 at 07:43:14PM CEST, dsahern@gmail.com wrote: >On 4/20/20 11:41 AM, Jiri Pirko wrote: >>> >>> I disagree. There are multiple master devices and no reason to have a >>> LAG specific get_slave. >> >> Do you have usecase for any other non-lag master type device? >> Note the ndo name can change whenever needed. I think the name should >> reflect the usage. >> > >right now, no. But nothing about the current need is LAG specific, so >don't make it seem like it is LAG specific with the name. I don't care really, I just thought we can make the connection by the name. Makes sense to me.
Mon, Apr 20, 2020 at 07:29:15PM CEST, dsahern@gmail.com wrote: >On 4/20/20 8:01 AM, Jiri Pirko wrote: >> Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >>> Add new ndo to get the xmit slave of master device. >>> User should release the slave when it's not longer needed. >>> When slave selection method is based on hash, then the user can ask to >>> get the xmit slave assume all the slaves can transmit by setting the >>> LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. >>> >>> Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >>> --- >>> include/linux/netdevice.h | 3 +++ >>> include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>> index 130a668049ab..e8852f3ad0b6 100644 >>> --- a/include/linux/netdevice.h >>> +++ b/include/linux/netdevice.h >>> @@ -1389,6 +1389,9 @@ struct net_device_ops { >>> struct netlink_ext_ack *extack); >>> int (*ndo_del_slave)(struct net_device *dev, >>> struct net_device *slave_dev); >>> + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >>> + struct sk_buff *skb, >>> + u16 flags); >> >> Please adjust the name to: >> ndo_get_lag_xmit_slave > >I disagree. There are multiple master devices and no reason to have a >LAG specific get_slave. Btw, did you notice that Maor is passing "lag" named values in the flags?
On 4/20/20 11:54 AM, Jiri Pirko wrote: > Mon, Apr 20, 2020 at 07:29:15PM CEST, dsahern@gmail.com wrote: >> On 4/20/20 8:01 AM, Jiri Pirko wrote: >>> Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >>>> Add new ndo to get the xmit slave of master device. >>>> User should release the slave when it's not longer needed. >>>> When slave selection method is based on hash, then the user can ask to >>>> get the xmit slave assume all the slaves can transmit by setting the >>>> LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. >>>> >>>> Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >>>> --- >>>> include/linux/netdevice.h | 3 +++ >>>> include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ >>>> 2 files changed, 35 insertions(+) >>>> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index 130a668049ab..e8852f3ad0b6 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -1389,6 +1389,9 @@ struct net_device_ops { >>>> struct netlink_ext_ack *extack); >>>> int (*ndo_del_slave)(struct net_device *dev, >>>> struct net_device *slave_dev); >>>> + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >>>> + struct sk_buff *skb, >>>> + u16 flags); >>> >>> Please adjust the name to: >>> ndo_get_lag_xmit_slave >> >> I disagree. There are multiple master devices and no reason to have a >> LAG specific get_slave. > > Btw, did you notice that Maor is passing "lag" named values in the flags? > yes. I disagree with enum name, but having LAG in the name of a flag is fine. To me that is the right place for a LAG specific request of a generic ndo in core code.
Mon, Apr 20, 2020 at 07:56:58PM CEST, dsahern@gmail.com wrote: >On 4/20/20 11:54 AM, Jiri Pirko wrote: >> Mon, Apr 20, 2020 at 07:29:15PM CEST, dsahern@gmail.com wrote: >>> On 4/20/20 8:01 AM, Jiri Pirko wrote: >>>> Mon, Apr 20, 2020 at 09:54:17AM CEST, maorg@mellanox.com wrote: >>>>> Add new ndo to get the xmit slave of master device. >>>>> User should release the slave when it's not longer needed. >>>>> When slave selection method is based on hash, then the user can ask to >>>>> get the xmit slave assume all the slaves can transmit by setting the >>>>> LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. >>>>> >>>>> Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >>>>> --- >>>>> include/linux/netdevice.h | 3 +++ >>>>> include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 35 insertions(+) >>>>> >>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>>> index 130a668049ab..e8852f3ad0b6 100644 >>>>> --- a/include/linux/netdevice.h >>>>> +++ b/include/linux/netdevice.h >>>>> @@ -1389,6 +1389,9 @@ struct net_device_ops { >>>>> struct netlink_ext_ack *extack); >>>>> int (*ndo_del_slave)(struct net_device *dev, >>>>> struct net_device *slave_dev); >>>>> + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, >>>>> + struct sk_buff *skb, >>>>> + u16 flags); >>>> >>>> Please adjust the name to: >>>> ndo_get_lag_xmit_slave >>> >>> I disagree. There are multiple master devices and no reason to have a >>> LAG specific get_slave. >> >> Btw, did you notice that Maor is passing "lag" named values in the flags? >> > >yes. I disagree with enum name, but having LAG in the name of a flag is >fine. To me that is the right place for a LAG specific request of a >generic ndo in core code. Generic ndo with lag-specific arg? Odd. Plus, there is a small chance this is ever going to be used for other master. And if so, could be very easily renamed then...
On 4/20/20 12:01 PM, Jiri Pirko wrote: > Generic ndo with lag-specific arg? Odd. Plus, there is a small chance > this is ever going to be used for other master. And if so, could be very > easily renamed then... core code should be generic, not specific and renamed at a later date when a second use case arises.
Mon, Apr 20, 2020 at 08:04:01PM CEST, dsahern@gmail.com wrote: >On 4/20/20 12:01 PM, Jiri Pirko wrote: >> Generic ndo with lag-specific arg? Odd. Plus, there is a small chance >> this is ever going to be used for other master. And if so, could be very >> easily renamed then... > >core code should be generic, not specific and renamed at a later date >when a second use case arises. Yeah, I guess we just have to agree to disagree :)
On 4/20/2020 9:48 PM, Jiri Pirko wrote: > Mon, Apr 20, 2020 at 08:04:01PM CEST, dsahern@gmail.com wrote: >> On 4/20/20 12:01 PM, Jiri Pirko wrote: >>> Generic ndo with lag-specific arg? Odd. Plus, there is a small chance >>> this is ever going to be used for other master. And if so, could be very >>> easily renamed then... >> core code should be generic, not specific and renamed at a later date >> when a second use case arises. > Yeah, I guess we just have to agree to disagree :) So I am remaining with the flags. Any suggestion for better name for the enum? Should I move master_xmit_get_slave from lag.h to netdevice.h? >
On 4/20/20 12:56 PM, Maor Gottlieb wrote: > > On 4/20/2020 9:48 PM, Jiri Pirko wrote: >> Mon, Apr 20, 2020 at 08:04:01PM CEST, dsahern@gmail.com wrote: >>> On 4/20/20 12:01 PM, Jiri Pirko wrote: >>>> Generic ndo with lag-specific arg? Odd. Plus, there is a small chance >>>> this is ever going to be used for other master. And if so, could be >>>> very >>>> easily renamed then... >>> core code should be generic, not specific and renamed at a later date >>> when a second use case arises. >> Yeah, I guess we just have to agree to disagree :) > > So I am remaining with the flags. Any suggestion for better name for the > enum? Should I move master_xmit_get_slave from lag.h to netdevice.h? >> IMHO, yes, that is a better place. generic ndo name and implementation. type specific flag as needed. This is consistent with net_device and ndo - both generic concepts - with specifics relegated to flags (e.g., IFF_*)
Mon, Apr 20, 2020 at 09:02:58PM CEST, dsahern@gmail.com wrote: >On 4/20/20 12:56 PM, Maor Gottlieb wrote: >> >> On 4/20/2020 9:48 PM, Jiri Pirko wrote: >>> Mon, Apr 20, 2020 at 08:04:01PM CEST, dsahern@gmail.com wrote: >>>> On 4/20/20 12:01 PM, Jiri Pirko wrote: >>>>> Generic ndo with lag-specific arg? Odd. Plus, there is a small chance >>>>> this is ever going to be used for other master. And if so, could be >>>>> very >>>>> easily renamed then... >>>> core code should be generic, not specific and renamed at a later date >>>> when a second use case arises. >>> Yeah, I guess we just have to agree to disagree :) >> >> So I am remaining with the flags. Any suggestion for better name for the >> enum? Should I move master_xmit_get_slave from lag.h to netdevice.h? >>> > >IMHO, yes, that is a better place. > >generic ndo name and implementation. >type specific flag as needed. > >This is consistent with net_device and ndo - both generic concepts - >with specifics relegated to flags (e.g., IFF_*) Why there is need for flags? Why a single bool can't do as I suggested? Do you see any usecase for another flag?
On 4/21/2020 8:37 AM, Jiri Pirko wrote: > Mon, Apr 20, 2020 at 09:02:58PM CEST, dsahern@gmail.com wrote: >> On 4/20/20 12:56 PM, Maor Gottlieb wrote: >>> On 4/20/2020 9:48 PM, Jiri Pirko wrote: >>>> Mon, Apr 20, 2020 at 08:04:01PM CEST, dsahern@gmail.com wrote: >>>>> On 4/20/20 12:01 PM, Jiri Pirko wrote: >>>>>> Generic ndo with lag-specific arg? Odd. Plus, there is a small chance >>>>>> this is ever going to be used for other master. And if so, could be >>>>>> very >>>>>> easily renamed then... >>>>> core code should be generic, not specific and renamed at a later date >>>>> when a second use case arises. >>>> Yeah, I guess we just have to agree to disagree :) >>> So I am remaining with the flags. Any suggestion for better name for the >>> enum? Should I move master_xmit_get_slave from lag.h to netdevice.h? >> IMHO, yes, that is a better place. >> >> generic ndo name and implementation. >> type specific flag as needed. >> >> This is consistent with net_device and ndo - both generic concepts - >> with specifics relegated to flags (e.g., IFF_*) > Why there is need for flags? Why a single bool can't do as I suggested? > Do you see any usecase for another flag? Currently no. I am okay with single bool.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 130a668049ab..e8852f3ad0b6 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1389,6 +1389,9 @@ struct net_device_ops { struct netlink_ext_ack *extack); int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev); + struct net_device* (*ndo_xmit_get_slave)(struct net_device *master_dev, + struct sk_buff *skb, + u16 flags); netdev_features_t (*ndo_fix_features)(struct net_device *dev, netdev_features_t features); int (*ndo_set_features)(struct net_device *dev, diff --git a/include/net/lag.h b/include/net/lag.h index 95b880e6fdde..c43b035989c4 100644 --- a/include/net/lag.h +++ b/include/net/lag.h @@ -6,6 +6,38 @@ #include <linux/if_team.h> #include <net/bonding.h> +enum lag_get_slaves_flags { + LAG_FLAGS_HASH_ALL_SLAVES = 1<<0 +}; + +/** + * master_xmit_slave_get - Get the xmit slave of master device + * @skb: The packet + * @flags: lag_get_slaves_flags + * + * This can be called from any context and does its own locking. + * The returned handle has the usage count incremented and the caller must + * use dev_put() to release it when it is no longer needed. + * %NULL is returned if no slave is found. + */ + +static inline +struct net_device *master_xmit_get_slave(struct net_device *master_dev, + struct sk_buff *skb, + u16 flags) +{ + const struct net_device_ops *ops = master_dev->netdev_ops; + struct net_device *slave = NULL; + + rcu_read_lock(); + if (ops->ndo_xmit_get_slave) + slave = ops->ndo_xmit_get_slave(master_dev, skb, flags); + if (slave) + dev_hold(slave); + rcu_read_unlock(); + return slave; +} + static inline bool net_lag_port_dev_txable(const struct net_device *port_dev) { if (netif_is_team_port(port_dev))
Add new ndo to get the xmit slave of master device. User should release the slave when it's not longer needed. When slave selection method is based on hash, then the user can ask to get the xmit slave assume all the slaves can transmit by setting the LAG_FLAGS_HASH_ALL_SLAVES bit in the flags argument. Signed-off-by: Maor Gottlieb <maorg@mellanox.com> --- include/linux/netdevice.h | 3 +++ include/net/lag.h | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)