On Fri, 19 Jul 2019 16:41:38 +0000 Slava Ovsiienko <viachesl...@mellanox.com> wrote:
> > -----Original Message----- > > From: Stephen Hemminger <step...@networkplumber.org> > > Sent: Friday, July 19, 2019 19:16 > > To: Slava Ovsiienko <viachesl...@mellanox.com> > > Cc: dev@dpdk.org; Yongseok Koh <ys...@mellanox.com>; Shahaf Shuler > > <shah...@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 <viachesl...@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 <step...@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.
