Messages in this thread | | | Date | Fri, 4 Aug 2023 14:04:46 +0530 | Subject | Re: [patch V3 23/40] x86/cpu: Provide cpu_init/parse_topology() | From | K Prateek Nayak <> |
| |
Hello Thomas,
On 8/4/2023 1:58 PM, Thomas Gleixner wrote: > On Fri, Aug 04 2023 at 13:44, K Prateek Nayak wrote: >> On 8/2/2023 3:51 PM, Thomas Gleixner wrote: >>> >>> [..snip..] >>> >>> +static void topo_set_max_cores(struct topo_scan *tscan) >>> +{ >>> + /* >>> + * Bug compatible for now. This is broken on hybrid systems: >>> + * 8 cores SMT + 8 cores w/o SMT >>> + * tscan.dom_ncpus[TOPO_CORE_DOMAIN] = 24; 24 / 2 = 12 !! >>> + * >>> + * Cannot be fixed without further topology enumeration changes. >>> + */ >>> + tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_CORE_DOMAIN] >> >>> + x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN]; >>> +} >>> >> >> In Documentation/arch/x86/topology.rst, "cpuinfo_x86.x86_max_cores" is >> described as "The number of cores in a package". In which case, >> shouldn't the above be: >> >> tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_PKG_DOMAIN] >> >> x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN]; >> >> since, with extended topology, there could be other higher domains and >> dom_ncpus[TOPO_CORE_DOMAIN] >> dom_shifts[TOPO_SMT_DOMAIN] should only >> give number of cores within the next domain (TOPO_MODULE_DOMAIN). > > You're right in principle. > >> Am I missing something here? > > The fact, that this is bug compatible. It's broken in several > aspects. The real fix is in the next series, where this function goes > away and actually uses real topology data to compute this. > > I could change this to be more "accurate" as you suggested, but that's > not making much of a difference.
Ah! I see. Thank you for clarifying. I'll keep an eye out for the next series.
-- Thanks and Regards, Prateek
| |