[dpdk-dev] [PATCH 0/2] fix dev_info_get in mlx secondary process

[dpdk-dev] [PATCH 0/2] fix dev_info_get in mlx secondary process

[Stephen Hemminger]

Both drivers had the same problem.

Stephen Hemminger (2):
  net/mlx4: fix crash in dev_info_get in secondary process
  net/mlx5: fix crash in dev_info_get in secondary process

 drivers/net/mlx4/mlx4.c        | 19 +++++++++----------
 drivers/net/mlx4/mlx4.h        |  1 +
 drivers/net/mlx4/mlx4_ethdev.c |  4 +---
 drivers/net/mlx5/mlx5.c        | 17 ++++++++---------
 drivers/net/mlx5/mlx5.h        |  1 +
 drivers/net/mlx5/mlx5_ethdev.c |  4 +---
 6 files changed, 21 insertions(+), 25 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in secondary process

[Stephen Hemminger]

mlx4_dev_info_get calls mlx4_get_ifname, but mlx4_get_ifname
uses priv->ctx which is not a valid pointer in a secondary
process. The fix is to cache the value in primary.

In the primary process, get and store the interface index of
the device so that secondary process can see it.

Bugzilla ID:320
Fixes: 61cbdd419478 ("net/mlx4: separate device control functions")
Cc: stable@dpdk.org
Reported-by: Suyang Ju <sju@paloaltonetworks.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/mlx4/mlx4.c        | 19 +++++++++----------
 drivers/net/mlx4/mlx4.h        |  1 +
 drivers/net/mlx4/mlx4_ethdev.c |  4 +---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 2e169b0887a7..bab2cadbe519 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -763,6 +763,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 	};
 	unsigned int vf;
 	int i;
+	char ifname[IF_NAMESIZE];
 
 	(void)pci_drv;
 	err = mlx4_init_once();
@@ -1002,17 +1003,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		     mac.addr_bytes[4], mac.addr_bytes[5]);
 		/* Register MAC address. */
 		priv->mac[0] = mac;
