Re: [PATCH] x86/tsc: Fix misplaced seqcount_latch_init() in cyc2ns_init_secondary_cpus()

On 8/4/26 3:22 PM, Peter Zijlstra wrote:
> On Tue, Aug 04, 2026 at 03:11:28PM +0800, Bo Li wrote:
>> In cyc2ns_init_secondary_cpus(), seqcount_latch_init(&c2n->seq) is
>> called _before_ c2n is advanced to the next CPU via per_cpu_ptr().
>> As a result:
>>
>>   1. On the first iteration, c2n still points at the BSP's struct, so
>>      the BSP's seqcount_latch is re-initialized. cyc2ns_init_boot_cpu()
>>      already did this correctly, so it happens to work by accident
>>      because no concurrent readers are live at __init time.
>>
>>   2. On subsequent iterations, seqcount_latch_init() initializes the
>>      previous CPU's seqcount (because c2n was advanced by the prior
>>      per_cpu_ptr()), so all CPUs except the last one in the for_each
>>      loop happen to get initialized. The last secondary CPU's seqcount
>>      is left uninitialized.
>>
>> Move seqcount_latch_init() _after_ c2n is pointed at the target CPU's
>> struct, so each secondary CPU's seqcount is correctly initialized.
>>
>> Fixes: e2a9ca29b5ed ("x86/tsc: Initialize cyc2ns when tsc frequency is determined")
>> Cc: [email protected]
>> Signed-off-by: Bo Li <[email protected]>
>> ---
>>   arch/x86/kernel/tsc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index ce10ae4b298b..84fb80492b01 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -223,8 +223,8 @@ static void __init cyc2ns_init_secondary_cpus(void)
>>   
>>   	for_each_possible_cpu(cpu) {
>>   		if (cpu != this_cpu) {
>> -			seqcount_latch_init(&c2n->seq);
>>   			c2n = per_cpu_ptr(&cyc2ns, cpu);
>> +			seqcount_latch_init(&c2n->seq);
>>   			c2n->data[0] = data[0];
>>   			c2n->data[1] = data[1];
>>   		}
> 
> No, this looks wrong. Note that per the 'cpu != this_cpu' there are only
> N-1 invocations. One CPU will not be initialized. Further note the name
> of this function, and the name of the function above it.
> 

Agreed - the loop is N-1 and deliberately skips this_cpu, which is
initialized by cyc2ns_init_boot_cpu(). That CPU is not my concern.
Sorry, my changelog was unclear. The issue is an off-by-one in the
existing code: seqcount_latch_init(&c2n->seq) runs before c2n is
advanced to the target CPU, so each iteration initializes the seqcount
of the previous c2n rather than the current one:

	c2n = this_cpu_ptr(&cyc2ns);		/* c2n -> boot cpu */
	for_each_possible_cpu(cpu) {
		if (cpu != this_cpu) {
			seqcount_latch_init(&c2n->seq);   /* inits the previous c2n */
			c2n = per_cpu_ptr(&cyc2ns, cpu);  /* only now advanced */
			...
		}
	}

Concretely, with this_cpu=0 and possible cpus {0,1,2,3}:
	cpu=1: inits cpu0 (boot, already done by cyc2ns_init_boot_cpu())
	cpu=2: inits cpu1
	cpu=3: inits cpu2
	=> cpu3, the last secondary, is never initialized, while boot is
	   redundantly re-initialized.

Moving seqcount_latch_init() after per_cpu_ptr() makes each secondary
initialize its own seqcount.