Page MenuHomeFreeBSD

uma: Respect uk_reserve in keg_drain()

Authored by markj on Oct 13 2020, 8:57 PM.



When we set a reserve of free items for a zone, we must take care to avoid
reclamining them. Modify keg_drain() to simply respect the reserved pool. For
now we take uk_reserve to be a per-NUMA domain quantity, since that's what
existing code does.

While here remove an always-false uk_freef == NULL check (kegs that
shouldn't be drained should set _NOFREE instead), and make sure that the
keg_drain() KTR statement does not reference an uninitialized variable.

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Oct 13 2020, 8:57 PM
markj created this revision.
1436–1437 ↗(On Diff #78194)

If it might be the case that the reserve could be large, this could prevent some pointless list surgery when we can't reclaim anything:

	n = dom->ud_free_slabs;
	if (n == 0 || dom->ud_free_items - keg->uk_ipers < keg->uk_reserve)
		goto unlock;
	LIST_SWAP(&freeslabs, &dom->ud_free_slab, uma_slab, us_link);
1443 ↗(On Diff #78194)

But there are also free items in the partial slabs, and those work fine for reserve allocations. This loop needs to include some kind of comparison of ud_free_items vs uk_reserve. Maybe:

	free_items = dom->ud_free_items - n * keg->uk_ipers;
	while (free_items < keg->uk_reserve) {
		free_items += keg->uk_ipers;
1448 ↗(On Diff #78194)

This reverses the order. Do we care? If so, we can do tail inserts by keeping track of the tail and using LIST_INSERT_AFTER.

1449–1451 ↗(On Diff #78194)

Rather than remove from hash above and re-insert here, can't we just move the hash remove block to below the re-insert block (and walk &freeslabs instead)?

markj marked 2 inline comments as done.
  • Address feedback.
  • Add a uk_reserve sysctl for each keg.
1436–1437 ↗(On Diff #78194)

I think we can go a bit further and determine whether we need to keep more slabs than we'd free, and in that case pull individual slabs off of the domain's free slab list.

1448 ↗(On Diff #78194)

I can't think of any reason this would hurt anything.

Don't assume that the number of free items is larger than the reserve.

Fix the problem by avoiding underflow instead.

1436–1437 ↗(On Diff #78194)

Here's a suggestion for rephrasing that into one loop where we calculate the number of slabs to keep up front. Take or leave according to your taste.

	partial = dom->ud_free_items - dom->ud_free_slabs * keg->uk_ipers;
	keep_slabs = 0;
	if (partial < keg->uk_reserve) {
		keep_slabs = min(dom->ud_free_slabs,
		    howmany(keg->uk_reserve - partial, keg->uk_ipers));
	n = dom->ud_free_slabs - keep_slabs;

	for (i = min(keep_slabs, n); i > 0; i--) {
		slab = LIST_FIRST(&dom->ud_free_slab);
		LIST_REMOVE(slab, us_link);
		LIST_INSERT_HEAD(&freeslabs, slab, us_link);

	if (keep_slabs < n)
		LIST_SWAP(&freeslabs, &dom->ud_free_slab, uma_slab, us_link);
1445 ↗(On Diff #78267)

>= ? If it's equal to reserve + ipers, we can subtract ipers and still meet reserve.

1436–1437 ↗(On Diff #78194)

Thanks, that's much nicer than what I wrote.

Simplify keg_drain_domain().

This revision is now accepted and ready to land.Oct 16 2020, 4:48 PM
1433 ↗(On Diff #78318)

Suggested comment to describe what follows: Are the free items in partially allocated slabs sufficient to meet the reserve? If not, compute the number of fully free slabs that must be kept.

1441 ↗(On Diff #78318)

When I first read this "min" operation, my immediate was "what in the world is going on here." I think that this deserves a comment explaining that this is an optimization.

By the way, in case you are going to use the summary in the commit message, "reclamining" is misspelled.

markj marked 2 inline comments as done.

Add comments per Alan's notes.

This revision now requires review to proceed.Oct 16 2020, 7:55 PM
This revision is now accepted and ready to land.Oct 16 2020, 8:47 PM
This revision was automatically updated to reflect the committed changes.