Page MenuHomeFreeBSD

uma: unify layout paths and improve efficiency

Authored by rlibby on Jan 6 2020, 1:00 AM.



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

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

Event Timeline

rlibby created this revision.Jan 6 2020, 1:00 AM
rlibby added a comment.Jan 6 2020, 1:02 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.

markj added a comment.Jan 6 2020, 2:31 PM

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

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
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.

markj accepted this revision.Jan 6 2020, 6:21 PM

LGTM other than the minor comments.

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
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().

rlibby added a comment.Jan 6 2020, 8:05 PM

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

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.

rlibby updated this revision to Diff 66477.Jan 8 2020, 8:32 AM
rlibby marked 7 inline comments as done.

jeff & markj feedback

This revision now requires review to proceed.Jan 8 2020, 8:32 AM
rlibby added inline comments.Jan 8 2020, 8:33 AM
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.

rlibby updated this revision to Diff 66478.Jan 8 2020, 8:43 AM

forgot jeff feedback s/eff_nohdr/eff_offpage/

jeff accepted this revision.Jan 8 2020, 8:49 AM
jeff added inline comments.
1837 ↗(On Diff #66478)

Can we do ipers_offpage to be consistent with eff_offpage?

This revision is now accepted and ready to land.Jan 8 2020, 8:49 AM
rlibby updated this revision to Diff 66480.Jan 8 2020, 8:59 AM

jeff feedback, s/ipers_nohdr/ipers_offpage/ too

We can always revisit the names if/when we do vm_page-embedded slab

This revision now requires review to proceed.Jan 8 2020, 8:59 AM
rlibby marked 3 inline comments as done.Jan 8 2020, 9:01 AM
rlibby added inline comments.
1837 ↗(On Diff #66478)


markj accepted this revision.Jan 8 2020, 4:29 PM
This revision is now accepted and ready to land.Jan 8 2020, 4:29 PM
This revision was automatically updated to reflect the committed changes.
rlibby marked an inline comment as done.