Re: [patch 01/24] timekeeping: Provide ktime_get_snapshot_id()

[Thomas Weißschuh]

On Tue, May 26, 2026 at 07:13:33PM +0200, Thomas Gleixner wrote:
> ktime_get_snapshot() provides a snapshot of the underlying clocksource
> counter value and the corresponding CLOCK_MONOTONIC_RAW, CLOCK_REALTIME and
> CLOCK_BOOTTIME timestamps.
> 
> There is no usage of CLOCK_REALTIME and CLOCK_BOOTTIME at the same time and
> CLOCK_BOOTTIME support was just added for the ARM64 KVM tracing mechanism,
> which needs CLOCK_BOOTTIME and the underlying clocksource counter value.
> 
> ktime_get_snapshot() is also not suitable for usage with CLOCK_AUX, but
> that's a prerequisite to support PTP hardware timestamping for CLOCK_AUX
> steering.
> 
> As a first step, rename ktime_get_snapshot() to ktime_get_snapshot_id(),
> which now takes a clockid argument to select the clock which needs to be
> captured. The result is stored in system_time_snapshot::sys, which will
> replace the system_time_snapshot::real/boot members once all usage sites
> have been converted.
> 
> ktime_get_snapshot() is a simple wrapper which hands in CLOCK_REALTIME as
> clockid argument for the conversion period. That means CLOCK_REALTIME is
> now captured twice, but that redunancy is only temporary.
> 
> No functional change vs. current users of ktime_get_snapshot()
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxx>

Reviewed-by: Thomas Weißschuh <thomas.weissschuh@xxxxxxxxxxxxx>

Plus some subjective nitpicks below.

> ---
>  include/linux/timekeeping.h |   29 ++++++++++-----
>  kernel/time/timekeeping.c   |   84 +++++++++++++++++++++++++++++++++-----------
>  2 files changed, 84 insertions(+), 29 deletions(-)
> 
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -276,24 +276,28 @@ static inline bool ktime_get_aux_ts64(cl
>  #endif
>  
>  /**
> - * struct system_time_snapshot - simultaneous raw/real time capture with
> - *				 counter value
> - * @cycles:	Clocksource counter value to produce the system times
> - * @real:	Realtime system time
> - * @boot:	Boot time
> - * @raw:	Monotonic raw system time
> - * @cs_id:	Clocksource ID
> + * struct system_time_snapshot - Simultaneous time capture of CLOCK_MONOTONIC_RAW,
> + *				 a selected CLOCK_* and the clocksource counter value
> + * @cycles:		Clocksource counter value to produce the system times
> + * @sys:		The system time of the selected CLOCK ID
> + * @real:		Realtime system time
> + * @boot:		Boot time
> + * @raw:		Monotonic raw system time
> + * @cs_id:		Clocksource ID
>   * @clock_was_set_seq:	The sequence number of clock-was-set events
>   * @cs_was_changed_seq:	The sequence number of clocksource change events
> + * @valid:		True if the snapshot is valid
>   */
>  struct system_time_snapshot {
>  	u64			cycles;
> +	ktime_t			sys;
>  	ktime_t			real;
>  	ktime_t			boot;
>  	ktime_t			raw;
>  	enum clocksource_ids	cs_id;
>  	unsigned int		clock_was_set_seq;
>  	u8			cs_was_changed_seq;
> +	u8			valid;
>  };
>  
>  /**
> @@ -341,9 +345,16 @@ extern int get_device_system_crosststamp
>  			struct system_device_crosststamp *xtstamp);
>  
>  /*
> - * Simultaneously snapshot realtime and monotonic raw clocks
> + * Simultaneously snapshot a given clock with MONOTONIC_RAW and the underlying
> + * clocksource counter value.
>   */
> -extern void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot);
> +extern bool ktime_get_snapshot_id(struct system_time_snapshot *systime_snapshot,
> +				  clockid_t clock_id);

None of the callers (except the wrapper below, which gets removed later) is
checking the return value. Having both the return value and the valid member
looks a bit weird, too.

Clockid parameter first for consistency?

