Page MenuHomeFreeBSD

Use a vm_domainset iterator in keg_fetch_slab().
ClosedPublic

Authored by markj on Oct 4 2018, 8:00 PM.

Details

Summary

Right now, UMA uses a hand-rolled round-robin policy. Aside from the
duplication of logic, this means that we're not following the "minskip"
policy introduced in r338507. We also want to avoid blocking in
individual domains with a WAITOK allocation. Thus, refactor
keg_fetch_slab() to make use of a vm_domainset iterator instead.

I changed keg_alloc_slab() to return with the keg unlocked on failure,
identical to zone_fetch_slab_multi(). I also made changes to ensure
that uk_reserve is only accessed with the keg lock held.

One side effect of this is that UMA allocations are now round-robin by
thread rather than round-robin by keg. I don't believe this will matter
in practice.

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

markj created this revision.Oct 4 2018, 8:00 PM
emaste added a subscriber: emaste.Oct 4 2018, 8:19 PM
jeff added a comment.Oct 10 2018, 11:56 PM

This is surprisingly simple. I am a little uneasy with the prospect of not doing round-robin on the keg's iterator. Can we not pass in an iterator somehow?

sys/vm/uma_core.c
2633 ↗(On Diff #48745)

This name makes the code below confusing to read. It looks like we only fetch the first slab if we're allowed to use reserve.

Can we name it something like:
keg_available_slab()

I don't love that name either but anything that indicates that it's a filter or limit check and not dipping into the reserve specifically.

markj marked 2 inline comments as done.Oct 11 2018, 1:21 AM
In D17420#373676, @jeff wrote:

This is surprisingly simple. I am a little uneasy with the prospect of not doing round-robin on the keg's iterator. Can we not pass in an iterator somehow?

I tried to find a way to do that; the issue is that a) we want to update the iterator with the keg locked, and b) we want to be able to call vm_wait_doms() in vm_domainset_iter_policy(), but we have to drop the keg lock to do that.

We could pass the lock object in as a separate parameter, but I also want to minimize the number of parameters we pass to vm_domainset_*() since those functions are called frequently. Do you see a different approach?

sys/vm/uma_core.c
2633 ↗(On Diff #48745)

My intention was just to prevent unsynchronized access to uk_reserve, but we can do a bit better and define a subroutine which returns the first free slab if we're not dipping into the reserve. That eliminates some code duplication and makes it easier to find a decent name.

markj updated this revision to Diff 48999.Oct 11 2018, 1:22 AM
markj marked an inline comment as done.
  • Add keg_fetch_free_slab().
jeff added a comment.Oct 11 2018, 10:26 PM
In D17420#373676, @jeff wrote:

This is surprisingly simple. I am a little uneasy with the prospect of not doing round-robin on the keg's iterator. Can we not pass in an iterator somehow?

I tried to find a way to do that; the issue is that a) we want to update the iterator with the keg locked, and b) we want to be able to call vm_wait_doms() in vm_domainset_iter_policy(), but we have to drop the keg lock to do that.
We could pass the lock object in as a separate parameter, but I also want to minimize the number of parameters we pass to vm_domainset_*() since those functions are called frequently. Do you see a different approach?

We could pass a domainset_ref to the iterator initialization instead of just a policy. This will give us the right iter pointer. The keg could have a domainset_ref embedded that is set according to initialization flags.

We could always do NOWAIT iterations and put the blocking in the caller to avoid passing in the lock. It's not that much extra code for the caller.

markj updated this revision to Diff 49228.Oct 16 2018, 4:11 PM

Use a per-keg domainset_ref to maintain per-keg round-robin, as requested
by Jeff.

jeff added a comment.Oct 17 2018, 9:12 PM

I like this

This revision was not accepted when it landed; it landed in state Needs Review.Oct 24 2018, 4:42 PM
This revision was automatically updated to reflect the committed changes.