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

[Dave Hansen <dave.hansen@intel.com>]

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.

> 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.

I also kinda dislike the comment talking about "sockets" and the code
talking about "packages".  I also did a big *gulp* when I saw this:

	#define topology_max_packages() (__max_logical_packages)

and:

>         /*
>          * 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.