Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Shuming Chu (Shuming), TangYiwei.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68912 )
Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa ......................................................................
Patch Set 3:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68912/comment/dc0ed2e8_de1a685f PS3, Line 9: reassign reassign*s*
https://review.coreboot.org/c/coreboot/+/68912/comment/ac74b3c2_926a7665 PS3, Line 12: break break*s*
https://review.coreboot.org/c/coreboot/+/68912/comment/950c665e_1333c69e PS3, Line 15: This moves the sorting to of the lapic_ids to the srat table generation
This moves the sorting to of --> This moves the sorting of ?
Yes, the `to` is spurious
File src/soc/intel/xeon_sp/nb_acpi.c:
https://review.coreboot.org/c/coreboot/+/68912/comment/97ee8c56_8e0acc46 PS3, Line 41: if (num_cpus > 1) We don't need these checks, bubblesort bails out early if there's less than 2 elements to sort.
https://review.coreboot.org/c/coreboot/+/68912/comment/370b5e29_41ef8f3c PS3, Line 29: /* 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.
https://review.coreboot.org/c/coreboot/+/68912/comment/f1523c9e_c7daa9d9 PS3, 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.
https://review.coreboot.org/c/coreboot/+/68912/comment/f20c49ef_7f60227e PS3, Line 59: 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); ```