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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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 |
Comment Actions
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. |
| 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? |
Comment Actions
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 if (parse_srat() == 0)
parse_slit(); |
| 446–447 ↗ | (On Diff #5222) | Agreed, just needs the blank line removed. |
| 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 |
| 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. |
Comment Actions
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.
| head/sys/vm/vm_phys.h | ||
|---|---|---|
| 64 | defining a variable in a header file seems like a bad idea? | |