[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89iKOK9GyjQ_s_eo5XNugHARSRoeeu2c1muhBj8oxYL6UEQ@mail.gmail.com>
Date: Tue, 14 Oct 2025 07:39:59 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Harshitha Ramamurthy <hramamurthy@...gle.com>
Cc: netdev@...r.kernel.org, joshwash@...gle.com, andrew+netdev@...n.ch,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, willemb@...gle.com,
pkaligineedi@...gle.com, jfraker@...gle.com, ziweixiao@...gle.com,
thostet@...gle.com, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH net] gve: Check valid ts bit on RX descriptor before hw timestamping
On Mon, Oct 13, 2025 at 5:47 PM Harshitha Ramamurthy
<hramamurthy@...gle.com> wrote:
>
> From: Tim Hostetler <thostet@...gle.com>
>
> The device returns a valid bit in the LSB of the low timestamp byte in
> the completion descriptor that the driver should check before
> setting the SKB's hardware timestamp. If the timestamp is not valid, do not
> hardware timestamp the SKB.
>
> Cc: stable@...r.kernel.org
> Fixes: b2c7aeb49056 ("gve: Implement ndo_hwtstamp_get/set for RX timestamping")
> Reviewed-by: Joshua Washington <joshwash@...gle.com>
> Signed-off-by: Tim Hostetler <thostet@...gle.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@...gle.com>
> ---
> drivers/net/ethernet/google/gve/gve.h | 2 ++
> drivers/net/ethernet/google/gve/gve_desc_dqo.h | 3 ++-
> drivers/net/ethernet/google/gve/gve_rx_dqo.c | 18 ++++++++++++------
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index bceaf9b05cb4..4cc6dcbfd367 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -100,6 +100,8 @@
> */
> #define GVE_DQO_QPL_ONDEMAND_ALLOC_THRESHOLD 96
>
> +#define GVE_DQO_RX_HWTSTAMP_VALID 0x1
> +
> /* Each slot in the desc ring has a 1:1 mapping to a slot in the data ring */
> struct gve_rx_desc_queue {
> struct gve_rx_desc *desc_ring; /* the descriptor ring */
> diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> index d17da841b5a0..f7786b03c744 100644
> --- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> +++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> @@ -236,7 +236,8 @@ struct gve_rx_compl_desc_dqo {
>
> u8 status_error1;
>
> - __le16 reserved5;
> + u8 reserved5;
> + u8 ts_sub_nsecs_low;
> __le16 buf_id; /* Buffer ID which was sent on the buffer queue. */
>
> union {
> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 7380c2b7a2d8..02e25be8a50d 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -456,14 +456,20 @@ static void gve_rx_skb_hash(struct sk_buff *skb,
> * Note that this means if the time delta between packet reception and the last
> * clock read is greater than ~2 seconds, this will provide invalid results.
> */
> -static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx, u32 hwts)
> +static void gve_rx_skb_hwtstamp(struct gve_rx_ring *rx,
> + const struct gve_rx_compl_desc_dqo *desc)
> {
> u64 last_read = READ_ONCE(rx->gve->last_sync_nic_counter);
> struct sk_buff *skb = rx->ctx.skb_head;
> - u32 low = (u32)last_read;
> - s32 diff = hwts - low;
> -
> - skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> + u32 ts, low;
> + s32 diff;
> +
> + if (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) {
> + ts = le32_to_cpu(desc->ts);
> + low = (u32)last_read;
> + diff = ts - low;
> + skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(last_read + diff);
> + }
If (desc->ts_sub_nsecs_low & GVE_DQO_RX_HWTSTAMP_VALID) can vary among
all packets received on this queue,
I will suggest you add an
else {
skb_hwtstamps(skb)->hwtstamp = 0;
}
This is because napi_reuse_skb() does not currently clear this field.
So if a prior skb had hwtstamp set to a timestamp, then merged in GRO,
and recycled, we have the risk of reusing an old timestamp
if GVE_DQO_RX_HWTSTAMP_VALID is unset.
We could also handle this generically, at the cost of one extra
instruction in the fast path.
> }
>
> static void gve_rx_free_skb(struct napi_struct *napi, struct gve_rx_ring *rx)
> @@ -919,7 +925,7 @@ static int gve_rx_complete_skb(struct gve_rx_ring *rx, struct napi_struct *napi,
> gve_rx_skb_csum(rx->ctx.skb_head, desc, ptype);
>
> if (rx->gve->ts_config.rx_filter == HWTSTAMP_FILTER_ALL)
> - gve_rx_skb_hwtstamp(rx, le32_to_cpu(desc->ts));
> + gve_rx_skb_hwtstamp(rx, desc);
>
> /* RSC packets must set gso_size otherwise the TCP stack will complain
> * that packets are larger than MTU.
> --
> 2.51.0.740.g6adb054d12-goog
>
Powered by blists - more mailing lists