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

[Stephen Hemminger <stephen@networkplumber.org>]

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.