Re: [dpdk-stable] [18.11] net/mlx5: cache associated network device index

[Kevin Traynor <ktraynor@redhat.com>]

On 28/01/2020 15:27, Alexander Kozyrev wrote:
> From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> [ upstream commit fa2e14d4921a2c4a1268231e72ccd73d1e7e58fc ]
> 
> The associated device index is retrieved via Netlink request to
> underlying Infiniband device driver. This network device index
> is permanent throughout the lifetime of device. We do not
> spawn the rte_eth_dev ports without associated network device, and
> if network device is being unbound we get the remove notification
> message and rte_eth_dev port is also detached. So, we may store
> the ifindex in mlx5_device_spawn() routine at rte_eth_dev port
> creation and initialization time and use the cached value further
> instead of doing actual Netlink request.
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c        | 11 +++++++++--
>  drivers/net/mlx5/mlx5.h        |  1 +
>  drivers/net/mlx5/mlx5_ethdev.c | 20 +++++++++-----------
>  3 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index d7833d0..29370f5 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -734,7 +734,8 @@
>  mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	       struct ibv_device *ibv_dev,
>  	       struct mlx5_dev_config config,
> -	       const struct mlx5_switch_info *switch_info)
> +	       const struct mlx5_switch_info *switch_info,
> +	       unsigned int ifindex)
>  {
>  	struct ibv_context *ctx;
>  	struct ibv_device_attr_ex attr;
> @@ -1132,6 +1133,12 @@
>  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
>  		eth_dev->data->representor_id = priv->representor_id;
>  	}
> +	/*
> +	 * Store associated network device interface index. This index
> +	 * is permanent throughout the lifetime of device. So, we may store
> +	 * the ifindex here and use the cached value further.
> +	 */
> +	priv->if_index = ifindex;
>  	eth_dev->data->dev_private = priv;
>  	priv->dev_data = eth_dev->data;
>  	eth_dev->data->mac_addrs = priv->mac;
> @@ -1492,7 +1499,7 @@ struct mlx5_dev_spawn_data {
>  
>  		list[i].eth_dev = mlx5_dev_spawn(&pci_dev->device,
>  						 list[i].ibv_dev, dev_config,
> -						 &list[i].info);
> +						 &list[i].info, list[i].ifindex);
>  		if (!list[i].eth_dev) {
>  			if (rte_errno != EBUSY && rte_errno != EEXIST)
>  				break;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index b409f20..26cbdbc 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -197,6 +197,7 @@ struct mlx5_priv {
>  	unsigned int representor:1; /* Device is a port representor. */
>  	uint16_t domain_id; /* Switch domain identifier. */
>  	int32_t representor_id; /* Port representor identifier. */
> +	unsigned int if_index; /* Associated kernel network device index. */
>  	/* RX/TX queues. */
>  	unsigned int rxqs_n; /* RX queues array size. */
>  	unsigned int txqs_n; /* TX queues array size. */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 9c7fc6b..d49cb59 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -220,10 +220,10 @@ struct ethtool_link_settings {
>  mlx5_get_ifname(const struct rte_eth_dev *dev, char (*ifname)[IF_NAMESIZE])
>  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> -	unsigned int ifindex =
> -		priv->nl_socket_rdma >= 0 ?
> -		mlx5_nl_ifindex(priv->nl_socket_rdma, priv->ibdev_name) : 0;
> +	unsigned int ifindex;
>  
> +	assert(priv);
> +	ifindex = mlx5_ifindex(dev);
>  	if (!ifindex) {
>  		if (!priv->representor)
>  			return mlx5_get_master_ifname(dev, ifname);
> @@ -248,14 +248,14 @@ struct ethtool_link_settings {
>  unsigned int
>  mlx5_ifindex(const struct rte_eth_dev *dev)
>  {
> -	char ifname[IF_NAMESIZE];
> +	struct mlx5_priv *priv = dev->data->dev_private;
>  	unsigned int ifindex;
>  
> -	if (mlx5_get_ifname(dev, &ifname))
> -		return 0;
> -	ifindex = if_nametoindex(ifname);
> +	assert(priv);
> +	assert(priv->if_index);
> +	ifindex = priv->if_index;
>  	if (!ifindex)
> -		rte_errno = errno;
> +		rte_errno = ENXIO;
>  	return ifindex;
>  }
>  
> @@ -502,7 +502,6 @@ struct ethtool_link_settings {
>  	struct mlx5_priv *priv = dev->data->dev_private;
>  	struct mlx5_dev_config *config = &priv->config;
>  	unsigned int max;
> -	char ifname[IF_NAMESIZE];
>  
>  	/* FIXME: we should ask the device for these values. */
>  	info->min_rx_bufsize = 32;
> @@ -523,8 +522,7 @@ struct ethtool_link_settings {
>  	info->rx_offload_capa = (mlx5_get_rx_port_offloads() |
>  				 info->rx_queue_offload_capa);
>  	info->tx_offload_capa = mlx5_get_tx_port_offloads(dev);
> -	if (mlx5_get_ifname(dev, &ifname) == 0)
> -		info->if_index = if_nametoindex(ifname);
> +	info->if_index = mlx5_ifindex(dev);
>  	info->reta_size = priv->reta_idx_n ?
>  		priv->reta_idx_n : config->ind_table_max_size;
>  	info->hash_key_size = MLX5_RSS_HASH_KEY_LEN;
> 

Applied, thanks.