Details

Reviewers

alc
kib
jeff
gallatin

Group Reviewers

manpages

Commits

rS339452: Create some global domainsets and refactor NUMA registration.

Summary

I'm finding that it's useful to have static global domainsets for use in
kernel allocators (malloc(), kmem_malloc(), UMA, kstacks). Add some
plumbing to make that easy:

Add a vm_phys routine that can be invoked by MD code once the NUMA topology is discovered. Right now this is only used by the SRAT/SLIT parser, but will likely be used by at least the FDT code on non-ACPI systems in the future.
Predefined domainsets are allocated from .data instead of from the domainset zone. This lets us use them in UMA without introducing a bootstrapping problem. This results in a bit of bloat, but not much: struct domainset is 40 bytes with LP64, and MAXMEMDOM is small on all platforms.
Predefine domainset_roundrobin and domainset_prefer.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Oct 4 2018, 7:54 PM

Harbormaster completed remote builds in B19975: Diff 48740.Oct 4 2018, 7:54 PM

Remove a useless comment.

Harbormaster completed remote builds in B19977: Diff 48742.Oct 4 2018, 7:59 PM

markj edited the summary of this revision. (Show Details)Oct 4 2018, 8:11 PM

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

emaste added a subscriber: emaste.Oct 4 2018, 8:18 PM

markj mentioned this in D17304: Swap in processes unless there's a global memory shortage..Oct 4 2018, 8:20 PM

kib accepted this revision.Oct 5 2018, 5:34 PM

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

markj added a child revision: D17439: Make it possible to disable NUMA support with a tunable..Oct 5 2018, 7:31 PM

You mention kstacks. I have not gone through the rest of your reviews, but I was wondering if you were working on affinitizing kstacks. Did you plan to break the cache out by domain, or drop it for affinitized kstack allocations? The default is so undersized these days, we're mostly not getting the benefit of the cache anyway (and the cache might be pessimal, since it is protected by a global lock).

In D17416#372648, @gallatin wrote:

You mention kstacks. I have not gone through the rest of your reviews, but I was wondering if you were working on affinitizing kstacks. Did you plan to break the cache out by domain, or drop it for affinitized kstack allocations? The default is so undersized these days, we're mostly not getting the benefit of the cache anyway (and the cache might be pessimal, since it is protected by a global lock).

I wasn't working on affinity. Rather, I want to ensure that the kstack allocation policy allows allocations from any domain, ensuring that we can permit kstack swapins so long as at least one domain has some free pages. See D17304; it's really just a bug fix for 12.0.

I haven't thought much about the problems you listed. Do you have a proposal?

markj mentioned this in D17417: Refactor domainset iterators in preparation for use by malloc() and UMA..Oct 8 2018, 5:17 PM

gallatin added inline comments.Oct 8 2018, 5:55 PM

sys/sys/domainset.h
100 ↗	(On Diff #48742)	Can we abbreviate these radically? I can see them being used in your follow-on reviews, and having them so seems to push a lot of things to the next line. Maybe DSET_RR and DSET_PREF ?

In D17416#372763, @markj wrote:

In D17416#372648, @gallatin wrote:

You mention kstacks. I have not gone through the rest of your reviews, but I was wondering if you were working on affinitizing kstacks. Did you plan to break the cache out by domain, or drop it for affinitized kstack allocations? The default is so undersized these days, we're mostly not getting the benefit of the cache anyway (and the cache might be pessimal, since it is protected by a global lock).

I wasn't working on affinity. Rather, I want to ensure that the kstack allocation policy allows allocations from any domain, ensuring that we can permit kstack swapins so long as at least one domain has some free pages. See D17304; it's really just a bug fix for 12.0.

I haven't thought much about the problems you listed. Do you have a proposal?

I do not. I have been doing a lot of ad-hoc domain affinitization and noticed kstacks as a potential problem area. I took the small default size of the cache and the global lock around it as a sign that nobody has really thought about kstacks for quite some time.

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

markj added inline comments.

sys/sys/domainset.h
100 ↗	(On Diff #48742)	Ok, I will change them. I also meant to document them in domainset(9).

Rename and document macros.
Annotate the global domainsets with __read_mostly.

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

Harbormaster completed remote builds in B20072: Diff 48908.Oct 8 2018, 8:28 PM

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

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

I'm not a fan of DSET_ but otherwise this LGTM.

sys/sys/domainset.h
100 ↗	(On Diff #48742)	The other macros in this file use DOMAINSET_. Is that not brief enough? I don't mind pref/rr.

markj marked an inline comment as done.Oct 11 2018, 1:28 AM

markj added inline comments.

sys/sys/domainset.h
100 ↗	(On Diff #48742)	My preference also is to keep the namespace consistent and use DOMAINSET_, but I didn't want to quibble over it. If @gallatin is ok with that change I'll make it.

gallatin added inline comments.Oct 11 2018, 1:09 PM

sys/sys/domainset.h
100 ↗	(On Diff #48742)	That's fine with me. Especially if we make malloc_domain() a wrapper for malloc_domainset(...DOMAINSET_PREFER...) as I think was suggested on one of the other reviews.