ifindex: provide global immutable complementary to per-vnet index

[glebius]

Summary

The global index would allow to search ifnet by index outside of
vnet context (e.g. netisr) and will persist across if_vmove().

[bz]

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.

[glebius]

In D33265#752112, @bz wrote:

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.

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.

[glebius]

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

[bz]

So let me ask; once you are done, does this mean ifconfig foo0 destroy will block until all mbufs serialized to the immutable index belonging to the foo0 ifp will be freed?

[glebius]

In D33265#752160, @bz wrote:

So let me ask; once you are done, does this mean ifconfig foo0 destroy will block until all mbufs serialized to the immutable index belonging to the foo0 ifp will be freed?

Nope. ifconfig foo0 destroy will proceed exactly as it does now. Later, deserialization will fail to find ifnet and will free mbuf.

[kp]

In D33265#752123, @glebius wrote:

But what about interface's internal queue, like epair's buf ring?

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.

sys/net/if.c

322

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.

[glebius]

sys/net/if.c

322

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?

[kp]

sys/net/if.c

322

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).

[glebius]

sys/net/if.c

322

Yes, this is my plan. ifnet_byindex() will behave differently called from virtual and non-virtual context (remember netisr!). If there is vnet context, it will do the check.

[bz]

sys/net/if.c

322

I would prefer a few days to think about this before we end up with 7 different versions trying things out.

Making the ifindex global was something which was argued against when VIMAGE was iniatally done. @brooks and @rwatson were part of that.

[glebius]

sys/net/if.c

322

Making the ifindex global was something which was argued against when VIMAGE was iniatally done. @brooks and @rwatson were part of that.

I would guess that was because V_if_index was accessed all over the place. Now it is private, so all logic is in one place.

[glebius]

I'd really be glad to get some input from @brooks and/or @rwatson. Or some additional thoughts from @bz, @kp, @ae, @melifaro and general network. Meanwhile I'm starting to prototype version where if_index table will be global and ifnet_byindex() will be VIMAGE aware.

[glebius]

Use https://reviews.freebsd.org/D33672 instead. I will try to rebase children on top of D33672, if phabricator allows that.