Page MenuHomeFreeBSD

if_move: create new ifp instances on each vmove
Needs ReviewPublic

Authored by melifaro on Sep 21 2022, 6:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 6:26 PM
Unknown Object (File)
Sun, Dec 1, 7:02 PM
Unknown Object (File)
Nov 25 2024, 12:26 AM
Unknown Object (File)
Nov 24 2024, 7:39 AM
Unknown Object (File)
Nov 21 2024, 3:32 PM
Unknown Object (File)
Nov 17 2024, 10:34 PM
Unknown Object (File)
Nov 10 2024, 9:04 AM
Unknown Object (File)
Nov 8 2024, 3:13 PM
Subscribers

Details

Reviewers
bz
kp
glebius
Group Reviewers
network
Summary

VNET has brought a huge advantage of easily creating independent networks stacks on a single machine. One of the pillars is the ability to assign certain interface to the particular VNET (and bring it back). This is useful when adding external-connectivity interfaces relying on physical interfaces (plans, bridges) or simply physical interfaces themselves

Internally this move (dubbed vmove) is implemented by shutting the interface down, removing all addresses, multicast groups, closing attached sockets, etc, and actually reassigning if_vnet the the new value.
Unfortunately, this approach leads to the abstraction leak - every network-related subsystem have to account for the random VNET change of an ifp.
Given the fact that decent number of variables are VNET-local, it manifests added complexity. For example, all traffic-queuing parts of the kernel have to handle this VNET changes. Nexthops logic have to account for that as well. It is not always easy to actually address. IMO the existing approach is too fragile and provokes hard-to-find “bugs” the neighbouring subsystems when migrating interfaces between VNETs.

This review proposes an alternative approach to the problem for the cloned interfaces. The base idea is to leverage existing cloner interfaces and create a new ifp in the target VNET, copying some properties from the original ifp and then destroying it. To be more specific, cloner framework adds a new vmove_f method that allows “complex” interfaces such as vlan to copy interface-specific properties such as tag or parent interface. It also provide helpers (ifc_save_vparams(), ifc_apply_vparams()) to copy basic properties like mtu or interface description. Not all interfaces need to implement vmove_f callback - many “simple” ones like lo can leverage default if_vmove_simple(), which creates an interface with the same name in the target VNET and destroys original interface upon success.
Note that this approach allows gradual migration - interfaces that are not converted yet can explicitly mark their cloners with IFC_F_NOMOVE, which will trigger fallback to the existing if_vmove() logic.

The implementation provides an implementation of vmove_f handler for if_vlan and if_epair. Some works on the implementation:

  • if_vlan does interface destruction first to propagate eventhandler event to the driver, so it can update the ifp afterwards
  • if_epair is more complex. oifp pointer is now not static, so some tweaks are required to safely access sc->oifp
Test Plan

All sys/net, sys/netinet, sys/netinet6 tests passes.

10:13 [0] m@devel0 s kyua test -k /usr/tests/sys/net/Kyuafile
...
routing/test_rtsock_l3:rtm_add_v4_temporal1_success  ->  failed: /usr/home/melifaro/free/head/tests/sys/net/routing/test_rtsock_l3.c:181: expected RTM_DELETE message, got 14 (RTM_IFINFO)  [0.024s]
routing/test_rtsock_l3:rtm_add_v6_temporal1_success  ->  failed: /usr/home/melifaro/free/head/tests/sys/net/routing/test_rtsock_l3.c:181: expected RTM_DELETE message, got 14 (RTM_IFINFO)  [0.025s]
if_lagg_test:witness  ->  failed: Lock-order reversals involving if_lagg.c detected  [0.038s]
134/137 passed (3 failed)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 47538
Build 44425: arc lint + arc unit

Event Timeline

melifaro edited the test plan for this revision. (Show Details)
melifaro added reviewers: bz, kp, glebius, network.
melifaro edited the summary of this revision. (Show Details)

That's an interesting idea, but I'm not clear on what the long-term plan is.
This will work (and might indeed be an improvement) for cloned interfaces, but we'd still have to maintain the previous way of handling things for "real" (e.g. igb0) interfaces as they're moved between vnets.

I worry that we'll end up doing things one way for cloned interfaces, and another for other interfaces, which gives us twice the opportunity for bugs, and half as much testing.

In D36651#833505, @kp wrote:

That's an interesting idea, but I'm not clear on what the long-term plan is.
This will work (and might indeed be an improvement) for cloned interfaces, but we'd still have to maintain the previous way of handling things for "real" (e.g. igb0) interfaces as they're moved between vnets.

I worry that we'll end up doing things one way for cloned interfaces, and another for other interfaces, which gives us twice the opportunity for bugs, and half as much testing.

Thank you for the feedback!
Indeed, I should have included the proposal for the physical interfaces.
The primary goal of this change in general is to replace the current mechanism. I'm currently thinking of the following:

  • Slightly re-purpose if_reassign() to perform the same function as vmove_f handler (potentially even use if_reassign() directly even for cloned interfaces)
  • Write the handlers for the "relevant" (e.g. modern, likely to be used in routers/cloud setups) interfaces such as vnet/iflib/mellanox/chelsio
  • Add a deprecation warning with a wiki link, specifying how to report it, to the current cloning method
  • Wait for a couple of months and remove the current cloning method
sys/net/if.c
1372

Shouldn't we return from if_vmove_loan() at this point if ifc_vmove() succeeded?

ifc_vmove() (when returning 0) should have already removed the interface from the current vnet and moved it into pr->pr_vnet, so if we don't return now we're going to if_unlink_ifnet(), which is not going to find the ifp (because we just moved it), and we'll return ENODEV rather than 0.

1445

Same question as for if_vmove_loan().

In D36651#833505, @kp wrote:

That's an interesting idea, but I'm not clear on what the long-term plan is.
This will work (and might indeed be an improvement) for cloned interfaces, but we'd still have to maintain the previous way of handling things for "real" (e.g. igb0) interfaces as they're moved between vnets.

I worry that we'll end up doing things one way for cloned interfaces, and another for other interfaces, which gives us twice the opportunity for bugs, and half as much testing.

Thank you for the feedback!
Indeed, I should have included the proposal for the physical interfaces.
The primary goal of this change in general is to replace the current mechanism. I'm currently thinking of the following:

  • Slightly re-purpose if_reassign() to perform the same function as vmove_f handler (potentially even use if_reassign() directly even for cloned interfaces)
  • Write the handlers for the "relevant" (e.g. modern, likely to be used in routers/cloud setups) interfaces such as vnet/iflib/mellanox/chelsio
  • Add a deprecation warning with a wiki link, specifying how to report it, to the current cloning method
  • Wait for a couple of months and remove the current cloning method

That makes sense.