Re: [PATCH 0/6] net/gve: add hardware timestamping support

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 11 May 2026 22:43:48 +0000
mark-blasko <blasko@google.com> wrote:

> From: Mark Blasko <blasko@google.com>
> 
> This patch series introduces support for GVE hardware timestamping on DQO
> queues. To support concurrent access, a mutex lock is introduced to protect
> admin queue operations. A mechanism is then added to periodically synchronize
> the NIC clock via AdminQ, and support is introduced for the read_clock ethdev
> operation. Finally, the RX datapath is updated to reconstruct full 64-bit
> timestamps from the 32-bit values in DQO descriptors.
> 
> Mark Blasko (6):
>   net/gve: add thread safety to admin queue
>   net/gve: add device option support for HW timestamps
>   net/gve: add AdminQ command for NIC timestamps
>   net/gve: add periodic NIC clock synchronization
>   net/gve: support read clock ethdev op
>   net/gve: reconstruct HW timestamps from DQO
> 
>  doc/guides/nics/features/gve.ini       |   1 +
>  doc/guides/nics/gve.rst                |  18 +++
>  doc/guides/rel_notes/release_26_07.rst |   3 +
>  drivers/net/gve/base/gve_adminq.c      | 127 +++++++++++++++++----
>  drivers/net/gve/base/gve_adminq.h      |  29 +++++
>  drivers/net/gve/base/gve_desc_dqo.h    |   8 +-
>  drivers/net/gve/gve_ethdev.c           | 148 ++++++++++++++++++++++++-
>  drivers/net/gve/gve_ethdev.h           |  39 +++++++
>  drivers/net/gve/gve_rx_dqo.c           |  26 +++++
>  9 files changed, 377 insertions(+), 22 deletions(-)
> 

Overall looks good, a little concerned about using ROBUST mutex.

AI review (with more opus 4.7)

Based on my review, here are the findings on this series. The patches mostly look reasonable but I have several concerns:

---

Some findings on this series.


[PATCH 1/6] net/gve: add thread safety to admin queue

The mutex is initialized with PTHREAD_MUTEX_ROBUST but pthread_mutex_lock
return values are never checked anywhere in the new code.

	pthread_mutexattr_setrobust(&mutexattr, PTHREAD_MUTEX_ROBUST);
	pthread_mutex_init(&priv->adminq_lock, &mutexattr);
	...
	pthread_mutex_lock(&priv->adminq_lock);

A robust mutex returns EOWNERDEAD on lock when the previous owner died
holding it, and transitions to permanent ENOTRECOVERABLE if
pthread_mutex_consistent() is not called before unlock. Either drop the
ROBUST attribute (matching the existing flow_rule_lock pattern in this
same driver, which uses only PROCESS_SHARED), or handle EOWNERDEAD and
ENOTRECOVERABLE explicitly.


[PATCH 3/6] net/gve: add AdminQ command for NIC timestamps

gve_adminq_alloc() explicitly zeroes every other adminq counter, but
the new adminq_report_nic_timestamp_cnt is not added to that list. The
counters get re-zeroed on every adminq_alloc, including from
gve_dev_reset() which calls gve_adminq_free() and then re-runs
gve_init_priv() -> gve_adminq_alloc(). After a device reset, every
counter except this new one will be back to zero, leaving inconsistent
stats. Add the assignment in gve_adminq_alloc().


[PATCH 4/6] net/gve: add periodic NIC clock synchronization

Unnecessary cast of void *:

	priv->nic_ts_report = (struct gve_nic_ts_report *)priv->nic_ts_report_mz->addr;

The same applies to the function-arg cast in gve_read_nic_clock:

	struct gve_priv *priv = (struct gve_priv *)arg;

Assignment from void * does not need a cast in C.

If the reschedule in gve_read_nic_clock fails:

	err = rte_eal_alarm_set(GVE_NIC_CLOCK_READ_PERIOD_MS * 1000,
				gve_read_nic_clock, priv);
	if (err < 0)
		PMD_DRV_LOG(ERR, "Failed to reschedule NIC clock read alarm, ret=%d", err);

no further callback will fire, no failures will accumulate, and
nic_ts_stale stays at whatever it was. If reschedule failure occurs
while stale is 0, the Rx datapath in 6/6 will keep attaching
reconstructed timestamps from an arbitrarily old base. Set nic_ts_stale
to 1 on reschedule failure so the Rx datapath stops trusting the cache.

gve_setup_nic_timestamp() discards the gve_alloc_nic_ts_report() error.
If the memzone allocation fails, priv->nic_timestamp_supported (set in
2/6) remains true, so dev_info_get() in 6/6 will advertise
RTE_ETH_RX_OFFLOAD_TIMESTAMP and read_clock() in 5/6 will return -EIO
on every call. Clear priv->nic_timestamp_supported on alloc failure so
the capability is advertised honestly.


[PATCH 6/6] net/gve: reconstruct HW timestamps from DQO

priv->mbuf_timestamp_offset sits in rte_zmalloc'd dev_private memory so
it starts at 0, but the fast path uses:

	priv->mbuf_timestamp_offset >= 0

The check is dead under the rxq->timestamp_enabled gate today, but the
>= 0 form suggests an "offset registered" semantic that isn't actually
enforced - a zero-initialized offset passes. Initialize
priv->mbuf_timestamp_offset to -1 in gve_dev_init(), or remove the
inner check since timestamp_enabled is the real gate.