-#ifndef NDEBUG
-		{
-			char ifname[IF_NAMESIZE];
-
-			if (mlx4_get_ifname(priv, &ifname) == 0)
-				DEBUG("port %u ifname is \"%s\"",
-				      priv->port, ifname);
-			else
-				DEBUG("port %u ifname is unknown", priv->port);
+
+		if (mlx4_get_ifname(priv, &ifname) == 0) {
+			DEBUG("port %u ifname is \"%s\"",
+			      priv->port, ifname);
+			priv->if_index = if_nametoindex(ifname);
+		} else {
+			DEBUG("port %u ifname is unknown", priv->port);
 		}
-#endif
+
 		/* Get actual MTU if possible. */
 		mlx4_mtu_get(priv, &priv->mtu);
 		DEBUG("port %u MTU is %u", priv->port, priv->mtu);
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index cd0d637ac2bf..81b529ee8030 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -159,6 +159,7 @@ struct mlx4_priv {
 	struct ibv_device_attr device_attr; /**< Device properties. */
 	struct ibv_pd *pd; /**< Protection Domain. */
 	/* Device properties. */
+	unsigned int if_index;	/**< Associated network device index */
 	uint16_t mtu; /**< Configured MTU. */
 	uint8_t port; /**< Physical port number. */
 	uint32_t started:1; /**< Device started, flows enabled. */
diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
index ceef921620a8..5d28c0116d21 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -616,7 +616,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 {
 	struct mlx4_priv *priv = dev->data->dev_private;
 	unsigned int max;
-	char ifname[IF_NAMESIZE];
 
 	/* FIXME: we should ask the device for these values. */
 	info->min_rx_bufsize = 32;
@@ -637,8 +636,7 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	info->rx_queue_offload_capa = mlx4_get_rx_queue_offloads(priv);
 	info->rx_offload_capa = (mlx4_get_rx_port_offloads(priv) |
 				 info->rx_queue_offload_capa);
-	if (mlx4_get_ifname(priv, &ifname) == 0)
-		info->if_index = if_nametoindex(ifname);
+	info->if_index = priv->if_index;
 	info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE;
 	info->speed_capa =
 			ETH_LINK_SPEED_1G |
-- 
2.20.1

Re: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in secondary process

[Matan Azrad]

Hi Stephen

 From: Stephen Hemminger
> mlx4_dev_info_get calls mlx4_get_ifname, but mlx4_get_ifname uses priv-
> >ctx which is not a valid pointer in a secondary process. The fix is to cache the
> value in primary.
> 
> In the primary process, get and store the interface index of the device so
> that secondary process can see it.
> 
> Bugzilla ID:320
> Fixes: 61cbdd419478 ("net/mlx4: separate device control functions")
> Cc: stable@dpdk.org
> Reported-by: Suyang Ju <sju@paloaltonetworks.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Matan Azrad <matan@mellanox.com>

Thanks for the fix.

> ---
>  drivers/net/mlx4/mlx4.c        | 19 +++++++++----------
>  drivers/net/mlx4/mlx4.h        |  1 +
>  drivers/net/mlx4/mlx4_ethdev.c |  4 +---
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index
> 2e169b0887a7..bab2cadbe519 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -763,6 +763,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct
> rte_pci_device *pci_dev)
>  	};
>  	unsigned int vf;
>  	int i;
> +	char ifname[IF_NAMESIZE];
> 
>  	(void)pci_drv;
>  	err = mlx4_init_once();
> @@ -1002,17 +1003,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv,
> struct rte_pci_device *pci_dev)
>  		     mac.addr_bytes[4], mac.addr_bytes[5]);
>  		/* Register MAC address. */
>  		priv->mac[0] = mac;
> -#ifndef NDEBUG
> -		{
> -			char ifname[IF_NAMESIZE];
> -
> -			if (mlx4_get_ifname(priv, &ifname) == 0)
> -				DEBUG("port %u ifname is \"%s\"",
> -				      priv->port, ifname);
> -			else
> -				DEBUG("port %u ifname is unknown", priv-
> >port);
> +
> +		if (mlx4_get_ifname(priv, &ifname) == 0) {
> +			DEBUG("port %u ifname is \"%s\"",
> +			      priv->port, ifname);
> +			priv->if_index = if_nametoindex(ifname);
> +		} else {
> +			DEBUG("port %u ifname is unknown", priv->port);
>  		}
> -#endif
> +
>  		/* Get actual MTU if possible. */
>  		mlx4_mtu_get(priv, &priv->mtu);
>  		DEBUG("port %u MTU is %u", priv->port, priv->mtu); diff --git
> a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> cd0d637ac2bf..81b529ee8030 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -159,6 +159,7 @@ struct mlx4_priv {
>  	struct ibv_device_attr device_attr; /**< Device properties. */
>  	struct ibv_pd *pd; /**< Protection Domain. */
>  	/* Device properties. */
> +	unsigned int if_index;	/**< Associated network device index */
>  	uint16_t mtu; /**< Configured MTU. */
>  	uint8_t port; /**< Physical port number. */
>  	uint32_t started:1; /**< Device started, flows enabled. */ diff --git
> a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c index
> ceef921620a8..5d28c0116d21 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -616,7 +616,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)  {
>  	struct mlx4_priv *priv = dev->data->dev_private;
>  	unsigned int max;
> -	char ifname[IF_NAMESIZE];
> 
>  	/* FIXME: we should ask the device for these values. */
>  	info->min_rx_bufsize = 32;
> @@ -637,8 +636,7 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	info->rx_queue_offload_capa = mlx4_get_rx_queue_offloads(priv);
>  	info->rx_offload_capa = (mlx4_get_rx_port_offloads(priv) |
>  				 info->rx_queue_offload_capa);
> -	if (mlx4_get_ifname(priv, &ifname) == 0)
> -		info->if_index = if_nametoindex(ifname);
> +	info->if_index = priv->if_index;
>  	info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE;
>  	info->speed_capa =
>  			ETH_LINK_SPEED_1G |
> --
> 2.20.1

Re: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in secondary process

[Raslan Darawsheh]

Hi Stephen, 

Wrong headline format:
        net/mlx4: fix crash in dev_info_get in secondary process
can you please fix it? You shouldn't use the _ in the title format

Kindest regards,
Raslan Darawsheh

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Friday, July 12, 2019 11:54 PM
> To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Yongseok Koh <yskoh@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; sju@paloaltonetworks.com; Stephen Hemminger
> <stephen@networkplumber.org>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in
> secondary process
> 
> mlx4_dev_info_get calls mlx4_get_ifname, but mlx4_get_ifname
> uses priv->ctx which is not a valid pointer in a secondary
> process. The fix is to cache the value in primary.
> 
> In the primary process, get and store the interface index of
> the device so that secondary process can see it.
> 
> Bugzilla ID:320
> Fixes: 61cbdd419478 ("net/mlx4: separate device control functions")
> Cc: stable@dpdk.org
> Reported-by: Suyang Ju <sju@paloaltonetworks.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/mlx4/mlx4.c        | 19 +++++++++----------
>  drivers/net/mlx4/mlx4.h        |  1 +
>  drivers/net/mlx4/mlx4_ethdev.c |  4 +---
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 2e169b0887a7..bab2cadbe519 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -763,6 +763,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct
> rte_pci_device *pci_dev)
>  	};
>  	unsigned int vf;
>  	int i;
> +	char ifname[IF_NAMESIZE];
> 
>  	(void)pci_drv;
>  	err = mlx4_init_once();
> @@ -1002,17 +1003,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv,
> struct rte_pci_device *pci_dev)
>  		     mac.addr_bytes[4], mac.addr_bytes[5]);
>  		/* Register MAC address. */
>  		priv->mac[0] = mac;
> -#ifndef NDEBUG
> -		{
> -			char ifname[IF_NAMESIZE];
> -
> -			if (mlx4_get_ifname(priv, &ifname) == 0)
> -				DEBUG("port %u ifname is \"%s\"",
> -				      priv->port, ifname);
> -			else
> -				DEBUG("port %u ifname is unknown", priv-
> >port);
> +
> +		if (mlx4_get_ifname(priv, &ifname) == 0) {
> +			DEBUG("port %u ifname is \"%s\"",
> +			      priv->port, ifname);
> +			priv->if_index = if_nametoindex(ifname);
> +		} else {
> +			DEBUG("port %u ifname is unknown", priv->port);
>  		}
> -#endif
> +
>  		/* Get actual MTU if possible. */
>  		mlx4_mtu_get(priv, &priv->mtu);
>  		DEBUG("port %u MTU is %u", priv->port, priv->mtu);
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index cd0d637ac2bf..81b529ee8030 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -159,6 +159,7 @@ struct mlx4_priv {
>  	struct ibv_device_attr device_attr; /**< Device properties. */
>  	struct ibv_pd *pd; /**< Protection Domain. */
>  	/* Device properties. */
> +	unsigned int if_index;	/**< Associated network device index */
>  	uint16_t mtu; /**< Configured MTU. */
>  	uint8_t port; /**< Physical port number. */
>  	uint32_t started:1; /**< Device started, flows enabled. */
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> b/drivers/net/mlx4/mlx4_ethdev.c
> index ceef921620a8..5d28c0116d21 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -616,7 +616,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  {
>  	struct mlx4_priv *priv = dev->data->dev_private;
>  	unsigned int max;
> -	char ifname[IF_NAMESIZE];
> 
>  	/* FIXME: we should ask the device for these values. */
>  	info->min_rx_bufsize = 32;
> @@ -637,8 +636,7 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	info->rx_queue_offload_capa = mlx4_get_rx_queue_offloads(priv);
>  	info->rx_offload_capa = (mlx4_get_rx_port_offloads(priv) |
>  				 info->rx_queue_offload_capa);
> -	if (mlx4_get_ifname(priv, &ifname) == 0)
> -		info->if_index = if_nametoindex(ifname);
> +	info->if_index = priv->if_index;
>  	info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE;
>  	info->speed_capa =
>  			ETH_LINK_SPEED_1G |
> --
> 2.20.1

Re: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in secondary process

[Raslan Darawsheh]

Hi,
> -----Original Message-----
> From: Raslan Darawsheh
> Sent: Sunday, August 4, 2019 9:58 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> Yongseok Koh <yskoh@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; sju@paloaltonetworks.com; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in
> secondary process
> 
> Hi Stephen,
> 
> Wrong headline format:
>         net/mlx4: fix crash in dev_info_get in secondary process can you please
> fix it? You shouldn't use the _ in the title format
> 
> Kindest regards,
> Raslan Darawsheh
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > Sent: Friday, July 12, 2019 11:54 PM
> > To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; Yongseok Koh <yskoh@mellanox.com>; Slava
> > Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; sju@paloaltonetworks.com; Stephen Hemminger
> > <stephen@networkplumber.org>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/2] net/mlx4: fix crash in dev_info_get in
> > secondary process
> >
> > mlx4_dev_info_get calls mlx4_get_ifname, but mlx4_get_ifname uses
> > priv->ctx which is not a valid pointer in a secondary process. The fix
> > is to cache the value in primary.
> >
> > In the primary process, get and store the interface index of the
> > device so that secondary process can see it.
> >
> > Bugzilla ID:320
> > Fixes: 61cbdd419478 ("net/mlx4: separate device control functions")
> > Cc: stable@dpdk.org
> > Reported-by: Suyang Ju <sju@paloaltonetworks.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  drivers/net/mlx4/mlx4.c        | 19 +++++++++----------
> >  drivers/net/mlx4/mlx4.h        |  1 +
> >  drivers/net/mlx4/mlx4_ethdev.c |  4 +---
> >  3 files changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index
> > 2e169b0887a7..bab2cadbe519 100644
> > --- a/drivers/net/mlx4/mlx4.c
> > +++ b/drivers/net/mlx4/mlx4.c
> > @@ -763,6 +763,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv,
> > struct rte_pci_device *pci_dev)
> >  	};
> >  	unsigned int vf;
> >  	int i;
> > +	char ifname[IF_NAMESIZE];
> >
> >  	(void)pci_drv;
> >  	err = mlx4_init_once();
> > @@ -1002,17 +1003,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv,
> > struct rte_pci_device *pci_dev)
> >  		     mac.addr_bytes[4], mac.addr_bytes[5]);
> >  		/* Register MAC address. */
> >  		priv->mac[0] = mac;
> > -#ifndef NDEBUG
> > -		{
> > -			char ifname[IF_NAMESIZE];
> > -
> > -			if (mlx4_get_ifname(priv, &ifname) == 0)
> > -				DEBUG("port %u ifname is \"%s\"",
> > -				      priv->port, ifname);
> > -			else
> > -				DEBUG("port %u ifname is unknown", priv-
> > >port);
> > +
> > +		if (mlx4_get_ifname(priv, &ifname) == 0) {
> > +			DEBUG("port %u ifname is \"%s\"",
> > +			      priv->port, ifname);
> > +			priv->if_index = if_nametoindex(ifname);
> > +		} else {
> > +			DEBUG("port %u ifname is unknown", priv->port);
> >  		}
> > -#endif
> > +
> >  		/* Get actual MTU if possible. */
> >  		mlx4_mtu_get(priv, &priv->mtu);
> >  		DEBUG("port %u MTU is %u", priv->port, priv->mtu); diff --git
> > a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> > cd0d637ac2bf..81b529ee8030 100644
> > --- a/drivers/net/mlx4/mlx4.h
> > +++ b/drivers/net/mlx4/mlx4.h
> > @@ -159,6 +159,7 @@ struct mlx4_priv {
> >  	struct ibv_device_attr device_attr; /**< Device properties. */
> >  	struct ibv_pd *pd; /**< Protection Domain. */
> >  	/* Device properties. */
> > +	unsigned int if_index;	/**< Associated network device index */
> >  	uint16_t mtu; /**< Configured MTU. */
> >  	uint8_t port; /**< Physical port number. */
> >  	uint32_t started:1; /**< Device started, flows enabled. */ diff
> > --git a/drivers/net/mlx4/mlx4_ethdev.c
> > b/drivers/net/mlx4/mlx4_ethdev.c index ceef921620a8..5d28c0116d21
> > 100644
> > --- a/drivers/net/mlx4/mlx4_ethdev.c
> > +++ b/drivers/net/mlx4/mlx4_ethdev.c
> > @@ -616,7 +616,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev,
> struct
> > rte_eth_dev_info *info)  {
> >  	struct mlx4_priv *priv = dev->data->dev_private;
> >  	unsigned int max;
> > -	char ifname[IF_NAMESIZE];
> >
> >  	/* FIXME: we should ask the device for these values. */
> >  	info->min_rx_bufsize = 32;
> > @@ -637,8 +636,7 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev,
> struct
> > rte_eth_dev_info *info)
> >  	info->rx_queue_offload_capa = mlx4_get_rx_queue_offloads(priv);
> >  	info->rx_offload_capa = (mlx4_get_rx_port_offloads(priv) |
> >  				 info->rx_queue_offload_capa);
> > -	if (mlx4_get_ifname(priv, &ifname) == 0)
> > -		info->if_index = if_nametoindex(ifname);
> > +	info->if_index = priv->if_index;
> >  	info->hash_key_size = MLX4_RSS_HASH_KEY_SIZE;
> >  	info->speed_capa =
> >  			ETH_LINK_SPEED_1G |
> > --
> > 2.20.1


Patch applied to next-net-mlx after fixing wrong headline format,

Kindest regards,
Raslan Darawsheh

[dpdk-dev] [PATCH 2/2] net/mlx5: fix crash in dev_info_get in secondary process

[Stephen Hemminger]

mlx5_dev_info_get calls mlx5_get_ifname, but mlx5_get_ifname
uses priv->ctx which is not a valid pointer in a secondary
process. The fix is to cache the value in primary.

In the primary process, get and store the interface index of
the device so that secondary process can see it.

Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/mlx5/mlx5.c        | 17 ++++++++---------
 drivers/net/mlx5/mlx5.h        |  1 +
 drivers/net/mlx5/mlx5_ethdev.c |  4 +---
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index d93f92db56b5..27c5ef9b1763 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1105,6 +1105,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	int own_domain_id = 0;
 	uint16_t port_id;
 	unsigned int i;
+	char ifname[IF_NAMESIZE];
 
 	/* Determine if this port representor is supposed to be spawned. */
 	if (switch_info->representor && dpdk_dev->devargs) {
@@ -1479,18 +1480,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		mac.addr_bytes[0], mac.addr_bytes[1],
 		mac.addr_bytes[2], mac.addr_bytes[3],
 		mac.addr_bytes[4], mac.addr_bytes[5]);
-#ifndef NDEBUG
-	{
-		char ifname[IF_NAMESIZE];
 
-		if (mlx5_get_ifname(eth_dev, &ifname) == 0)
-			DRV_LOG(DEBUG, "port %u ifname is \"%s\"",
+	if (mlx5_get_ifname(eth_dev, &ifname) == 0) {
+		priv->if_index = if_nametoindex(ifname);
+		DRV_LOG(DEBUG, "port %u ifname is \"%s\"",
 				eth_dev->data->port_id, ifname);
-		else
-			DRV_LOG(DEBUG, "port %u ifname is unknown",
-				eth_dev->data->port_id);
+	} else {
+		DRV_LOG(DEBUG, "port %u ifname is unknown",
+			eth_dev->data->port_id);
 	}
-#endif
+
 	/* Get actual MTU if possible. */
 	err = mlx5_get_mtu(eth_dev, &priv->mtu);
 	if (err) {
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 5af3f413cdcb..a06ffd444255 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -342,6 +342,7 @@ struct mlx5_priv {
 	uint16_t vlan_filter[MLX5_MAX_VLAN_IDS]; /* VLAN filters table. */
 	unsigned int vlan_filter_n; /* Number of configured VLAN filters. */
 	/* Device properties. */
+	unsigned int if_index; /* Associated kernel network device index. */
 	uint16_t mtu; /* Configured MTU. */
 	unsigned int isolated:1; /* Whether isolated mode is enabled. */
 	unsigned int representor:1; /* Device is a port representor. */
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index eeefe4df3cd4..41e58db5e573 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -605,7 +605,6 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	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;
@@ -626,8 +625,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	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 = priv->if_index;
 	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;
-- 
2.20.1

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: fix crash in dev_info_get in secondary process

[Slava Ovsiienko]

Hi, Stephen

>mlx5_get_ifname uses priv->ctx which is not a valid pointer in a secondary process.

1. Sorry, mlx5_priv structure does not contain ctx field.  Do you mean priv->sh  (shared context) ?
This one is allocated with rte_zmalloc(), that supposes shared memory allocation and shared
context structure should be valid within secondary process context.

It seems there is another issue - mlx5_get_ifname() uses priv->nl_socket_rdma
which is process-specific socket handle (fd), so routine may fail.

2. Generally I'm OK with caching ifindex in priv structure - it seems the ifindex is permanent 
throughout interface lifetime. The mlx5_dev_spawn has a parameter mlx5_dev_spawn_data *spawn,
it contains the successfully queried ifindex of the device being spawned - no need to query again.

The cached valued should be retrieved by mlx5_ifindex() routine and  mlx5_get_ifname()
should be refactored to use mlx5_ifindex().

WBR,
Slava

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 12, 2019 23:54
> To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Yongseok Koh <yskoh@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; sju@paloaltonetworks.com; Stephen Hemminger
> <stephen@networkplumber.org>
> Subject: [PATCH 2/2] net/mlx5: fix crash in dev_info_get in secondary
> process
> 
> mlx5_dev_info_get calls mlx5_get_ifname, but mlx5_get_ifname uses priv-
> >ctx which is not a valid pointer in a secondary process. The fix is to cache
> the value in primary.
> 
> In the primary process, get and store the interface index of the device so that
> secondary process can see it.
> 
> Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/mlx5/mlx5.c        | 17 ++++++++---------
>  drivers/net/mlx5/mlx5.h        |  1 +
>  drivers/net/mlx5/mlx5_ethdev.c |  4 +---
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> d93f92db56b5..27c5ef9b1763 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1105,6 +1105,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	int own_domain_id = 0;
>  	uint16_t port_id;
>  	unsigned int i;
> +	char ifname[IF_NAMESIZE];
> 
>  	/* Determine if this port representor is supposed to be spawned. */
>  	if (switch_info->representor && dpdk_dev->devargs) { @@ -1479,18
> +1480,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  		mac.addr_bytes[0], mac.addr_bytes[1],
>  		mac.addr_bytes[2], mac.addr_bytes[3],
>  		mac.addr_bytes[4], mac.addr_bytes[5]); -#ifndef NDEBUG
> -	{
> -		char ifname[IF_NAMESIZE];
> 
> -		if (mlx5_get_ifname(eth_dev, &ifname) == 0)
> -			DRV_LOG(DEBUG, "port %u ifname is \"%s\"",

> +	if (mlx5_get_ifname(eth_dev, &ifname) == 0) {
> +		priv->if_index = if_nametoindex(ifname);
> +		DRV_LOG(DEBUG, "port %u ifname is \"%s\"",
>  				eth_dev->data->port_id, ifname);
> -		else
> -			DRV_LOG(DEBUG, "port %u ifname is unknown",
> -				eth_dev->data->port_id);
> +	} else {
> +		DRV_LOG(DEBUG, "port %u ifname is unknown",
> +			eth_dev->data->port_id);
>  	}
> -#endif
> +
>  	/* Get actual MTU if possible. */
>  	err = mlx5_get_mtu(eth_dev, &priv->mtu);
>  	if (err) {
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 5af3f413cdcb..a06ffd444255 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -342,6 +342,7 @@ struct mlx5_priv {
>  	uint16_t vlan_filter[MLX5_MAX_VLAN_IDS]; /* VLAN filters table. */
>  	unsigned int vlan_filter_n; /* Number of configured VLAN filters. */
>  	/* Device properties. */
> +	unsigned int if_index; /* Associated kernel network device index. */
>  	uint16_t mtu; /* Configured MTU. */
>  	unsigned int isolated:1; /* Whether isolated mode is enabled. */
>  	unsigned int representor:1; /* Device is a port representor. */ diff --
> git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index eeefe4df3cd4..41e58db5e573 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -605,7 +605,6 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	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;
> @@ -626,8 +625,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	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 = priv->if_index;
>  	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;
> --
> 2.20.1

[dpdk-dev] [PATCH 0/2] net/mlx5: cache the associated network device ifindex

[Viacheslav Ovsiienko]

In mlx5 PMD 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 event 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.

This approach allows the query API routines like mlx5_link_update to be
thread-safe due to Netlink request elimination. mlx5_link_update() may
be called in asynchronous event handler concurrently and it may cause
application hang.

This patch extends and updates the [1]. 

[1] http://patches.dpdk.org/patch/56417/

Proposed-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

Viacheslav Ovsiienko (2):
  net/mlx5: cache the associated network device ifindex
  Revert "net/mlx5: fix master device Netlink socket sharing"

 drivers/net/mlx5/mlx5.c        |  11 ++++
 drivers/net/mlx5/mlx5.h        |   7 +--
 drivers/net/mlx5/mlx5_ethdev.c | 128 ++++-------------------------------------
 3 files changed, 22 insertions(+), 124 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex

[Viacheslav Ovsiienko]

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

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;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex

[Stephen Hemminger]

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?

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex

[Slava Ovsiienko]

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

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex

[Slava Ovsiienko]

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

[dpdk-dev] [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Viacheslav Ovsiienko]

This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
The netlink requests are replaced by ifindex caching and
not needed anymore.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket sharing")
---
 drivers/net/mlx5/mlx5.h        |   6 ---
 drivers/net/mlx5/mlx5_ethdev.c | 109 ++---------------------------------------
 2 files changed, 3 insertions(+), 112 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 4ae6738..b150107 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -518,15 +518,9 @@ struct mlx5_priv {
 /* mlx5_ethdev.c */
 
 int mlx5_get_ifname(const struct rte_eth_dev *dev, char (*ifname)[IF_NAMESIZE]);
-int mlx5_get_ifname_base(const struct rte_eth_dev *base,
-			 const struct rte_eth_dev *dev,
-			 char (*ifname)[IF_NAMESIZE]);
 int mlx5_get_master_ifname(const char *ibdev_path, char (*ifname)[IF_NAMESIZE]);
 unsigned int mlx5_ifindex(const struct rte_eth_dev *dev);
 int mlx5_ifreq(const struct rte_eth_dev *dev, int req, struct ifreq *ifr);
-int mlx5_ifreq_base(const struct rte_eth_dev *base,
-		    const struct rte_eth_dev *dev,
-		    int req, struct ifreq *ifr);
 int mlx5_get_mtu(struct rte_eth_dev *dev, uint16_t *mtu);
 int mlx5_set_flags(struct rte_eth_dev *dev, unsigned int keep,
 		   unsigned int flags);
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 3803be3..e418676 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -240,51 +240,6 @@ struct ethtool_link_settings {
 }
 
 /**
- * Get interface name for the specified device, uses the extra base
- * device resources to perform Netlink requests.
- *
- * This is a port representor-aware version of mlx5_get_master_ifname().
- *
- * @param[in] base
- *   Pointer to Ethernet device to use Netlink socket from
- *   to perfrom requests.
- * @param[in] dev
- *   Pointer to Ethernet device.
- * @param[out] ifname
- *   Interface name output buffer.
- *
- * @return
- *   0 on success, a negative errno value otherwise and rte_errno is set.
- */
-int
-mlx5_get_ifname_base(const struct rte_eth_dev *base,
-		     const struct rte_eth_dev *dev,
-		     char (*ifname)[IF_NAMESIZE])
-{
-	struct mlx5_priv *priv = dev->data->dev_private;
-	struct mlx5_priv *priv_base = base->data->dev_private;
-	unsigned int ifindex;
-
-	assert(priv);
-	assert(priv->sh);
-	assert(priv_base);
-	ifindex = priv_base->nl_socket_rdma >= 0 ?
-		  mlx5_nl_ifindex(priv_base->nl_socket_rdma,
-				  priv->sh->ibdev_name,
-				  priv->ibv_port) : 0;
-	if (!ifindex) {
-		if (!priv->representor)
-			return mlx5_get_master_ifname(priv->sh->ibdev_path,
-						      ifname);
-		rte_errno = ENXIO;
-		return -rte_errno;
-	}
-	if (if_indextoname(ifindex, &(*ifname)[0]))
-		return 0;
-	rte_errno = errno;
-	return -rte_errno;
-}
-/**
  * Get the interface index from device name.
  *
  * @param[in] dev
@@ -346,51 +301,6 @@ struct ethtool_link_settings {
 }
 
 /**
- * Perform ifreq ioctl() on specified Ethernet device,
- * ifindex, name and other attributes are requested
- * on the base device to avoid specified device Netlink
- * socket sharing (this is not thread-safe).
- *
- * @param[in] base
- *   Pointer to Ethernet device to get dev attributes.
- * @param[in] dev
- *   Pointer to Ethernet device to perform ioctl.
- * @param req
- *   Request number to pass to ioctl().
- * @param[out] ifr
- *   Interface request structure output buffer.
- *
- * @return
- *   0 on success, a negative errno value otherwise and rte_errno is set.
- */
-int
-mlx5_ifreq_base(const struct rte_eth_dev *base,
-		const struct rte_eth_dev *dev,
-		int req, struct ifreq *ifr)
-{
-	int sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
-	int ret = 0;
-
-	if (sock == -1) {
-		rte_errno = errno;
-		return -rte_errno;
-	}
-	ret = mlx5_get_ifname_base(base, dev, &ifr->ifr_name);
-	if (ret)
-		goto error;
-	ret = ioctl(sock, req, ifr);
-	if (ret == -1) {
-		rte_errno = errno;
-		goto error;
-	}
-	close(sock);
-	return 0;
-error:
-	close(sock);
-	return -rte_errno;
-}
-
-/**
  * Get device MTU.
  *
  * @param dev
@@ -872,15 +782,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 				ifr = (struct ifreq) {
 					.ifr_data = (void *)&edata,
 				};
-				/*
-				 * Use special version of mlx5_ifreq()
-				 * to get master device name with local
-				 * device Netlink socket. Using master
-				 * device Netlink socket is not thread
-				 * safe.
-				 */
-				ret = mlx5_ifreq_base(dev, master,
-						      SIOCETHTOOL, &ifr);
+				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
 			}
 		}
 		if (ret) {
@@ -977,12 +879,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 				ifr = (struct ifreq) {
 					.ifr_data = (void *)&gcmd,
 				};
-				/*
-				 * Avoid using master Netlink socket.
-				 * This is not thread-safe.
-				 */
-				ret = mlx5_ifreq_base(dev, master,
-						      SIOCETHTOOL, &ifr);
+				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
 			}
 		}
 		if (ret) {
@@ -1003,7 +900,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 
 	*ecmd = gcmd;
 	ifr.ifr_data = (void *)ecmd;
-	ret = mlx5_ifreq_base(dev, master ? master : dev, SIOCETHTOOL, &ifr);
+	ret = mlx5_ifreq(master ? master : dev, SIOCETHTOOL, &ifr);
 	if (ret) {
 		DRV_LOG(DEBUG,
 			"port %u ioctl(SIOCETHTOOL,"
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Stephen Hemminger]

On Fri, 19 Jul 2019 05:31:45 +0000
Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:

> This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
> The netlink requests are replaced by ifindex caching and
> not needed anymore.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket sharing")


Can mlx5 drop dependency o netlink (libmnl)?

Re: [dpdk-dev] [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 19, 2019 19:17
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket
> sharing"
> 
> On Fri, 19 Jul 2019 05:31:45 +0000
> Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
> > The netlink requests are replaced by ifindex caching and not needed
> > anymore.
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket
> > sharing")
> 
> 
> Can mlx5 drop dependency o netlink (libmnl)?

What do you mean?  Dependency on libmnl is already dropped.
The few remaining Netlink requests do not use libmnl (but does libnl).

With best regards, Slava

Re: [dpdk-dev] [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Stephen Hemminger]

On Fri, 19 Jul 2019 16:21:17 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, July 19, 2019 19:17
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>
> > Subject: Re: [PATCH 2/2] Revert "net/mlx5: fix master device Netlink socket
> > sharing"
> > 
> > On Fri, 19 Jul 2019 05:31:45 +0000
> > Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> >   
> > > This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
> > > The netlink requests are replaced by ifindex caching and not needed
> > > anymore.
> > >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket
> > > sharing")  
> > 
> > 
> > Can mlx5 drop dependency o netlink (libmnl)?  
> 
> What do you mean?  Dependency on libmnl is already dropped.
> The few remaining Netlink requests do not use libmnl (but does libnl).
> 
> With best regards, Slava

Good to see, libnl is quite large, it would be good to drop that
(or use libmnl instead).

[dpdk-dev] [PATCH v2 0/2] net/mlx5: cache the associated network device ifindex

[Viacheslav Ovsiienko]

In mlx5 PMD 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 event 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.

This approach allows the query API routines like mlx5_link_update to be
thread-safe due to Netlink request elimination. mlx5_link_update() may
be called in asynchronous event handler concurrently and it may cause
application hang.

This patch extends and updates the [1]. 

[1] http://patches.dpdk.org/patch/56417/

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

---
v2:
  -- comments addressed

v1:
  -- http://patches.dpdk.org/cover/56749/

Viacheslav Ovsiienko (2):
  net/mlx5: cache the associated network device ifindex
  Revert "net/mlx5: fix master device Netlink socket sharing"

 drivers/net/mlx5/mlx5.c        |   7 +++
 drivers/net/mlx5/mlx5.h        |   7 +--
 drivers/net/mlx5/mlx5_ethdev.c | 128 ++++-------------------------------------
 3 files changed, 18 insertions(+), 124 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v2 1/2] net/mlx5: cache the associated network device ifindex

[Viacheslav Ovsiienko]

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>
---
 drivers/net/mlx5/mlx5.c        |  7 +++++++
 drivers/net/mlx5/mlx5.h        |  1 +
 drivers/net/mlx5/mlx5_ethdev.c | 19 +++++++------------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 608daed..2f6254b 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1630,6 +1630,13 @@ 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. So, we may store
+	 * the ifindex here and use the cached value further.
+	 */
+	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 a73375a..1011dcc 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 6c9bcf1..dfd9e97 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;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2 1/2] net/mlx5: cache the associated network device ifindex

[Yongseok Koh]

> On Jul 21, 2019, at 7:56 AM, Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> 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        |  7 +++++++
> drivers/net/mlx5/mlx5.h        |  1 +
> drivers/net/mlx5/mlx5_ethdev.c | 19 +++++++------------
> 3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 608daed..2f6254b 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1630,6 +1630,13 @@ 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. So, we may store
> +	 * the ifindex here and use the cached value further.
> +	 */
> +	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 a73375a..1011dcc 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 6c9bcf1..dfd9e97 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;
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH v2 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Viacheslav Ovsiienko]

This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
The netlink requests are replaced by ifindex caching and
not needed anymore.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket sharing")
---
 drivers/net/mlx5/mlx5.h        |   6 ---
 drivers/net/mlx5/mlx5_ethdev.c | 109 ++---------------------------------------
 2 files changed, 3 insertions(+), 112 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 1011dcc..3e75961 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -518,15 +518,9 @@ struct mlx5_priv {
 /* mlx5_ethdev.c */
 
 int mlx5_get_ifname(const struct rte_eth_dev *dev, char (*ifname)[IF_NAMESIZE]);
-int mlx5_get_ifname_base(const struct rte_eth_dev *base,
-			 const struct rte_eth_dev *dev,
-			 char (*ifname)[IF_NAMESIZE]);
 int mlx5_get_master_ifname(const char *ibdev_path, char (*ifname)[IF_NAMESIZE]);
 unsigned int mlx5_ifindex(const struct rte_eth_dev *dev);
 int mlx5_ifreq(const struct rte_eth_dev *dev, int req, struct ifreq *ifr);
-int mlx5_ifreq_base(const struct rte_eth_dev *base,
-		    const struct rte_eth_dev *dev,
-		    int req, struct ifreq *ifr);
 int mlx5_get_mtu(struct rte_eth_dev *dev, uint16_t *mtu);
 int mlx5_set_flags(struct rte_eth_dev *dev, unsigned int keep,
 		   unsigned int flags);
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index dfd9e97..9629cfb 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -240,51 +240,6 @@ struct ethtool_link_settings {
 }
 
 /**
- * Get interface name for the specified device, uses the extra base
- * device resources to perform Netlink requests.
- *
- * This is a port representor-aware version of mlx5_get_master_ifname().
- *
- * @param[in] base
- *   Pointer to Ethernet device to use Netlink socket from
- *   to perfrom requests.
- * @param[in] dev
- *   Pointer to Ethernet device.
- * @param[out] ifname
- *   Interface name output buffer.
- *
- * @return
- *   0 on success, a negative errno value otherwise and rte_errno is set.
- */
-int
-mlx5_get_ifname_base(const struct rte_eth_dev *base,
-		     const struct rte_eth_dev *dev,
-		     char (*ifname)[IF_NAMESIZE])
-{
-	struct mlx5_priv *priv = dev->data->dev_private;
-	struct mlx5_priv *priv_base = base->data->dev_private;
-	unsigned int ifindex;
-
-	assert(priv);
-	assert(priv->sh);
-	assert(priv_base);
-	ifindex = priv_base->nl_socket_rdma >= 0 ?
-		  mlx5_nl_ifindex(priv_base->nl_socket_rdma,
-				  priv->sh->ibdev_name,
-				  priv->ibv_port) : 0;
-	if (!ifindex) {
-		if (!priv->representor)
-			return mlx5_get_master_ifname(priv->sh->ibdev_path,
-						      ifname);
-		rte_errno = ENXIO;
-		return -rte_errno;
-	}
-	if (if_indextoname(ifindex, &(*ifname)[0]))
-		return 0;
-	rte_errno = errno;
-	return -rte_errno;
-}
-/**
  * Get the interface index from device name.
  *
  * @param[in] dev
@@ -346,51 +301,6 @@ struct ethtool_link_settings {
 }
 
 /**
- * Perform ifreq ioctl() on specified Ethernet device,
- * ifindex, name and other attributes are requested
- * on the base device to avoid specified device Netlink
- * socket sharing (this is not thread-safe).
- *
- * @param[in] base
- *   Pointer to Ethernet device to get dev attributes.
- * @param[in] dev
- *   Pointer to Ethernet device to perform ioctl.
- * @param req
- *   Request number to pass to ioctl().
- * @param[out] ifr
- *   Interface request structure output buffer.
- *
- * @return
- *   0 on success, a negative errno value otherwise and rte_errno is set.
- */
-int
-mlx5_ifreq_base(const struct rte_eth_dev *base,
-		const struct rte_eth_dev *dev,
-		int req, struct ifreq *ifr)
-{
-	int sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
-	int ret = 0;
-
-	if (sock == -1) {
-		rte_errno = errno;
-		return -rte_errno;
-	}
-	ret = mlx5_get_ifname_base(base, dev, &ifr->ifr_name);
-	if (ret)
-		goto error;
-	ret = ioctl(sock, req, ifr);
-	if (ret == -1) {
-		rte_errno = errno;
-		goto error;
-	}
-	close(sock);
-	return 0;
-error:
-	close(sock);
-	return -rte_errno;
-}
-
-/**
  * Get device MTU.
  *
  * @param dev
@@ -872,15 +782,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 				ifr = (struct ifreq) {
 					.ifr_data = (void *)&edata,
 				};
-				/*
-				 * Use special version of mlx5_ifreq()
-				 * to get master device name with local
-				 * device Netlink socket. Using master
-				 * device Netlink socket is not thread
-				 * safe.
-				 */
-				ret = mlx5_ifreq_base(dev, master,
-						      SIOCETHTOOL, &ifr);
+				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
 			}
 		}
 		if (ret) {
@@ -977,12 +879,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 				ifr = (struct ifreq) {
 					.ifr_data = (void *)&gcmd,
 				};
-				/*
-				 * Avoid using master Netlink socket.
-				 * This is not thread-safe.
-				 */
-				ret = mlx5_ifreq_base(dev, master,
-						      SIOCETHTOOL, &ifr);
+				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
 			}
 		}
 		if (ret) {
@@ -1003,7 +900,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 
 	*ecmd = gcmd;
 	ifr.ifr_data = (void *)ecmd;
-	ret = mlx5_ifreq_base(dev, master ? master : dev, SIOCETHTOOL, &ifr);
+	ret = mlx5_ifreq(master ? master : dev, SIOCETHTOOL, &ifr);
 	if (ret) {
 		DRV_LOG(DEBUG,
 			"port %u ioctl(SIOCETHTOOL,"
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2 2/2] Revert "net/mlx5: fix master device Netlink socket sharing"

[Yongseok Koh]

> On Jul 21, 2019, at 7:56 AM, Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> This reverts commit e28111ac9864af09e826241a915dfff87a9c00ad.
> The netlink requests are replaced by ifindex caching and
> not needed anymore.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Fixes: e28111ac9864 ("net/mlx5: fix master device Netlink socket sharing")
> ---

Acked-by: Yongseok Koh <yskoh@mellanox.com>

> drivers/net/mlx5/mlx5.h        |   6 ---
> drivers/net/mlx5/mlx5_ethdev.c | 109 ++---------------------------------------
> 2 files changed, 3 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 1011dcc..3e75961 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -518,15 +518,9 @@ struct mlx5_priv {
> /* mlx5_ethdev.c */
> 
> int mlx5_get_ifname(const struct rte_eth_dev *dev, char (*ifname)[IF_NAMESIZE]);
> -int mlx5_get_ifname_base(const struct rte_eth_dev *base,
> -			 const struct rte_eth_dev *dev,
> -			 char (*ifname)[IF_NAMESIZE]);
> int mlx5_get_master_ifname(const char *ibdev_path, char (*ifname)[IF_NAMESIZE]);
> unsigned int mlx5_ifindex(const struct rte_eth_dev *dev);
> int mlx5_ifreq(const struct rte_eth_dev *dev, int req, struct ifreq *ifr);
> -int mlx5_ifreq_base(const struct rte_eth_dev *base,
> -		    const struct rte_eth_dev *dev,
> -		    int req, struct ifreq *ifr);
> int mlx5_get_mtu(struct rte_eth_dev *dev, uint16_t *mtu);
> int mlx5_set_flags(struct rte_eth_dev *dev, unsigned int keep,
> 		   unsigned int flags);
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index dfd9e97..9629cfb 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -240,51 +240,6 @@ struct ethtool_link_settings {
> }
> 
> /**
> - * Get interface name for the specified device, uses the extra base
> - * device resources to perform Netlink requests.
> - *
> - * This is a port representor-aware version of mlx5_get_master_ifname().
> - *
> - * @param[in] base
> - *   Pointer to Ethernet device to use Netlink socket from
> - *   to perfrom requests.
> - * @param[in] dev
> - *   Pointer to Ethernet device.
> - * @param[out] ifname
> - *   Interface name output buffer.
> - *
> - * @return
> - *   0 on success, a negative errno value otherwise and rte_errno is set.
> - */
> -int
> -mlx5_get_ifname_base(const struct rte_eth_dev *base,
> -		     const struct rte_eth_dev *dev,
> -		     char (*ifname)[IF_NAMESIZE])
> -{
> -	struct mlx5_priv *priv = dev->data->dev_private;
> -	struct mlx5_priv *priv_base = base->data->dev_private;
> -	unsigned int ifindex;
> -
> -	assert(priv);
> -	assert(priv->sh);
> -	assert(priv_base);
> -	ifindex = priv_base->nl_socket_rdma >= 0 ?
> -		  mlx5_nl_ifindex(priv_base->nl_socket_rdma,
> -				  priv->sh->ibdev_name,
> -				  priv->ibv_port) : 0;
> -	if (!ifindex) {
> -		if (!priv->representor)
> -			return mlx5_get_master_ifname(priv->sh->ibdev_path,
> -						      ifname);
> -		rte_errno = ENXIO;
> -		return -rte_errno;
> -	}
> -	if (if_indextoname(ifindex, &(*ifname)[0]))
> -		return 0;
> -	rte_errno = errno;
> -	return -rte_errno;
> -}
> -/**
>  * Get the interface index from device name.
>  *
>  * @param[in] dev
> @@ -346,51 +301,6 @@ struct ethtool_link_settings {
> }
> 
> /**
> - * Perform ifreq ioctl() on specified Ethernet device,
> - * ifindex, name and other attributes are requested
> - * on the base device to avoid specified device Netlink
> - * socket sharing (this is not thread-safe).
> - *
> - * @param[in] base
> - *   Pointer to Ethernet device to get dev attributes.
> - * @param[in] dev
> - *   Pointer to Ethernet device to perform ioctl.
> - * @param req
> - *   Request number to pass to ioctl().
> - * @param[out] ifr
> - *   Interface request structure output buffer.
> - *
> - * @return
> - *   0 on success, a negative errno value otherwise and rte_errno is set.
> - */
> -int
> -mlx5_ifreq_base(const struct rte_eth_dev *base,
> -		const struct rte_eth_dev *dev,
> -		int req, struct ifreq *ifr)
> -{
> -	int sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
> -	int ret = 0;
> -
> -	if (sock == -1) {
> -		rte_errno = errno;
> -		return -rte_errno;
> -	}
> -	ret = mlx5_get_ifname_base(base, dev, &ifr->ifr_name);
> -	if (ret)
> -		goto error;
> -	ret = ioctl(sock, req, ifr);
> -	if (ret == -1) {
> -		rte_errno = errno;
> -		goto error;
> -	}
> -	close(sock);
> -	return 0;
> -error:
> -	close(sock);
> -	return -rte_errno;
> -}
> -
> -/**
>  * Get device MTU.
>  *
>  * @param dev
> @@ -872,15 +782,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
> 				ifr = (struct ifreq) {
> 					.ifr_data = (void *)&edata,
> 				};
> -				/*
> -				 * Use special version of mlx5_ifreq()
> -				 * to get master device name with local
> -				 * device Netlink socket. Using master
> -				 * device Netlink socket is not thread
> -				 * safe.
> -				 */
> -				ret = mlx5_ifreq_base(dev, master,
> -						      SIOCETHTOOL, &ifr);
> +				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
> 			}
> 		}
> 		if (ret) {
> @@ -977,12 +879,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
> 				ifr = (struct ifreq) {
> 					.ifr_data = (void *)&gcmd,
> 				};
> -				/*
> -				 * Avoid using master Netlink socket.
> -				 * This is not thread-safe.
> -				 */
> -				ret = mlx5_ifreq_base(dev, master,
> -						      SIOCETHTOOL, &ifr);
> +				ret = mlx5_ifreq(master, SIOCETHTOOL, &ifr);
> 			}
> 		}
> 		if (ret) {
> @@ -1003,7 +900,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
> 
> 	*ecmd = gcmd;
> 	ifr.ifr_data = (void *)ecmd;
> -	ret = mlx5_ifreq_base(dev, master ? master : dev, SIOCETHTOOL, &ifr);
> +	ret = mlx5_ifreq(master ? master : dev, SIOCETHTOOL, &ifr);
> 	if (ret) {
> 		DRV_LOG(DEBUG,
> 			"port %u ioctl(SIOCETHTOOL,"
> -- 
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH v2 0/2] net/mlx5: cache the associated network device ifindex

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Viacheslav Ovsiienko
> Sent: Sunday, July 21, 2019 5:57 PM
> To: dev@dpdk.org
> Cc: Yongseok Koh <yskoh@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; stephen@networkplumber.org
> Subject: [dpdk-dev] [PATCH v2 0/2] net/mlx5: cache the associated network
> device ifindex
> 
> In mlx5 PMD 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 event 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.
> 
> This approach allows the query API routines like mlx5_link_update to be
> thread-safe due to Netlink request elimination. mlx5_link_update() may
> be called in asynchronous event handler concurrently and it may cause
> application hang.
> 
> This patch extends and updates the [1].
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F56417%2F&amp;data=02%7C01%7Crasland%40mell
> anox.com%7C3c155fb435384529783608d70debb168%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C636993178256642102&amp;sdata=GNj8%2B79
> %2FoBQzu7esmy4wN4e6WC7RHREDbV4OZTq6SOA%3D&amp;reserved=0
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---

Series applied to next-net-mlx

Kindest regards,
Raslan Darawsheh

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: fix crash in dev_info_get in secondary process

[Raslan Darawsheh]

Hi Stephen,

Can you please confirm that Slava's patch fixed your issue and this patch is not needed anymore for MLX5?
So that I can take mlx4 patch only from this series?

Kindest regards,
Raslan Darawsheh

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Friday, July 12, 2019 11:54 PM
> To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Yongseok Koh <yskoh@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; sju@paloaltonetworks.com; Stephen Hemminger
> <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH 2/2] net/mlx5: fix crash in dev_info_get in
> secondary process
> 
> mlx5_dev_info_get calls mlx5_get_ifname, but mlx5_get_ifname
> uses priv->ctx which is not a valid pointer in a secondary
> process. The fix is to cache the value in primary.
> 
> In the primary process, get and store the interface index of
> the device so that secondary process can see it.
> 
> Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/mlx5/mlx5.c        | 17 ++++++++---------
>  drivers/net/mlx5/mlx5.h        |  1 +
>  drivers/net/mlx5/mlx5_ethdev.c |  4 +---
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index d93f92db56b5..27c5ef9b1763 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1105,6 +1105,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	int own_domain_id = 0;
>  	uint16_t port_id;
>  	unsigned int i;
> +	char ifname[IF_NAMESIZE];
> 
>  	/* Determine if this port representor is supposed to be spawned. */
>  	if (switch_info->representor && dpdk_dev->devargs) {
> @@ -1479,18 +1480,16 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  		mac.addr_bytes[0], mac.addr_bytes[1],
>  		mac.addr_bytes[2], mac.addr_bytes[3],
>  		mac.addr_bytes[4], mac.addr_bytes[5]);
> -#ifndef NDEBUG
> -	{
> -		char ifname[IF_NAMESIZE];
> 
> -		if (mlx5_get_ifname(eth_dev, &ifname) == 0)
> -			DRV_LOG(DEBUG, "port %u ifname is \"%s\"",
> +	if (mlx5_get_ifname(eth_dev, &ifname) == 0) {
> +		priv->if_index = if_nametoindex(ifname);
> +		DRV_LOG(DEBUG, "port %u ifname is \"%s\"",
>  				eth_dev->data->port_id, ifname);
> -		else
> -			DRV_LOG(DEBUG, "port %u ifname is unknown",
> -				eth_dev->data->port_id);
> +	} else {
> +		DRV_LOG(DEBUG, "port %u ifname is unknown",
> +			eth_dev->data->port_id);
>  	}
> -#endif
> +
>  	/* Get actual MTU if possible. */
>  	err = mlx5_get_mtu(eth_dev, &priv->mtu);
>  	if (err) {
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 5af3f413cdcb..a06ffd444255 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -342,6 +342,7 @@ struct mlx5_priv {
>  	uint16_t vlan_filter[MLX5_MAX_VLAN_IDS]; /* VLAN filters table. */
>  	unsigned int vlan_filter_n; /* Number of configured VLAN filters. */
>  	/* Device properties. */
> +	unsigned int if_index; /* Associated kernel network device index. */
>  	uint16_t mtu; /* Configured MTU. */
>  	unsigned int isolated:1; /* Whether isolated mode is enabled. */
>  	unsigned int representor:1; /* Device is a port representor. */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c
> index eeefe4df3cd4..41e58db5e573 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -605,7 +605,6 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	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;
> @@ -626,8 +625,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	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 = priv->if_index;
>  	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;
> --
> 2.20.1

Re: [dpdk-dev] [PATCH 2/2] net/mlx5: fix crash in dev_info_get in secondary process

[Stephen Hemminger]

On Wed, 31 Jul 2019 07:36:26 +0000
Raslan Darawsheh <rasland@mellanox.com> wrote:

> Hi Stephen,
> 
> Can you please confirm that Slava's patch fixed your issue and this patch is not needed anymore for MLX5?
> So that I can take mlx4 patch only from this series?
> 
> Kindest regards,
> Raslan Darawsheh

OK, but my other concern is that mlx5 patch will not backport cleanly to stable.