Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Christian Walter, Angel Pons, Arthur Heymans, Shuming Chu (Shuming), TangYiwei.
5 comments:
Patchset:
Thanks for reviewing! I think MADT code actually needs something similar. I also want to put the topology code inside common mp_init.c. It's common accros x86 CPUs.
File src/soc/intel/xeon_sp/cpx/cpu.c:
Patch Set #3, Line 81: apic_id
Should be package_id
File src/soc/intel/xeon_sp/nb_acpi.c:
/* First all regular cpus, then all the threads */
for (cpu = all_devices; cpu; cpu = cpu->next) {
if (!is_enabled_cpu(cpu))
continue;
if (num_cpus >= ARRAY_SIZE(apic_ids))
break;
/* Is this a thread? */
if (cpu->path.apic.apic_id & 1)
continue;
apic_ids[num_cpus++] = cpu->path.apic.apic_id;
}
const uint32_t num_of_cores = num_cpus;
if (num_cpus > 1)
bubblesort(&apic_ids[0], num_of_cores, NUM_ASCENDING);
Maybe rewrite this as a loop?
```
const uint32_t max_th_per_core = 2;
uint32_t cpus_so_far = 0;
for (uint32_t th_in_core = 0; th_in_core < max_th_per_core; th_in_core++) {
for (cpu = all_devices; cpu; cpu = cpu->next) {
if (!is_enabled_cpu(cpu))
continue;
if (num_cpus >= ARRAY_SIZE(apic_ids))
break;
/* Is this a thread? */
if (cpu->path.apic.apic_id % max_th_per_core != th_in_core)
continue;
apic_ids[num_cpus++] = cpu->path.apic.apic_id;
}
bubblesort(&apic_ids[cpus_so_far], num_cpus - cpus_so_far, NUM_ASCENDING);
cpus_so_far = num_cpus;
}
```Yes, no idea if we'll ever have to deal with more than 2 threads per core. The compiler should be able to do loop unrolling if it thinks it's worth it.
I want to put that information in struct path. It already has a tread_id entry. it can loop over thread_id. If no thread_id CPU is found a loop can break. That would allow for more than 2 threads per core_id.
Patch Set #3, Line 54: if (num_cpus > 1)
What's the purpose of this check? Did you mean to check if `num_cpus - num_of_cores > 1` instead? You don't need any checks, `bubblesort()` bails out early if there's less than 2 elements to sort.
copy pasta from MADT code.
for (cpu = all_devices; cpu; cpu = cpu->next) {
if (!is_enabled_cpu(cpu))
continue;
if (cpu->path.apic.apic_id == apic_ids[i])
break;
}
Hmmm, having to re-do this search is a bit inefficient... The problem seems to be that the `bubblesort()` function doesn't allow sorting `struct device *` according to the value of some member. Making a generic sort function in C would involve void pointers and function pointers:
```
typedef void (*comparator_func_t)(void *, void *);
void generic_bubblesort(void *array[], size_t len, comparator_func_t fn);
```
Using array of struct *path or struct *device and a comparator is indeed the way to go :-)
To view, visit change 68912. To unsubscribe, or for help writing mail filters, visit settings.