> +
> +static inline void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
> +{
> +	WARN_ON_ONCE(!ktime_get_snapshot_id(systime_snapshot, CLOCK_REALTIME));
> +}
>  
>  /*
>   * Persistent clock related interfaces
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1183,43 +1183,87 @@ noinstr time64_t __ktime_get_real_second
>  }
>  
>  /**
> - * ktime_get_snapshot - snapshots the realtime/monotonic raw clocks with counter
> - * @systime_snapshot:	pointer to struct receiving the system time snapshot
> + * ktime_get_snapshot_id -  Simultaneously snapshot a given clock ID with
> + *			    CLOCK_MONOTONIC_RAW and the underlying
> + *			    clocksource counter value.
> + * @systime_snapshot:	Pointer to struct receiving the system time snapshot
> + * @clock_id:		The clock ID to snapshot
>   */
> -void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
> +bool ktime_get_snapshot_id(struct system_time_snapshot *systime_snapshot, clockid_t clock_id)
>  {
> -	struct timekeeper *tk = &tk_core.timekeeper;
> +	ktime_t base_raw, base_sys, offs_sys, *offs, offs_zero = 0;
> +	u64 nsec_raw, nsec_sys, now;
> +	struct timekeeper *tk;
> +	struct tk_data *tkd;

This indirection only makes sense with the support for AUX clocks added later.

>  	unsigned int seq;
> -	ktime_t base_raw;
>  	ktime_t base_real;
>  	ktime_t base_boot;

The ktime_t variable declarations are weird now.

> -	u64 nsec_raw;
> -	u64 nsec_real;
> -	u64 now;
>  
> -	WARN_ON_ONCE(timekeeping_suspended);
> +	/* Invalidate the snapshot for all failure cases */
> +	systime_snapshot->valid = false;
> +
> +	if (WARN_ON_ONCE(timekeeping_suspended))
> +		return false;
> +
> +	switch (clock_id) {
> +	case CLOCK_REALTIME:
> +		tkd = &tk_core;
> +		offs = &tk_core.timekeeper.offs_real;
> +		break;
> +	/* Map RAW to MONOTONIC so the loop below is trivial */
> +	case CLOCK_MONOTONIC_RAW:
> +	case CLOCK_MONOTONIC:
> +		tkd = &tk_core;
> +		offs = &offs_zero;
> +		break;
> +	case CLOCK_BOOTTIME:
> +		tkd = &tk_core;
> +		offs = &tk_core.timekeeper.offs_boot;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return false;
> +	}
> +
> +	tk = &tkd->timekeeper;
>  
>  	do {
> -		seq = read_seqcount_begin(&tk_core.seq);
> +		seq = read_seqcount_begin(&tkd->seq);
> +
>  		now = tk_clock_read(&tk->tkr_mono);
>  		systime_snapshot->cs_id = tk->tkr_mono.clock->id;
>  		systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq;
>  		systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq;
> -		base_real = ktime_add(tk->tkr_mono.base,
> -				      tk_core.timekeeper.offs_real);
> -		base_boot = ktime_add(tk->tkr_mono.base,
> -				      tk_core.timekeeper.offs_boot);
> +
> +		base_sys = tk->tkr_mono.base;
> +		offs_sys = *offs;
>  		base_raw = tk->tkr_raw.base;
> -		nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
> -		nsec_raw  = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
> -	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +		/* Kept around until the callers are fixed up */
> +		base_real = ktime_add(base_sys, tk_core.timekeeper.offs_real);
> +		base_boot = ktime_add(base_sys, tk_core.timekeeper.offs_boot);
> +
> +		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
> +		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
> +	} while (read_seqcount_retry(&tkd->seq, seq));
>  
>  	systime_snapshot->cycles = now;
> -	systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
> -	systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real);
> +	systime_snapshot->sys = ktime_add_ns(base_sys, offs_sys + nsec_sys);

Technically offs_sys is ktime_t and not u64, so would need ktime_add().
(Not that it makes a difference)

> +	systime_snapshot->real = ktime_add_ns(base_real, nsec_sys);
> +	systime_snapshot->boot = ktime_add_ns(base_boot, nsec_sys);
>  	systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
> +
> +	/*
> +	 * Special case for PTP. Just transfer the raw time into sys,
> +	 * so the call sites can consistently use snap::sys.
> +	 */
> +	if (clock_id == CLOCK_MONOTONIC_RAW)
> +		systime_snapshot->sys = systime_snapshot->raw;
> +	/* Tell the consumer that this snapshot is valid */
> +	systime_snapshot->valid = true;
> +	return true;
>  }
> -EXPORT_SYMBOL_GPL(ktime_get_snapshot);
> +EXPORT_SYMBOL_GPL(ktime_get_snapshot_id);
>  
>  /* Scale base by mult/div checking for overflow */
>  static int scale64_check_overflow(u64 mult, u64 div, u64 *base)
>