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

[adrian]

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.

[cse_cem_gmail_com]

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

style

[rpaulo]

Only minor comments.

sys/vm/vm_phys.c

320

Extra ;

sys/x86/acpica/srat.c

131

Needs to be removed.

133

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

134

Usually we don't print messages here.

149

This looks bogus...

153

error is always 0.

160

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

397

acpi_disabled() again.

434

Unnecessary parentheses.

[adrian]

Address some of the concerns from rui.

[ngie]

sys/vm/vm_phys.c

144–147

Sort the sysctl?

318

Why have a hardcoded value of 255?

341–343

style(9): remove braces

sys/x86/acpica/srat.c

425–426

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

430–433

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

445

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

[stas]

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

[jhb]

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

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

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

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

sys/x86/acpica/srat.c

77

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

82

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.

98

Trim extra newline.

138

You can drop these extra braces.

153

Yes, error is unused and should be removed.

406

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

425–426

Agreed, just needs the blank line removed.

430–433

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

445

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

[rpaulo]

sys/x86/acpica/srat.c

406

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]

Include some more suggestions from people.

[adrian]

Another update, from jhb.

[adrian]

Another update.

[adrian]

REmove unneeded NULL, from jhb.

[jhb]

sys/x86/acpica/srat.c

404

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]

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.

[avg]

head/sys/vm/vm_phys.h

64 ↗

(On Diff #5269)

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