Page MenuHomeFreeBSD

Add SLIT enumeration and memory locality table to VM/ACPI.
ClosedPublic

Authored by adrian on May 6 2015, 1:14 AM.

Details

Summary

This adds ACPI SLIT table parsing to the x86 ACPI code and some very simple
storage/awareness in vm_phys.c. It exposes the locality map via sysctl.
It doesn't at all attempt to use it for memory / scheduling decisions.

Diff Detail

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

Event Timeline

adrian updated this revision to Diff 5221.May 6 2015, 1:14 AM
adrian retitled this revision from to Add SLIT enumeration and memory affinity table to VM/ACPI..
adrian updated this object.
adrian edited the test plan for this revision. (Show Details)
adrian added a reviewer: jhb.
adrian retitled this revision from Add SLIT enumeration and memory affinity table to VM/ACPI. to Add SLIT enumeration and memory locality table to VM/ACPI..May 6 2015, 1:16 AM
adrian updated this object.

Looks ok to me. There are some uncodified assumptions in the ACPI to vm_locality mapping that I'd prefer to see in some sort of ASSERT form. The printfs in the parsing probably go away or are cleaned up too.

sys/vm/vm_phys.c
320 ↗(On Diff #5221)

style

rpaulo accepted this revision.May 6 2015, 1:44 AM
rpaulo added a reviewer: rpaulo.
rpaulo added a subscriber: rpaulo.

Only minor comments.

sys/vm/vm_phys.c
320 ↗(On Diff #5221)

Extra ;

sys/x86/acpica/srat.c
127 ↗(On Diff #5221)

Needs to be removed.

129 ↗(On Diff #5221)

This isn't how ACPI resources work. Just use if (acpi_disabled("slit")).

130 ↗(On Diff #5221)

Usually we don't print messages here.

145 ↗(On Diff #5221)

This looks bogus...

149 ↗(On Diff #5221)

error is always 0.

156 ↗(On Diff #5221)

It might be better to create VM accessor methods to set/get the memory locality.

404 ↗(On Diff #5221)

acpi_disabled() again.

441 ↗(On Diff #5221)

Unnecessary parentheses.

This revision is now accepted and ready to land.May 6 2015, 1:44 AM
adrian updated this revision to Diff 5222.May 6 2015, 1:52 AM
adrian edited edge metadata.

Address some of the concerns from rui.

This revision now requires review to proceed.May 6 2015, 1:52 AM
ngie added a subscriber: ngie.May 6 2015, 4:50 AM
ngie added inline comments.
sys/vm/vm_phys.c
144–147 ↗(On Diff #5222)

Sort the sysctl?

318 ↗(On Diff #5222)

Why have a hardcoded value of 255?

341–343 ↗(On Diff #5222)

style(9): remove braces

sys/x86/acpica/srat.c
428–431 ↗(On Diff #5222)

Ow. It would be nice if this was a constant, and maybe something like -1. Or is this documented somewhere in the ACPI spec...?

443 ↗(On Diff #5222)

Maybe parse_slit should be a void return type, not int. That would remove the "need" for the (void) cast.

446–447 ↗(On Diff #5222)

Move this back up to the same spot as the old code?

stas accepted this revision.May 6 2015, 6:01 AM
stas added a reviewer: stas.
stas added a subscriber: stas.

Looks good. Besides things ngie already commented on I don't see any issues.

This revision is now accepted and ready to land.May 6 2015, 6:01 AM
jhb edited edge metadata.May 6 2015, 6:18 PM

In general I think this could wait until we actually made use of the info. However, I assume you are already doing that in an outside branch and just pushing up that one part?

sys/vm/vm_phys.c
74 ↗(On Diff #5222)

You don't need the NULL assignment. The BSS is zero already (note that mem_affinity on the previous line relies on this as does kernel code in general).

309 ↗(On Diff #5222)

What are f and t? I think the i and j you use below in the caller is a bit more obvious that these are just indices into a matrix. I don't think f and t imply that to the reader.

318 ↗(On Diff #5222)

Probably because the matrix contains uint8_t values, so 255 is probably used to mark invalid cells.

sys/x86/acpica/srat.c
73 ↗(On Diff #5222)

Maybe group these with the SRAT members up above? Generally FreeBSD kernel code puts all the globals together in one spot in the file.

78 ↗(On Diff #5222)

Please call this slit_parse_table() or some such. The name "walk_table" is only used in other ACPI/x86 places for functions that iterate over tables that contain subentries invoking a callback on each subentry (e.g. MPTable, SMBIOS, SRAT). In this case you are just parsing the matrix directly.

94 ↗(On Diff #5222)

Trim extra newline.

134 ↗(On Diff #5222)

You can drop these extra braces.

149 ↗(On Diff #5222)

Yes, error is unused and should be removed.

404 ↗(On Diff #5222)

That's an old bug. :) It's been this way for a long time, so using resource_disabled for "slit" is probably more consistent.

428–431 ↗(On Diff #5222)

I think he's doing this due to the table being uint8_t. It could perhaps be a matrix of ints instead and use -1 for this (which would also simplify the code in vm_phys.c by removing the 255 special case).

443 ↗(On Diff #5222)

Agreed. It has an unused error variable that should also be removed. I would also just merge
init_mem_locality into the parse_slit() routine instead of doing it separately. so that the entire routine is just:

if (parse_srat() == 0)
    parse_slit();
446–447 ↗(On Diff #5222)

Agreed, just needs the blank line removed.

rpaulo added inline comments.May 6 2015, 11:22 PM
sys/x86/acpica/srat.c
404 ↗(On Diff #5222)

Those have been bugs and need to be fixed. It's definitely not more consistent:

rpaulo@akita freebsd/sys/dev % grep -r  acpi_disabled acpi*  | wc
   35     155    2120
rpaulo@akita freebsd/sys/dev % grep -r  resource_disabled acpi*  | wc
    5      21     299
adrian updated this revision to Diff 5246.May 7 2015, 2:38 AM
adrian edited edge metadata.

Include some more suggestions from people.

This revision now requires review to proceed.May 7 2015, 2:38 AM
adrian updated this revision to Diff 5247.May 7 2015, 2:40 AM
adrian edited edge metadata.

Another update, from jhb.

adrian updated this revision to Diff 5248.May 7 2015, 2:42 AM

Another update.

adrian updated this revision to Diff 5249.May 7 2015, 2:44 AM

REmove unneeded NULL, from jhb.

jhb added inline comments.May 7 2015, 2:20 PM
sys/x86/acpica/srat.c
404 ↗(On Diff #5249)

That doesn't change that hint.srat.0.disable already exists in 9.x and 10.x. To change now is POLA. I probably chose the non-ACPI route since it is more like the rest of the system, e.g. hint.apic.0.disabled to disable APIC (since that is not ACPI specific). If you want to add acpi_disabled for SRAT and disable SRAT if either is true but only use acpi_disabled for SLIT that is ok I guess.

jhb added a comment.May 7 2015, 7:34 PM

The comment in init_mem_locality() can be removed as it is no longer accurate, but I also still think that those two lines should just be moved into slit_parse_table() directly (which should return void) and init_mem_locality() should be removed.

Aside from a few style nits (extra blank lines and braces), the only other thing is that Rui would probably rather you use acpi_disabled() for SLIT. We can later add acpi_disabled() for SRAT so that SRAT can be disabled via both methods, but that doesn't need to be done in this change.

This revision was automatically updated to reflect the committed changes.
avg added a subscriber: avg.May 8 2015, 9:50 AM
avg added inline comments.
head/sys/vm/vm_phys.h
64

defining a variable in a header file seems like a bad idea?