Re: [PATCH net-next v6 3/3] gve: implement PTP gettimex64

[Jacob Keller]

On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> From: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> 
> Enable chrony and phc2sys to synchronize system clock to NIC clock.
> 
> Two paths are implemented: a precise path using system counter values
> sampled by the device, and a fallback path using system counter values
> sampled in the driver using ptp_read_system_prets()/postts().
> 
> To use the precise path, the current system clocksource must match the
> units returned by the device, which on x86 is X86_TSC and on ARM64 is
> ARM_ARCH_COUNTER. The clockid requested for the cross-timestamp must
> be either CLOCK_REALTIME or CLOCK_MONOTONIC_RAW. These conditions hold
> by default on GCP VMs using Chrony, so we expect the precise path to be
> used the vast majority of the time. If the system clocksource is changed
> to kvm-clock, it activates the fallback path. Ethtool counters have been
> added to count how many times each path is used.
> 
> The uncertainty window in the precise path is typically around 1-2us,
> while in the fallback path is around 60-80us.
> 
> Stub implementions of adjfine and adjtime are added to avoid NULL
> dereference when phc2sys tries to adjust the clock.
> 
> Cc: John Stultz <jstultz@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> Reviewed-by: Kevin Yang <yyd@xxxxxxxxxx>
> Reviewed-by: Naman Gulati <namangulati@xxxxxxxxxx>
> Signed-off-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> ---
> Changes in v6:
> - Added a fallback to driver-sampled time sandwich that is used when
>   the following conditions are not met:
>   - The system clock source is X86_TSC or ARM_ARCH_COUNTER
>   - The requested clockid is CLOCK_REALTIME or CLOCK_MONOTONIC_RAW
>   - The architecture is x86 or ARM64
> - Added ethtool statistics to count how many cross-timestamps used the
>   precise path versus fallback path.
> - Fixed printf format specifier.
> - Added stub implementions of adjtime and adjfine to prevent NULL
>   dereference when phc2sys tries to adjust clock.

All excellent improvements.

> - Moved system time snapshot back to gve_ptp_gettimex64() so we can get the
>   current system clock source from it. It is OK for it to not be inside
>   the mutex or retry loop because lock contention and retries should be
>   extremely rare, and chrony filters out bad samples.
> 

I'm a bit worried about this part, but if I understand, it would only
affect the fallback variant anyways since the clock samples are taken in
the host in the precise/fast path.

> Changes in v5:
> - Reformulate retry loop in terms of total timeout (Jakub Kicinski)
> 
> Changes in v3:
> - Take system time snapshot inside the mutex
> - Return -EOPNOTSUPP if cross-timestamp is requested on an arch other
>   than x86 or arm64
> 
> Changes in v2:
>  - fix compilation warning on ARM by casting cycles_t to u64
> ---
>  drivers/net/ethernet/google/gve/gve.h         |   8 +
>  drivers/net/ethernet/google/gve/gve_adminq.h  |   4 +-
>  drivers/net/ethernet/google/gve/gve_ethtool.c |   3 +
>  drivers/net/ethernet/google/gve/gve_ptp.c     | 237 +++++++++++++++++-
>  4 files changed, 243 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 7b69d0cfc0d5..4de3ce60060e 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -880,6 +880,14 @@ struct gve_priv {
>  	u32 stats_report_trigger_cnt; /* count of device-requested stats-reports since last reset */
>  	u32 suspend_cnt; /* count of times suspended */
>  	u32 resume_cnt; /* count of times resumed */
> +	/* count of cross-timestamps attempted using system timestamps
> +	 * from the AQ command
> +	 */
> +	u32 ptp_precise_xtstamps;
> +	/* count of cross-timestamps attempted using system timestamps sampled
> +	 * by the driver
> +	 */

Helpful for analyzing when its not working. Nice!

> +static int gve_cycles_to_clock_fn(ktime_t *device_time,
> +				  struct system_counterval_t *system_counterval,
> +				  void *ctx)
> +{
> +	struct gve_cycles_to_clock_callback_ctx *context = ctx;
> +
> +	*device_time = 0;
> +
> +	system_counterval->cycles = context->cycles;
> +	system_counterval->use_nsecs = false;
> +
> +	if (IS_ENABLED(CONFIG_X86))
> +		system_counterval->cs_id = CSID_X86_TSC;
> +	else if (IS_ENABLED(CONFIG_ARM64))
> +		system_counterval->cs_id = CSID_ARM_ARCH_COUNTER;
> +	else

No single kernel can be compiled for both CONFIG_X86 *and* CONFIG_ARM64
simultaneously, so there is no need to check in sequence and an
exclusive if/else path is fine. Ok.

> +
> +static bool
> +gve_can_use_system_ts_from_device(enum clocksource_ids system_clock_source,
> +				  clockid_t clockid)
> +{
> +	if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC_RAW)
> +		return false;
> +
> +	/* If the system clock source matches the system clock
> +	 * returned by the AdminQ command, we can use the system
> +	 * timestamps returned by the device, otherwise we have to
> +	 * fall back to sampling system time from the host which
> +	 * is less accurate.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86))
> +		return system_clock_source == CSID_X86_TSC;
> +	else if (IS_ENABLED(CONFIG_ARM64))
> +		return system_clock_source == CSID_ARM_ARCH_COUNTER;
> +

This feels a bit duplicated to me, since you have the same check
elsewhere. Would it make sense to pull this to its own function check?
Its short enough I think that is only warranted if you have a true
(non-nit) cause to make a v7.

Thus:

Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>