Re: [RFC PATCH v2 0/8] timekeeping: Fix draft tracking precision and add feed-forward discipline via vmclock

[Thomas Gleixner <tglx@kernel.org>]

On Sun, May 24 2026 at 17:15, Thomas Gleixner wrote:
> On Fri, May 22 2026 at 17:50, David Woodhouse wrote:
>> On Fri, 2026-05-22 at 17:28 +0200, Thomas Gleixner wrote:
>>> 
>>>   git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git timers/ptp/timekeeping
>>
>> In 94dd85a8d0a ("timekeeping: Add system_counterval_t to struct
>> system_device_crosststamp") my version ditched the system_counterval_t
>> on the stack and just used the one in xtstamp directly.
>
> Which is wrong. I did it the way I did for a very good reason.
>
>> The convert_base_to_cs() function probably wants to scv->id=cs->id for
>> itself anyway; otherwise it's leaving behind an inconsistent
>> system_counterval_t object which... will lead to exactly the bug my
>> first version of that had, that I see you avoided :)
>
> No. It can't because that would corrupt the object for the retry case,
> which would then hand back the wrong value.
>
> The object _IS_ consistent because the csid in there is related to the
> PTM value and not to the clocksource. The function updates the @cycles
> value and leaves everything else untouched. The clock ID for the @cyles
> value is guaranteed to be the clock ID of the system clocksource, so
> using this is the right thing to do.
>
> Just because it looks tempting or your AI buddy told you so doesn't make
> it correct.

And it's worse. We both are wrong :)

There is an existing bug in that code for the retry case. Fix below.

Thanks,

        tglx
---
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1343,12 +1343,14 @@ static bool convert_clock(u64 *val, u32
 	return true;
 }
 
-static bool convert_base_to_cs(struct system_counterval_t *scv)
+static bool convert_base_to_cs(struct system_counterval_t *scv, u64 *cycles)
 {
 	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
 	struct clocksource_base *base;
 	u32 num, den;
 
+	*cycles = scv->cycles;
+
 	/* The timestamp was taken from the time keeper clock source */
 	if (cs->id == scv->cs_id)
 		return true;
@@ -1364,10 +1366,10 @@ static bool convert_base_to_cs(struct sy
 	num = scv->use_nsecs ? cs->freq_khz : base->numerator;
 	den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
 
-	if (!convert_clock(&scv->cycles, num, den))
+	if (!convert_clock(cycles, num, den))
 		return false;
 
-	scv->cycles += base->offset;
+	*cycles += base->offset;
 	return true;
 }
 
@@ -1479,9 +1481,8 @@ int get_device_system_crosststamp(int (*
 		 * installed timekeeper clocksource
 		 */
 		if (system_counterval.cs_id == CSID_GENERIC ||
-		    !convert_base_to_cs(&system_counterval))
+		    !convert_base_to_cs(&system_counterval, &cycles))
 			return -ENODEV;
-		cycles = system_counterval.cycles;
 
 		/*
 		 * Check whether the system counter value provided by the