Details

Reviewers

jeff
markj

Commits

rS356535: uma: unify layout paths and improve efficiency

Summary

Unify the keg layout selection paths (keg_small_init, keg_large_init,
keg_cachespread_init), and slightly improve memory efficiecy by:

using the padding of the final item to store the slab header,
not going OFFPAGE if we have a choice unless it improves efficiency.

Test Plan

kyua test -k /usr/tests/sys/Kyuafile

sysctl vm.uma ...

$ diff --side-by-side --suppress-common-lines vm.uma.before-layout-short.txt vm.uma.after-3-layout-short.txt 
vm.uma.SLEEPQUEUE.keg.efficiency: 96			      |	vm.uma.SLEEPQUEUE.keg.efficiency: 100
vm.uma.SLEEPQUEUE.keg.ipers: 31				      |	vm.uma.SLEEPQUEUE.keg.ipers: 32
vm.uma.tfo_1.keg.ipers: 252				      |	vm.uma.tfo_1.keg.ipers: 253
vm.uma.tfo_3.keg.ipers: 252				      |	vm.uma.tfo_3.keg.ipers: 253
vm.uma.tfo_ccache_entries_1.keg.efficiency: 96		      |	vm.uma.tfo_ccache_entries_1.keg.efficiency: 100
vm.uma.tfo_ccache_entries_1.keg.ipers: 31		      |	vm.uma.tfo_ccache_entries_1.keg.ipers: 32
vm.uma.tfo_ccache_entries_3.keg.efficiency: 96		      |	vm.uma.tfo_ccache_entries_3.keg.efficiency: 100
vm.uma.tfo_ccache_entries_3.keg.ipers: 31		      |	vm.uma.tfo_ccache_entries_3.keg.ipers: 32
vm.uma.tfo_ccache_entries.keg.efficiency: 96		      |	vm.uma.tfo_ccache_entries.keg.efficiency: 100
vm.uma.tfo_ccache_entries.keg.ipers: 31			      |	vm.uma.tfo_ccache_entries.keg.ipers: 32
vm.uma.tfo.keg.ipers: 252				      |	vm.uma.tfo.keg.ipers: 253

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rlibby created this revision.Jan 6 2020, 1:00 AM

Harbormaster completed remote builds in B28497: Diff 66397.Jan 6 2020, 1:00 AM

Jeff asked that the old code be left in place for posting the diff, for readability of the diff. The #ifdef 0 code will be deleted before commit.

This mostly seems ok to me, I just had a couple of questions.

sys/vm/uma_core.c
1677 ↗	(On Diff #66397)	Can you explain how this definition works? UMA_MAX_WASTE is the "max waste percentage" and for the default value of 10 I can see why UMA_MIN_EFF_FRAC gives the right value, but I can't see how it makes sense for UMA_MAX_WASTE == 20 for instance.
1793 ↗	(On Diff #66397)	This looks like it should be rsize instead of uk_size, but I guess it makes no difference for now?
1812 ↗	(On Diff #66397)	You can combine the tests here like you do above.

rlibby added inline comments.Jan 6 2020, 5:20 PM

sys/vm/uma_core.c
1677 ↗	(On Diff #66397)	Hmm, I see now that the comment does say "percentage", and by coincidence it actually was with the value of 10. But that wasn't how it was used, `(wastedspace >= slabsize / UMA_MAX_WASTE)`, which means 1/10th of slab size. A value of 20 would have meant 1/20th, or 5%. I was following the definition in code, but I wouldn't be opposed to changing it actually to be a percentage like the comment claims, and changing this to `UMA_FRAC(100 - UMA_MAX_WASTE, 100)`, or maybe just `UMA_FRAC(9, 10)` and deleting `UMA_MAX_WASTE`.
1793 ↗	(On Diff #66397)	If it makes a difference, then that would mean we are allocating a page for padding. Surely we don't want to do that? However in reality our alignment must be a power of two, so it couldn't happen without greater than page alignment, which we should maybe just assert about.
1812 ↗	(On Diff #66397)	Will do.

LGTM other than the minor comments.

sys/vm/uma_core.c
1677 ↗	(On Diff #66397)	Ah, thanks. I think keeping the definition as-is (optionally removing UMA_MAX_WASTE) and fixing the comment would be fine. I don't have any strong feelings about it, just got confused by the comment.
1793 ↗	(On Diff #66397)	I came to the same conclusion after remembering that UMA only implements sub-page alignment. I think an assertion would be useful, maybe alongside the one in uma_zcreate().

This revision is now accepted and ready to land.Jan 6 2020, 6:21 PM

jeff added inline comments.Jan 6 2020, 7:43 PM

sys/vm/uma_core.c
1677 ↗	(On Diff #66397)	Given the macros above I'd prefer to define it as a 0-100 value and make a UMA_PCT_FRAC()
1680 ↗	(On Diff #66397)	Please comment about the definition of the hdr bool.
1756 ↗	(On Diff #66397)	In retrospect I don't know why I defined the alignment as a mask.
1782 ↗	(On Diff #66397)	Pretty lame of me to have left a "128k" constant in the code. This is just limiting the size of the contiguous allocation that we'll do to something somewhat sane. Can you wrap it into a define like UMA_CACHESPREAD_MAX_CONTIG or something similar.
1812 ↗	(On Diff #66397)	Also I have slowly been making the code compliant with style by explicitly testing against 0 or != 0.
1817 ↗	(On Diff #66397)	Can this not now just call slab_ipers_hdr directly rather than doing the goto?
1831 ↗	(On Diff #66397)	hdr/nohdr is not consistent with other language. Can we refer to it as ipers_inline/ipers_offpage? It's wordier but more explicit.
1848 ↗	(On Diff #66397)	for ipers == 1 we can always use the uk_off because we always have the first element. For large allocations this will be much faster because you don't have to walk the page tables in pmap_kextract().

@jeff Will do for the rest of the comments. A couple clarifying questions.

sys/vm/uma_core.c
1817 ↗	(On Diff #66397)	Yes... it should. I wrote it this way in anticipation of retrying the `/* TODO: vm_page-embedded slab */`, but I now see that even with vm_page-embedded slab, if we get here then we won't embed the next time around either.
1831 ↗	(On Diff #66397)	What would you say to eff_offpage, but keeping ipers_nohdr? In anticipation of vm_page-embedded slab header. ipers_nohdr / _offpage applies to both offpage and embedded. So there would be ipers, eff, ipers_nohdr, eff_offpage, and in the future also eff_embed.
1848 ↗	(On Diff #66397)	I agree... do you see the need for a change here? Or a change to the comment? ipers == 1 falls into the "all item start addresses are in the same page" case.

jeff & markj feedback

This revision now requires review to proceed.Jan 8 2020, 8:32 AM

Harbormaster completed remote builds in B28534: Diff 66477.Jan 8 2020, 8:32 AM

rlibby added inline comments.Jan 8 2020, 8:33 AM

sys/vm/uma_core.c
1756 ↗	(On Diff #66397)	Alas. Probably not worth worrying about. I think if we tried to fix it up below the API, it might just make things more confusing.