Re: [PATCH] x86/tsc: Use topology_max_packages() to get package number

[Feng Tang]

On Fri, Mar 15, 2024 at 10:58:38AM -0700, Dave Hansen wrote:
> On 3/15/24 04:26, Feng Tang wrote:
> > Thomas' recent patchset of refactoring x86 topology code introduces
> > topology_max_package(), which works well in most of the above cases.
> > The only exceptions are 'nr_cpus=' and 'possible_cpus=' setup, which
> > sets up the 'nr_cpu_ids' and rejects the rest of the CPUs, and may
> > cause topology_max_package() less than the real package number, but
> > it's fine as it is rarely used debug option, and logical package
> > number really matters in this check. So use the more accurate
> > topology_max_package() to replace nr_online_nodes().
> 
> In the end, we have a bunch of hardware enumeration and then a bunch of
> processing on top of it taking CPU hotplug support and kernel command
> lines into account.
> 
> The hardware enumeration is relatively simple.  The processing the
> kernel does on top of it is complicated.

Yes!

> 
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 5a69a49acc96..87e7c0e89db1 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1252,15 +1252,12 @@ static void __init check_system_tsc_reliable(void)
> >  	 *  - TSC which does not stop in C-States
> >  	 *  - the TSC_ADJUST register which allows to detect even minimal
> >  	 *    modifications
> > -	 *  - not more than two sockets. As the number of sockets cannot be
> > -	 *    evaluated at the early boot stage where this has to be
> > -	 *    invoked, check the number of online memory nodes as a
> > -	 *    fallback solution which is an reasonable estimate.
> > +	 *  - not more than four sockets.
> >  	 */
> >  	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> >  	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> >  	    boot_cpu_has(X86_FEATURE_TSC_ADJUST) &&
> > -	    nr_online_nodes <= 4)
> > +	    topology_max_packages() <= 4)
> >  		tsc_disable_clocksource_watchdog();
> >  }
> 
> I know there's some history here, but the changelog itself is not clear
> about what the problem is or how the patch solves it.

OK, will improve the changelog. The problem is nr_online_nodes() is
not a good option for get package number, it is mostly a memory node
concept, and easy be cheated by different kernel cmdline setup like
NUMA emulation and hotplug tricks, while it had an advantage of being
availab early before TSC get initialized. Thomas' patchset improves
the topology code, and provide a much better topology_max_packages().

> 
> I also kinda dislike the comment talking about "sockets" and the code
> talking about "packages".

Will unifiy to use 'package' term.

> I also did a big *gulp* when I saw this:
> 
> 	#define topology_max_packages() (__max_logical_packages)
> 
> and:

Latest code has dropped this.

Thanks,
Feng

> >         /*
> >          * Today neither Intel nor AMD support heterogeneous systems so
> >          * extrapolate the boot cpu's data to all packages.
> >          */
> >         ncpus = cpu_data(0).booted_cores * topology_max_smt_threads();
> >         __max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
> 
> Because Intel obviously has heterogeneous systems today.
> 
> So I'll buy that removing 'nr_online_nodes' takes NUMA out of the
> picture (which is good), but I want to hear more about why
> topology_max_packages() and '4' are the right things to be checking.
> 
> I suspect the real reason '4' was picked was to give the calculation
> some wiggle room because it's not actually all that precise.
> 
>