Re: [RFC PATCH 1/4] net/core: Introduce master_xmit_slave_get

[Saeed Mahameed <saeedm@mellanox.com>]

On Sun, 2020-01-26 at 15:21 +0200, Maor Gottlieb wrote:
> Add new ndo to get the xmit slave of master device.
> 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         | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 11bdf6cb30bd..faba4aa094e5 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1379,6 +1379,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_slave_get)(struct
> net_device *master_dev,
> +						      struct sk_buff
> *skb,
> +						      int lag);
>  	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..c710daf8f57a 100644
> --- a/include/net/lag.h
> +++ b/include/net/lag.h
> @@ -6,6 +6,25 @@
>  #include <linux/if_team.h>
>  #include <net/bonding.h>
>  
> +enum lag_get_slaves_flags {
> +	LAG_FLAGS_HASH_ALL_SLAVES = 1<<0
> +};
> +
> +static inline
> +struct net_device *master_xmit_slave_get(struct net_device
> *master_dev,
> +					 struct sk_buff *skb,
> +					 int flags)
> +{
> +	const struct net_device_ops *ops = master_dev->netdev_ops;
> +	struct net_device *slave = NULL;
> +
> +	rcu_read_lock();
> +	if (ops->ndo_xmit_slave_get)
> +		slave = ops->ndo_xmit_slave_get(master_dev, skb,
> flags);

what is the purpose of the rcu ? Aren't you supposed to dev_hold(slave)
under the rcu ?

and the caller should be responsible to issue the dev_put() .. 

otherwise slave is not guaranteed to stick around after this ndo
returns. or i am missing some assumptions that are not listed in this
patchset commit messages or cover letter.

Please clarify.


> +	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))