Make it possible to disable NUMA support with a tunable.
ClosedPublic
Actions

Authored by markj on Oct 5 2018, 7:31 PM.

Details

Reviewers

manu
alc
kib
jeff
jhb
gallatin
emaste

Group Reviewers

manpages

Commits

rS339616: Make it possible to disable NUMA support with a tunable.

Summary

This change depends on D17416. The idea is to provide a workaround for
anyone that's negatively impacted by NUMA being configured in GENERIC.
Setting vm.ndomains=1 in the loader overrides the NUMA topology
detection in srat.c.

Disabling NUMA this way does not completely remove the overhead
associated with this option. In particular, we will still go through the vm_domainset
iterator code on every memory allocation.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Oct 5 2018, 7:31 PM

Herald added a reviewer: manu. · View Herald TranscriptOct 5 2018, 7:31 PM

Herald added subscribers: emaste, andrew. · View Herald Transcript

Harbormaster completed remote builds in B20012: Diff 48802.Oct 5 2018, 7:31 PM

markj edited the summary of this revision. (Show Details)Oct 5 2018, 7:34 PM

markj added reviewers: alc, kib, jeff, jhb, gallatin, emaste.

kib added inline comments.Oct 5 2018, 7:50 PM

sys/vm/vm_phys.c
609 ↗	(On Diff #48802)	It would be slightly more friendly to user to fall back to either 1 or n in this case.
sys/x86/acpica/srat.c
538 ↗	(On Diff #48802)	I would write this in one line using ?: as in acpi_map_pxp_to_vm_domain() below.

Handle kib's notes.

Harbormaster completed remote builds in B20013: Diff 48804.Oct 5 2018, 7:54 PM

markj marked 2 inline comments as done.Oct 5 2018, 7:58 PM

Please break out the man page changes that are not related to the boot time tuneable into a seperate commit/differential.

share/man/man4/numa.4
34 ↗	(On Diff #48802)	Does not MAXMEMDOM still take an interger argument? And I think the default is 8?
68 ↗	(On Diff #48802)	To me this is the only change to the man page that belongs with this review, all the other changers have nothing to do with the code change in this review.

kib accepted this revision.Oct 5 2018, 8:05 PM

This revision is now accepted and ready to land.Oct 5 2018, 8:05 PM

andrew added inline comments.Oct 6 2018, 9:34 AM

sys/arm64/arm64/mp_machdep.c
579–581 ↗	(On Diff #48804)	This looks wrong when `vm_domains` is 1, `domain` will be unset. We'll also need to read the domain for the dual socket ThunderX and store it somewhere even in the single vm_domain case. This is because some cross socket interrupts don't work.

markj marked 2 inline comments as done.Oct 8 2018, 5:55 PM

markj added inline comments.

share/man/man4/numa.4
34 ↗	(On Diff #48802)	It does. The default is platform-dependent.
sys/arm64/arm64/mp_machdep.c
579–581 ↗	(On Diff #48804)	Hrmm. I would really like to be able to implement, e.g., pcpu_page_alloc() without an #ifdef NUMA. Could you point me to the code which depends on pc_domain's value?

My only suggestion might be to consider an explicit "disabled" tunable. For SMP we have 'kern.smp.disabled=1'. Having 'vm.numa.disabled=1' or 'kern.numa.disabled=1' might be more intuitive than setting ndomains to 1. (The implementation might be that you set ndomains to 1 if the tunable is set, but it's the UI I'm thinking of).

Fix vm_ndomains=1 case on arm64.
Change the tunable to vm.numa.disabled.

This revision now requires review to proceed.Oct 8 2018, 8:12 PM

Harbormaster completed remote builds in B20071: Diff 48906.Oct 8 2018, 8:12 PM

markj marked an inline comment as done.Oct 8 2018, 8:13 PM

markj added inline comments.

sys/arm64/arm64/mp_machdep.c
579–581 ↗	(On Diff #48804)	Are you sure that the value of pc_domain matters? I note that we read and store a numa-node-id property in the GICv3 code, and I'm able to boot a dual-socket ThunderX with this patch applied.

gallatin accepted this revision.Oct 8 2018, 8:37 PM

This revision is now accepted and ready to land.Oct 8 2018, 8:37 PM

alc accepted this revision.Oct 18 2018, 5:02 PM

alc added inline comments.

share/man/man4/numa.4
52–53 ↗	(On Diff #48906)	You can shorten this to: "By default it attempts to balance allocations across the domains." (As an aside, this sentence highlights the incomplete definition of NUMA above, specifically, that it does not mention the typical differences in interdomain versus intradomain bandwidth.)
sys/vm/vm_phys.c
608 ↗	(On Diff #48906)	Style? `d != 0`