uma: retire UMA_MD_SMALL_ALLOC
AbandonedPublic
Actions

Authored by bnovkov on May 1 2024, 1:44 PM.

Details

Reviewers

alc
markj
kib
andrew
manu

Summary

This patch replaces uma_small_alloc with a reservation-aware counterpart.

The machine-dependent UMA code is no longer tasked with page allocation, only with arranging access to DMAP addresses if need be.
small_alloc will now use uma_{dmap_to_vm_page, vm_page_to_dmap} to fetch DMAP addresses for individual pages.
UMA_MD_SMALL_ALLOC was removed and replaced by UMA_MD_DMAP_HOOK that can be used to override the default uma_{dmap_to_vm_page, vm_page_to_dmap} implementation.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bnovkov created this revision.May 1 2024, 1:44 PM

Herald added a reviewer: andrew. · View Herald TranscriptMay 1 2024, 1:44 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: riscv, olce, jhibbits and 2 others. · View Herald Transcript

bnovkov requested review of this revision.May 1 2024, 1:44 PM

bnovkov added a child revision: D45043: vm_reserv: introduce unmanaged reservations.May 1 2024, 1:54 PM

bnovkov added a parent revision: D45045: malloc(9): introduce M_NEVERFREED.

mhorne added a subscriber: mhorne.May 1 2024, 5:01 PM

mhorne added inline comments.

sys/amd64/include/vmparam.h
74–75	No longer "machine specific".
sys/kern/subr_vmem.c
634	Mentions `uma_small_alloc()`.
sys/vm/uma_core.c
1871–1872	Somewhat tangential to this patch, but this should be expressed differently. The same complex conditional appears in `sys/vm/vm_page.c`. Something like `#define VM_DUMP_STARTUP_PAGES` in `vmparam.h` would serve better as a flag we could check. Let me know if you'd like me to handle it.
2124	Is something missing from the diff? I don't see where this is used or anything to account for the absence of `uma_small_alloc()`.

mhorne added inline comments.May 1 2024, 5:02 PM

sys/vm/uma_core.c
2124	Ignore this, I didn't see the parent revisions :^)

kib added inline comments.May 1 2024, 10:14 PM

sys/vm/uma_core.c
1892	It is well-known issue with our linkers that they wrongly interpret the meaning of 'weak'. There is no mention of the priority of resolution among weak/non-weak in the ELF spec. I would much prefer avoid adding more such cases.
2138	Isn't this sequence racy? Suppose that the page m is reused right after free() but before drop_page(). Then we might leave page out of the dump when raced.

Address @mhorne 's and @kib 's comments:

Removed leftover references to old code in comments
Replaced __weak_symbol overriding with UMA_MD_DMAP_HOOK

bnovkov marked 7 inline comments as done.May 2 2024, 4:41 PM

bnovkov added inline comments.

sys/vm/uma_core.c
1871–1872	I agree, it is a messy expression. I'll look at other instances and try to land a fix in a separate patch.
2138	You're right, thank you for catching this. I've placed the dump drop before the page gets freed.

IMO this patch is in the wrong order in the series. It should be first: 98% of the patch is about deduplicating implementations of uma_small_alloc(), which is a non-functional change and worthy in its own right. The rest of the series, which needs more design review and discussion before getting into detailed code review, should build on top of that.

I think the first patch should simply deduplicate the uma_small_alloc() implementations without modifying the internals of the powerpc implementation.