[1/2] net/mlx5: cache the associated network device ifindex

Message ID 1563514305-27405-2-git-send-email-viacheslavo@mellanox.com
State Superseded, archived
Delegated to: Raslan Darawsheh
Headers show
Series
  • net/mlx5: cache the associated network device ifindex
Related show

Checks

Context Check Description
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Viacheslav Ovsiienko July 19, 2019, 5:31 a.m.
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.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5.c        | 11 +++++++++++
 drivers/net/mlx5/mlx5.h        |  1 +
 drivers/net/mlx5/mlx5_ethdev.c | 19 +++++++------------
 3 files changed, 19 insertions(+), 12 deletions(-)

Comments

Stephen Hemminger July 19, 2019, 4:15 p.m. | #1
On Fri, 19 Jul 2019 05:31:44 +0000
Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:

> +	/*
> +	 * Store associated network device interface index. This index
> +	 * is permanent throughout the lifetime of device. We do not spawn
> +	 * 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 here and use the cached value further. The network
> +	 * device name can be changed dynamically and should not be cached.
> +	 */
> +	assert(spawn->ifindex);
> +	priv->if_index = spawn->ifindex;

This correct, but overkill.

1. The comment is way too wordy. Please stick to only a couple of lines.
   If you feel more explanation is necessary put that in the commit log.
   
2. It is perfectly okay to return 0 as a value in dev_info.
   Therefore the assert is unnecessary.

3. Where is "Reported-by:"

4. What was wrong with my simpler patch?
Viacheslav Ovsiienko July 19, 2019, 4:41 p.m. | #2
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 19, 2019 19:16
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [PATCH 1/2] net/mlx5: cache the associated network device
> ifindex
> 
> On Fri, 19 Jul 2019 05:31:44 +0000
> Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > +	/*
> > +	 * Store associated network device interface index. This index
> > +	 * is permanent throughout the lifetime of device. We do not spawn
> > +	 * 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 here and use the cached value further. The network
> > +	 * device name can be changed dynamically and should not be
> cached.
> > +	 */
> > +	assert(spawn->ifindex);
> > +	priv->if_index = spawn->ifindex;
> 
> This correct, but overkill.
> 
> 1. The comment is way too wordy. Please stick to only a couple of lines.
>    If you feel more explanation is necessary put that in the commit log.

I'd prefer to see the issue description in the source,  not by searching the git log
for the appropriate commit. But OK, it does not matter.
 
> 2. It is perfectly okay to return 0 as a value in dev_info.
>    Therefore the assert is unnecessary.

Valid network interface index cannot be zero. For example, if_nametoindex()
returns zero in case of error. Also, in mlx5 we do not spawn ports without attached
network interfaces. Assert is not related to dev_info, it checks whether
the mlx5_dev_spawn() is called with valid ifindex for valid port (ifindex checked
against zero to validate infiniband port is active). We need this assert here.

> 3. Where is "Reported-by:"
It is in cover letter:
"Proposed-by: Stephen Hemminger <stephen@networkplumber.org>"
 Sorry, I forgot to add this one in commit message, will fix.

> 4. What was wrong with my simpler patch?
Please, see the cover letter. Your patch fixes only the part of problem -
the mlx5_dev_infos_get(). But it is just the case of unsafe mlx5_ifindex() usage.
mlx5_ifindex() itself must be fixed instead.

WBR, Slava
Stephen Hemminger July 19, 2019, 6:03 p.m. | #3
On Fri, 19 Jul 2019 16:41:38 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, July 19, 2019 19:16
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>
> > Subject: Re: [PATCH 1/2] net/mlx5: cache the associated network device
> > ifindex
> > 
> > On Fri, 19 Jul 2019 05:31:44 +0000
> > Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> >   
> > > +	/*
> > > +	 * Store associated network device interface index. This index
> > > +	 * is permanent throughout the lifetime of device. We do not spawn
> > > +	 * 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 here and use the cached value further. The network
> > > +	 * device name can be changed dynamically and should not be  
> > cached.  
> > > +	 */
> > > +	assert(spawn->ifindex);
> > > +	priv->if_index = spawn->ifindex;  
> > 
> > This correct, but overkill.
> > 
> > 1. The comment is way too wordy. Please stick to only a couple of lines.
> >    If you feel more explanation is necessary put that in the commit log.  
> 
> I'd prefer to see the issue description in the source,  not by searching the git log
> for the appropriate commit. But OK, it does not matter.
>  
> > 2. It is perfectly okay to return 0 as a value in dev_info.
> >    Therefore the assert is unnecessary.  
> 
> Valid network interface index cannot be zero. For example, if_nametoindex()
> returns zero in case of error. Also, in mlx5 we do not spawn ports without attached
> network interfaces. Assert is not related to dev_info, it checks whether
> the mlx5_dev_spawn() is called with valid ifindex for valid port (ifindex checked
> against zero to validate infiniband port is active). We need this assert here.
> 
> > 3. Where is "Reported-by:"  
> It is in cover letter:
> "Proposed-by: Stephen Hemminger <stephen@networkplumber.org>"
>  Sorry, I forgot to add this one in commit message, will fix.
> 
> > 4. What was wrong with my simpler patch?  
> Please, see the cover letter. Your patch fixes only the part of problem -
> the mlx5_dev_infos_get(). But it is just the case of unsafe mlx5_ifindex() usage.
> mlx5_ifindex() itself must be fixed instead.
> 
> WBR, Slava

