[1/2] net/mlx5: cache the associated network device ifindex
Checks
Commit Message
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
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?
> -----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
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.
> -----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
@@ -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;
@@ -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. */
@@ -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;