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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 12 May 2026 00:53:47 +0000
Mark Blasko <blasko@google.com> wrote:

> 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
> 
>  .mailmap                               |   1 +
>  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 +++++
>  10 files changed, 378 insertions(+), 22 deletions(-)
> 

Please use version when sending updated patches. See contribution docs.
I.e use -v2 --in-reply-to <mail message id>

AI review spotted some issues:

Patch 1: net/gve: add thread safety to admin queue

Info: The new adminq_lock is initialized with PTHREAD_MUTEX_ROBUST,
but pthread_mutex_lock() return values are not checked anywhere in
the patch. With robust attribute set, lock can return EOWNERDEAD if
a previous owner terminated while holding the mutex; the caller is
required to call pthread_mutex_consistent() in that case, otherwise
the mutex transitions to ENOTRECOVERABLE on subsequent locks. In a
single-process DPDK PMD a thread crash brings down the whole
process, so EOWNERDEAD is unreachable in practice and the robust
attribute is unnecessary. The existing flow_rule_lock in this
driver uses PROCESS_SHARED but not ROBUST. Drop
pthread_mutexattr_setrobust() to match.


Patch 5: net/gve: support read clock ethdev op

Warning: gve_read_clock and the periodic alarm callback
gve_read_nic_clock share a single DMA buffer (priv->nic_ts_report)
for the GVE_ADMINQ_REPORT_NIC_TIMESTAMP response. The adminq_lock
introduced in patch 1 serializes the adminq command exchange, but
is released inside gve_adminq_execute_cmd before either caller
reads the buffer:

    err = gve_adminq_report_nic_timestamp(priv,
                                priv->nic_ts_report_mz->iova);
    /* lock has already been released here */
    if (err != 0)
        return err;
    ts = be64_to_cpu(priv->nic_ts_report->nic_timestamp);

If the user thread (gve_read_clock) is preempted between returning
from gve_adminq_report_nic_timestamp and the be64_to_cpu read, the
alarm callback can run, memset() the buffer to zero, take the
adminq lock, issue its own command, and either leave its own
response in the buffer or be mid-DMA when the user thread reads.
The user thread will then read zero or a different command's
response.

The simplest fix is to return the cached
priv->last_read_nic_timestamp from gve_read_clock rather than
issuing a fresh adminq command. The periodic sync runs every
250ms, so the cached value is recent; the .read_clock semantics
do not require a brand new device read. Alternatively, use a
separate per-caller DMA buffer or extend the critical section
to cover the buffer read.


Patch 6: net/gve: reconstruct HW timestamps from DQO

Warning: RTE_ETH_RX_OFFLOAD_TIMESTAMP is added to
dev_info->rx_offload_capa whenever priv->nic_timestamp_supported
is true, but per-packet timestamp delivery is only implemented in
the DQO RX path (gve_rx_burst_dqo). For GQI devices,
rxq->timestamp_enabled is never set (gve_rx_queue_setup_dqo is
the only setup path that consults the offload flag), and the GQI
burst function does not touch the timestamp fields. A GQI user
that enables RX_OFFLOAD_TIMESTAMP will see the configure call
succeed and no timestamps appear in any mbuf - the offload is
silently a no-op.

The release notes for this series ("Added hardware timestamping
support on DQO queues") already scope the feature to DQO. The
capability advertisement should match:

    if (!gve_is_gqi(priv) && priv->nic_timestamp_supported)
        dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;

Info: doc/guides/nics/gve.rst's new "Timestamp Offload" section
should mention that the per-packet RX timestamp requires DQO
queue format. The .read_clock op works regardless of queue
format; only the per-packet path is DQO-only.