Page MenuHomeFreeBSD

vmem: Simplify bt_fill() callers a bit
ClosedPublic

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

Details

Summary

No functional change intended.

Diff Detail

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

Event Timeline

markj requested review of this revision.Oct 13 2020, 8:56 PM
markj created this revision.
This revision is now accepted and ready to land.Oct 13 2020, 10:36 PM
sys/kern/subr_vmem.c
1523 ↗(On Diff #78191)

This function can be further simplified:

error = bt_fill(vm, flags);
if (error == 0)

Why not just put the logic in bt_fill(), is there a future patch that plans to use _bt_fill()?

Or were you hoping to have bt_fill() inlined and _bt_fill() not inlined? If the latter, maybe consider adding the inline/noinline tags?

sys/kern/subr_vmem.c
1411–1418 ↗(On Diff #78191)

As an "oh, by the way," this code does not really implement "best fit". It selects the first available tag that is sufficiently large, not the best fit. The NetBSD version acknowledges this in a comment that didn't get brought over to FreeBSD:

		/*
		 * we assume that, for space efficiency, it's better to
		 * allocate from a smaller block.  thus we will start searching
		 * from the lower-order list than VM_INSTANTFIT.
		 * however, don't bother to find the smallest block in a free
		 * list because the list can be very long.  we can revisit it
		 * if/when it turns out to be a problem.
		 *
		 * note that the 'first' list can contain blocks smaller than
		 * the requested size.  thus we need to check bt_size.
		 */

When I first noticed this, I also looked at the old OpenSolaris sources. As per the earlier vmem paper, their implicit default is "instant fit", which behaves like our "first fit". Alternatively, you can explicitly request "first fit", which behaves like our "best fit", or "best fit", which we don't have. The earlier vmem paper didn't mention a "first fit" policy.

Why not just put the logic in bt_fill(), is there a future patch that plans to use _bt_fill()?

Or were you hoping to have bt_fill() inlined and _bt_fill() not inlined? If the latter, maybe consider adding the inline/noinline tags?

Yeah, I wanted to inline bt_fill() but not _bt_fill() to preserve the old code layout. With some usage patterns vmem_xalloc() will hit the "slow" path most of the time, so I'm not sure it matters much.

This revision now requires review to proceed.Oct 14 2020, 1:23 AM
This revision is now accepted and ready to land.Oct 14 2020, 3:26 AM
sys/kern/subr_vmem.c
1398–1401 ↗(On Diff #78203)
error = bt_fill(vm, flags);
if (error != 0)
        break;
1520 ↗(On Diff #78203)

This assignment is now redundant.

markj marked 2 inline comments as done.
  • Address alc's feedback.
This revision now requires review to proceed.Oct 14 2020, 2:05 PM
This revision is now accepted and ready to land.Oct 14 2020, 4:36 PM
This revision was automatically updated to reflect the committed changes.