[dpdk-dev] [PATCH 1/2] net/mlx5: cache the associated network device ifindex
Stephen Hemminger
stephen at networkplumber.org
Fri Jul 19 20:03:17 CEST 2019
On Fri, 19 Jul 2019 16:41:38 +0000
Slava Ovsiienko <viacheslavo at mellanox.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen at networkplumber.org>
> > Sent: Friday, July 19, 2019 19:16
> > To: Slava Ovsiienko <viacheslavo at mellanox.com>
> > Cc: dev at dpdk.org; Yongseok Koh <yskoh at mellanox.com>; Shahaf Shuler
> > <shahafs at 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 at 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 at 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.
More information about the dev
mailing list