On Mon, 11 May 2026 22:43:48 +0000 mark-blasko <[email protected]> wrote:
> From: Mark Blasko <[email protected]> > > 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.
