The global index would allow to search ifnet by index outside of
vnet context (e.g. netisr) and will persist across if_vmove().
Okay, I don't get the problem that is supposed to be solved with this. Also I don't like the naming at all as it gets very confusing.
I was one of the people who initially voted for an immutable ifindex but let me try to put the current thinking into words:
The ifIndex is an ID for an interface (you may think ifnet for the moment).
The moment an ifnet appears on a system it gets an ifIndex assigned.
If the interface is removed the ifIndex is freed.
Let me give you two examples why an ifIndex is NOT tied to an ifnet when it moves VNETs for an example that does not involve vnets:
You plug a USB ethernet in, you get an ifnet and you get an ifindex.
You unplug the USB dongle and both get freed.
At a later time you plug it back in; the ifIndex will very likely have changed.
The same goes if you 10 times create and destroy a cloned interface.
If you move an interface (ifnet) between vnets it vanishes from one system and appears in another, very much like you unplug your USB dongle from one laptop and plug it into another.
An ifIndex is not a hardware ID.
When you move an ifnet between a vnet everything should happen as if you'd unplug a physical card from a system and plug it back in into another (apart from that we do not create a fully new ifnet; consider that a shortcut and I'd say related to how cloned interfaces were made to work sadly).
The only other reason is that we do keep track of what to do with an ifnet in case the vnet goes away as a physical interface should re-appear somewhere again and not be lost. Now we could do a full detach and reattach on a bus for that but that works only in one direction as in the other direction it would never show up in a VNET but always the base system. So it is basically a "feature" of hierarchical jails which enforces us to keep the ifnet alive.
Maybe the interfaces should have been defined more clearly 10-15 years ago such as we do with other stuff these days (partial copies of structs to an offset). Maybe we should do these things in 14 and clean this up properly before more misconceptions are created plastering more over actual problems caused by misunderstandings and not fully supporting the way things are supposed to work.
If you look at the stack of reviews, you'll see an API to serialize/restore m_pkthdr.rcvif. Originally this branch didn't have this commit, and we used V_index to serialize, and it worked as intended overall, until VIMAGE comes into the play. First, there are cases where we want to resolve index to ifp and we don't have VNET. The most important one would be netisr, see D33268. Second, what is correct action on restoring ifnet pointers after if_vmove? Most probably packets should be dropped. But what about interface's internal queue, like epair's buf ring?
I'm not loving this revision, too. I'm open to suggestions.
Other option that I considered, but didn't yet coded, is :
- make the index table global
- provide function if_index() that would calculate ifnet's position among all ifnets of the particular vnet and use it instead of just reading ifp->if_index
- ifnet_byindex() also to calculate position among all ifnets that belong to current vnet
We can teach if_epair to purge its buf ring when it's being moved. That's easy and sane.
First, there are cases where we want to resolve index to ifp and we don't have VNET. The most important one would be netisr, see D33268.
That's harder, and I'm not sure what the answer is either.
Would it make sense to store the vnet pointer in the mbuf? Leaving aside that I don't know if we've got space for that, but I do believe mbufs only ever change vnet when sent through if_epair.
Is that right? If we allocated idx 1, 2 and 3 and then free the ifnet->if_index 2 one we'll have a hole.
You are right. So, although ifindex behavior guarantees sequential allocation, it doesn't guarantee absence of holes at later runtime.
What if we just make ifindex global and see is there any obscure software in ports that would get a problem due to non-sequential allocation? Just like Bjoern I don't like my patch. @bz @melifaro @ae your opinion?
Given that we can already have holes user space should be able to cope with that. I'm not going to promise there's no stupid software out there, but it shouldn't be much more broken that it already is if we make this change.
Making the ifindex global may work, but we'll have to be careful everywhere we use it to not return struct ifnets from the wrong vnet. I suppose that if we keep the existing functions and make those check that ifp->if_vnet == curvnet prior to returning them we should be fine, and can then add a new lookup function for use in netisr functions (which doesn't check curvnet).