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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

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