> -----Original Message----- > From: Stephen Hemminger <step...@networkplumber.org> > Sent: Friday, July 19, 2019 21:03 > 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 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. 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
