Page MenuHomeFreeBSD

VNETify dummynet
ClosedPublic

Authored by kp on Mar 15 2021, 8:10 PM.

Details

Summary

First attempt at VNETing dummynet.

This moves dn_cfg and other parameters into per VNET variables.

The taskqueue and control state remains global. This is missing any
locking around the global state and needs much more testing to ensure
that delay, and bandwidth limits work in vnets.

Test Plan

I have a basic set of tests for VNET that should be integrated into the FreeBSD
test suite that ensure basic functionality.

I want to write tests that examine the scalability across many VNETs, but VNET
removal is quite slow right now.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

thj requested review of this revision.Mar 15 2021, 8:10 PM

I will merge https://reviews.freebsd.org/D29246 but I wanted to post the diff so kp@ and I can coordinate

Andrey, I didn't get your reply when I asked about conflicts, but kp has forwarded it.
If this generates conflicts I am happy to work around your work in progress,
VNETed dummynet is what I want.

I think when my work will be ready, I will commit it as separate implementation. Thus you can commit your changes to go forward with your work.

sys/netpfil/ipfw/dn_aqm.h
137

Sidenote: sooner or later we'll want to turn these into counter_u64's.

sys/netpfil/ipfw/ip_dn_io.c
661

Is it worth asserting that it's empty at this point? (I.e. before we memset(0) it?

678

Shouldn't tick_last be V_tick_last?

(I think I've moved it into dn_parms in a different proposed patch, so it's probably academic.)

731–732

I think we only want to do this once, not once per vnet.

Possibly we'll have to find the minimal timeout though.

sys/netpfil/ipfw/ip_dummynet.c
2624

We need to be careful here. I'd have to go dig around to refresh my memory to be sure, but my recollection is that the order of operations on unload is counter-intuitive.

I believe there will be ip_dn_vnet_destroy() calls after this unload.

I'll post a slightly updated version soon.

The locking Tom cites does still need to be fixed, but I think it's mostly okay. The only missing part is the protection of schedlist and aqmlist, and those will probably be better served by NET_EPOCH than by a separate lock. (Because taking the lock in find_sched_type() doesn't protect the struct dn_alg we return from being removed after we release the lock. Being inside NET_EPOCH and waiting to complete the unload until after NET_EPOCH_WAIT should be fine.)

I'll try to post a patch for that later today.

sys/netpfil/ipfw/ip_dn_private.h
156–157

This needs to be moved out of dn_parms, just like schedlist. It's a global list of algorithms and only added to/removed from in the default vnet context (when modules are loaded/unloaded).

In D29274#682376, @kp wrote:

The locking Tom cites does still need to be fixed, but I think it's mostly okay. The only missing part is the protection of schedlist and aqmlist, and those will probably be better served by NET_EPOCH than by a separate lock. (Because taking the lock in find_sched_type() doesn't protect the struct dn_alg we return from being removed after we release the lock. Being inside NET_EPOCH and waiting to complete the unload until after NET_EPOCH_WAIT should be fine.)

I'll try to post a patch for that later today.

Actually, the submodules related code doesn't used, because all modules are compiled in the single module and there is no way to unload some single module. And there are several another problems with unloading if you want to try. :)

kp edited reviewers, added: thj; removed: kp.
In D29274#682381, @ae wrote:

Actually, the submodules related code doesn't used, because all modules are compiled in the single module and there is no way to unload some single module. And there are several another problems with unloading if you want to try. :)

That's a good point and I should have spotted that. I'm going to post that patch anyway, on the grounds that it'll be less incorrect that what we have right now.

We cannot sleep while in NET_EPOCH, so change the dummynet_task from NET_TASK
to regular TASK and enter NET_EPOCH after we've taken the VNET_LIST_RLOCK.

I'd like to commit this (and D30381) soon. It may not be perfect, but it means we can start adding test cases. Dummynet simply doesn't work with vnet prior to this patch, so even imperfect support (and so far I've not broken this) is better than the current state.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 3 2021, 3:20 PM
Closed by commit rGfe3bcfbda30e: VNETify dummynet (authored by thj, committed by kp). · Explain Why
This revision was automatically updated to reflect the committed changes.