Page MenuHomeFreeBSD

uma: multipage chicken switch
ClosedPublic

Authored by rlibby on Feb 3 2020, 5:59 PM.

Details

Test Plan

OK set vm.multipage_slabs=0

Diff Detail

Repository
rS 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

rlibby created this revision.Feb 3 2020, 5:59 PM
markj added inline comments.Feb 3 2020, 6:02 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

Do you actually need the TUNABLE_INT()? I thought that was for fetching tunables inline. CTLFLAG_RDTUN ought to be sufficient.

I'd suggest putting this under debug, assuming your intent is that this only be used for debugging regressions.

rlibby added inline comments.Feb 3 2020, 6:22 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

My understanding is that the sysctl method for tunables doesn't work until SI_SUB_KMEM, which is after SI_SUB_VM where we do uma_startup1 with the startup zones. (I don't think there's a particularly good reason for this, it seems in principle like it could be fixed.)

Yes, I expect for debugging regression. Okay, "debug.uma_multipage_slabs"?

markj accepted this revision.Feb 3 2020, 6:31 PM
markj added inline comments.
sys/vm/uma_core.c
365 ↗(On Diff #67726)

I see, I think you're right. I also can't see a reason it couldn't be done earlier.

Maybe "debug.vm.uma_multipage_slabs", though the debug OID is already kind of a mess. Either is fine with me.

This revision is now accepted and ready to land.Feb 3 2020, 6:31 PM
rlibby added inline comments.Feb 3 2020, 6:37 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

Adding a debug.vm node sounds good to me. I can move the sysctls in D20714 under there too.

rlibby added inline comments.Feb 3 2020, 10:32 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

Ah, I see we have vm.debug (not debug.vm) already, but under INVARIANTS. I can raise that out of invariants, and move it to uma_dbg.c. Do you think debug.vm is the better location in the sysctl tree?

markj added inline comments.Feb 3 2020, 11:13 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

I think debug.vm or even debug.vm.uma is slightly more consistent with most other subsystems. I also don't like that the sysctls under vm.debug are UMA-specific but don't contain UMA in the name. That said, I don't want to bikeshed, so please feel free to put them wherever you think it makes sense. I just think having "debug" somewhere in the name would be a good idea.

rlibby added a subscriber: glebius.Feb 4 2020, 8:25 AM
rlibby added inline comments.
sys/vm/uma_core.c
365 ↗(On Diff #67726)

No worries about bikeshedding, I don't mind cleaning this up and it's good to have a consistent idea of where we're headed.

That said, I propose:

  • For now, commit this as "vm.debug.uma_multipage_slabs", i.e. under the existing node.
  • "Soon", rename "vm.debug" to "debug.vm". (Or "debug.vm.uma"? TBD.)
  • And also move the various uma_dbg stuff in uma_core.c to uma_dbg.c.
markj added inline comments.Feb 4 2020, 4:56 PM
sys/vm/uma_core.c
365 ↗(On Diff #67726)

This sounds perfectly reasonable to me. I vote for debug.vm.uma FWIW.

rlibby updated this revision to Diff 67787.Feb 4 2020, 7:54 PM

Rename from vm.multipage_slabs to vm.debug.uma_multipage_slabs

This revision now requires review to proceed.Feb 4 2020, 7:54 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 4 2020, 10:40 PM
This revision was automatically updated to reflect the committed changes.