Page MenuHomeFreeBSD

uma: retire UMA_MD_SMALL_ALLOC
AbandonedPublic

Authored by bnovkov on May 1 2024, 1:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jul 7, 5:16 PM
Unknown Object (File)
Sat, Jul 6, 8:36 PM
Unknown Object (File)
Fri, Jul 5, 5:59 AM
Unknown Object (File)
Thu, Jun 27, 8:53 PM
Unknown Object (File)
Thu, Jun 27, 8:53 PM
Unknown Object (File)
Thu, Jun 27, 8:53 PM
Unknown Object (File)
Thu, Jun 27, 8:53 PM
Unknown Object (File)
Thu, Jun 13, 6:29 PM

Details

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

mhorne added inline comments.
sys/amd64/include/vmparam.h
75 ↗(On Diff #137956)

No longer "machine specific".

sys/kern/subr_vmem.c
634 ↗(On Diff #137956)

Mentions uma_small_alloc().

sys/vm/uma_core.c
1877–1878

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.

2148

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

sys/vm/uma_core.c
2148

Ignore this, I didn't see the parent revisions :^)

sys/vm/uma_core.c
1898

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.

2162

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.

bnovkov edited the summary of this revision. (Show Details)

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 added inline comments.
sys/vm/uma_core.c
1877–1878

I agree, it is a messy expression.
I'll look at other instances and try to land a fix in a separate patch.

2162

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.

sys/vm/uma_core.c
1877–1878

I think you can write #if MINIDUMP_PAGE_TRACKING == 1 && defined(UMA_USE_DMAP). But yes that should be a separate change.

bnovkov marked 2 inline comments as done.
bnovkov edited the summary of this revision. (Show Details)

Address @markj 's comment - uma_small_alloc code deduplication was carved out into a separate revision.

This revision is now accepted and ready to land.Jun 10 2024, 11:31 PM