Re: [PATCH] x86/tsc: Extend the watchdog check exemption to 4S/8S machine

[Feng Tang <feng.tang@intel.com>]

On Tue, Oct 11, 2022 at 09:09:12AM +0800, Feng Tang wrote:
> On Mon, Oct 10, 2022 at 07:23:10AM -0700, Dave Hansen wrote:
> > On 10/9/22 18:23, Feng Tang wrote:
> > >>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > >>> index cafacb2e58cc..b4ea79cb1d1a 100644
> > >>> --- a/arch/x86/kernel/tsc.c
> > >>> +++ b/arch/x86/kernel/tsc.c
> > >>> @@ -1217,7 +1217,7 @@ static void __init check_system_tsc_reliable(void)
> > >>>  	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 <= 2)
> > >>> +	    nr_online_nodes <= 8)
> > >> So you're saying all 8 socket systems since Broadwell (?) are TSC
> > >> sync'ed ?
> > > No, I didn't mean that. I haven't got chance to any 8 sockets
> > > machine, and I got a report last month that on one 8S machine,
> > > the TSC was judged 'unstable' by HPET as watchdog.
> > 
> > That's not a great check.  Think about numa=fake=4U, for instance.  Or a
> > single-socket system with persistent memory and high bandwidth memory.
> > 
> > Basically 'nr_online_nodes' is a software construct.  It's going to be
> > really hard to infer anything from it about what the _hardware_ is.
> 
> You are right! How to get the socket number was indeed a trouble when
> I worked on commit b50db7095fe0, the problem is related to the
> initialization order. This tsc check needs to be done in tsc_init(),
> while the node_stats[] get initialized in later's call of smp_init().
> 
> For the case you mentioned above, I dug out some old logs which showed
> its init order:
> 
>   numa=fake=4 on a SKL desktop
>   ================
>   [    0.000066] [tsc_early_init()]: nr_online_nodes = 1
>   [    0.000068] [tsc_early_init()]: nr_cpu_nodes = 0
>   [    0.000070] [tsc_early_init()]: nr_mem_nodes = 0
>   [    0.104015] [tsc_init()]: nr_online_nodes = 4
>   [    0.104019] [tsc_init()]: nr_cpu_nodes = 0
>   [    0.104022] [tsc_init()]: nr_mem_nodes = 4
>   [    0.124778] smp: Brought up 4 nodes, 4 CPUs
>   [    0.760915] [init_tsc_clocksource()]: nr_online_nodes = 4
>   [    0.760919] [init_tsc_clocksource()]: nr_cpu_nodes = 4
>   [    0.760922] [init_tsc_clocksource()]: nr_mem_nodes = 4
>   
>   QEMU with 2 CPU-DRAM nodes + 2 Persistent memory nodes 
>   ========================================================
>   [    0.066651] [tsc_early_init()]: nr_online_nodes = 1
>   [    0.067494] [tsc_early_init()]: nr_cpu_nodes = 0
>   [    0.068288] [tsc_early_init()]: nr_mem_nodes = 0
>   [    0.677694] [tsc_init()]: nr_online_nodes = 4
>   [    0.678862] [tsc_init()]: nr_cpu_nodes = 0
>   [    0.679962] [tsc_init()]: nr_mem_nodes = 4
>   [    1.139240] [init_tsc_clocksource()]: nr_online_nodes = 4
>   [    1.140576] [init_tsc_clocksource()]: nr_cpu_nodes = 2
>   [    1.141823] [init_tsc_clocksource()]: nr_mem_nodes = 4
>   [    1.660100] [kernel_init()]: nr_online_nodes = 4
>   [    1.661234] [kernel_init()]: nr_cpu_nodes = 2
>   [    1.662300] [kernel_init()]: nr_mem_nodes = 4
> 
> The 'nr_online_nodes' was chosed in the hope of that, in worse case
> the patch is just a nop and won't wrongly lift the check.
> 
> One possible solution for this problem is to leverage the SRAT table
> early init which is called before tsc_init(), and can provide CPU
> nodes info. Will try this way.

Th simple patch below is to have a dedicate CPU nodemask and set it in
early SRAT CPU parsing, still it has problem when sub-numa is enabled
in BIOS where there are more NUMA nodes in SRAT table. (also I'm
not sure the change to amdtopology.c is right)

Thanks,
Feng

diff --git a/arch/x86/include/asm/numa.h b/arch/x86/include/asm/numa.h
index e3bae2b60a0d..e745053a5f9a 100644
--- a/arch/x86/include/asm/numa.h
+++ b/arch/x86/include/asm/numa.h
@@ -31,6 +31,7 @@ extern int numa_off;
  */
 extern s16 __apicid_to_node[MAX_LOCAL_APIC];
 extern nodemask_t numa_nodes_parsed __initdata;
+extern nodemask_t numa_cpu_nodes __initdata;
 
 extern int __init numa_add_memblk(int nodeid, u64 start, u64 end);
 extern void __init numa_set_distance(int from, int to, int distance);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 179e0b1ba5cc..a2a7fc5aa15c 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -29,6 +29,7 @@
 #include <asm/intel-family.h>
 #include <asm/i8259.h>
 #include <asm/uv/uv.h>
+#include <asm/numa.h>
 
 unsigned int __read_mostly cpu_khz;	/* TSC clocks / usec, not used here */
 EXPORT_SYMBOL(cpu_khz);
@@ -1218,7 +1219,7 @@ first_dump();
 	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 <= 2)
+	    nodes_weight(numa_cpu_nodes) <= 2)
 		tsc_disable_clocksource_watchdog();
 }
 
diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
index b3ca7d23e4b0..6b982a16cc38 100644
--- a/arch/x86/mm/amdtopology.c
+++ b/arch/x86/mm/amdtopology.c
@@ -152,6 +152,7 @@ int __init amd_numa_init(void)
 		prevbase = base;
 		numa_add_memblk(nodeid, base, limit);
 		node_set(nodeid, numa_nodes_parsed);
+		node_set(nodeid, numa_cpu_nodes);
 	}
 
 	if (nodes_empty(numa_nodes_parsed))
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index 090125b3ee1f..82798fee97a2 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -21,6 +21,7 @@
 
 int numa_off;
 nodemask_t numa_nodes_parsed __initdata;
+nodemask_t numa_cpu_nodes __initdata;
 
 struct pglist_data *node_data[MAX_NUMNODES] __read_mostly;
 EXPORT_SYMBOL(node_data);
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 7688117ac2f4..11b08b317306 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -59,6 +59,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
+	node_set(node, numa_cpu_nodes);
 
 	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u\n",
@@ -106,6 +107,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 
 	set_apicid_to_node(apic_id, node);
 	node_set(node, numa_nodes_parsed);
+	node_set(node, numa_cpu_nodes);
 
 	printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u\n",