Re: [PATCH V2 mlx5-next 01/10] net/core: Introduce master_xmit_slave_get

[Jiri Pirko <jiri@resnulli.us>]

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
>