Will your patch be backported to stable?
It is critical that primary/secondary work on older releases.
Viacheslav Ovsiienko July 19, 2019, 6:31 p.m. | #4
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 19, 2019 21:03
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [PATCH 1/2] net/mlx5: cache the associated network device
> ifindex
> 
> On Fri, 19 Jul 2019 16:41:38 +0000
> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Friday, July 19, 2019 19:16
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf
> Shuler
> > > <shahafs@mellanox.com>
> > > Subject: Re: [PATCH 1/2] net/mlx5: cache the associated network
> > > device ifindex
> > >
> > > On Fri, 19 Jul 2019 05:31:44 +0000
> > > Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> > >
> > > > +	/*
> > > > +	 * Store associated network device interface index. This index
> > > > +	 * is permanent throughout the lifetime of device. We do not spawn
> > > > +	 * 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 here and use the cached value further. The network
> > > > +	 * device name can be changed dynamically and should not be
> > > cached.
> > > > +	 */
> > > > +	assert(spawn->ifindex);
> > > > +	priv->if_index = spawn->ifindex;
> > >
> > > This correct, but overkill.
> > >
> > > 1. The comment is way too wordy. Please stick to only a couple of lines.
> > >    If you feel more explanation is necessary put that in the commit log.
> >
> > I'd prefer to see the issue description in the source,  not by
> > searching the git log for the appropriate commit. But OK, it does not
> matter.
> >
> > > 2. It is perfectly okay to return 0 as a value in dev_info.
> > >    Therefore the assert is unnecessary.
> >
> > Valid network interface index cannot be zero. For example,
> > if_nametoindex() returns zero in case of error. Also, in mlx5 we do
> > not spawn ports without attached network interfaces. Assert is not
> > related to dev_info, it checks whether the mlx5_dev_spawn() is called
> > with valid ifindex for valid port (ifindex checked against zero to validate
> infiniband port is active). We need this assert here.
> >
> > > 3. Where is "Reported-by:"
> > It is in cover letter:
> > "Proposed-by: Stephen Hemminger <stephen@networkplumber.org>"
> >  Sorry, I forgot to add this one in commit message, will fix.
> >
> > > 4. What was wrong with my simpler patch?
> > Please, see the cover letter. Your patch fixes only the part of
> > problem - the mlx5_dev_infos_get(). But it is just the case of unsafe
> mlx5_ifindex() usage.
> > mlx5_ifindex() itself must be fixed instead.
> >
> > WBR, Slava
> 
> Will your patch be backported to stable?
> It is critical that primary/secondary work on older releases.
Quite possible.
Is there the full-featured secondary processes support in 18.11 LTS?
I think it's worth to recheck the hotplug feature in 18.11 to avoid
ifindex unexpected change (try to unbound/rebound network device from/to PCI one,
check whether it is disabled/rejected, etc).

With best regards, Slava

Patch

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 608daed..4d1edd8 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1630,6 +1630,17 @@  struct mlx5_dev_spawn_data {
 		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. We do not spawn
+	 * 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 here and use the cached value further. The network
+	 * device name can be changed dynamically and should not be cached.
+	 */
+	assert(spawn->ifindex);
+	priv->if_index = spawn->ifindex;
 	eth_dev->data->dev_private = priv;
 	priv->dev_data = eth_dev->data;
 	eth_dev->data->mac_addrs = priv->mac;
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index f254c8d..4ae6738 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -465,6 +465,7 @@  struct mlx5_priv {
 	uint16_t domain_id; /* Switch domain identifier. */
 	uint16_t vport_id; /* Associated VF vport index (if any). */
 	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 fdd6e03..3803be3 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -225,10 +225,7 @@  struct ethtool_link_settings {
 
 	assert(priv);
 	assert(priv->sh);
-	ifindex = priv->nl_socket_rdma >= 0 ?
-		  mlx5_nl_ifindex(priv->nl_socket_rdma,
-				  priv->sh->ibdev_name,
-				  priv->ibv_port) : 0;
+	ifindex = mlx5_ifindex(dev);
 	if (!ifindex) {
 		if (!priv->representor)
 			return mlx5_get_master_ifname(priv->sh->ibdev_path,
@@ -299,14 +296,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;
 }
 
@@ -641,7 +638,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;
@@ -662,8 +658,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;