Re: git: 91f44749c6fe - main - ifnet: make if_index global

  Marko,

On Sat, Jan 29, 2022 at 04:20:34PM +0100, Marko Zec wrote:
M> >     ifnet: make if_index global
M> >     
M> >     Now that ifindex is static to if.c we can unvirtualize it.  For
M> > lifetime of an ifnet its index never changes.  To avoid leaking
M> > foreign interfaces the net.link.generic.system.ifcount sysctl and the
M> > ifnet_byindex() KPI filter their returned value on curvnet.  Since
M> > if_vmove() no longer changes the if_index, inline ifindex_alloc() and
M> > ifindex_free() into if_alloc() and if_free() respectively.
M> >     
M> >     API wise the only change is that now minimum interface index can
M> > be greater than 1.
M> 
M> IMO this is a huge change in an API which has been stable like forever.
M> Was this tested with a broader spectrum of userland consumers, such as
M> FRR & alike?

What exactly is FRR? If we see there is an expectation from some software
that index shall start at 1, I will rethink this change. So far there is
no such evidence.

M> > The holes in interface indexes were always allowed.
M> 
M> True, the holes were formally allowed but were never huge, and now they
M> will be guaranteed with #vnets > 1.  Again, that's not something 3rd
M> party tools would expect.

Well "huge" or "small" doesn't make difference for algorithms. Holes are
allowed.

M> What could be the repercussions of ifindex globalization to 16-bit size
M> of ifru_index in struct ifreq?
M> What can prevent a vnet to inflate the global ifindex space, causing
M> the linear scan in sysctl_ifcount() to become a global problem?

We don't think that virtualization of if_index was there to save if_index
space, was it? I think the reason was just cause it was easier.

M> Why do we now need to check wheter curvnet != NULL in ifnet_byindex(),
M> i.e. are there callers to ifnet_byindex() where curvnet is not set?  If
M> so, that smells like a bug in the caller?

Good point. Maybe an assertion should be put there.

M> Or, is it the other way around, that ifnet_byindex() is now intended to
M> become a silent workaround for callers which lack curvnet context?  If
M> that is so, it is indeed becoming a major VNET KPI feature (though I'd
M> call it a bug), and as such, it should have been clearly stated in both
M> the commit message, and as a comment in the ifnet_byindex() code.
M> 
M> Finally, the commit message does not reveal any clues what this change
M> actually tries to accomplish.  The claim "Now that ifindex is static to
M> if.c we can unvirtualize it." tell us nothing about the true intent.

Well, the intent is revealed in the following commits, that use index
as the only true stable & unique identifier of an interface except its
pointer.

I have had a previous version, that was maintaining two indexes a virtual
and non-virtual:

https://reviews.freebsd.org/D33265

If you ask me of a perfect version of this design, I would say same thing
I already said in other topics: if_vmove shall not exist. If if_vmove
didn't exist, a virtual index would provide stable and unique identifier
for an interface. If interfaces were tied to their IFNET for their lifetime,
so less problems would exist. I already lost count of problems created by
if_vmove :(

M> Sorry for chiming in late, but my sentiment against this change is
M> obvious...

I'm also sorry for not including you in the review. Please add yourself
to https://reviews.freebsd.org/project/members/24/

-- 
Gleb Smirnoff