netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2] wireguard: queuing: preserve napi_id on decapsulation
@ 2025-11-03 10:34 Miroslav Lichvar
  2025-11-03 16:00 ` Jason A. Donenfeld
  0 siblings, 1 reply; 3+ messages in thread
From: Miroslav Lichvar @ 2025-11-03 10:34 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Jason A. Donenfeld, Willem de Bruijn

The socket timestamping option SOF_TIMESTAMPING_OPT_PKTINFO needs the
skb napi_id in order to provide the index of the device that captured
the hardware receive timestamp. However, wireguard resets most of the
skb headers, including the napi_id, which prevents the timestamping
option from working as expected. The missing index prevents applications
that rely on it (e.g. chrony) from processing hardware receive
timestamps (unlike with transmit timestamps looped back to the error
queue, where the IP_PKTINFO index identifies the device that captured
the timestamp).

Preserve the napi_id in wg_reset_packet() of received packets in order
to make the timestamping option useful with wireguard tunnels and enable
highly accurate synchronization.

Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---

Notes:
    v2:
    - don't copy napi_id if only CONFIG_XPS is defined
    - improve commit message

 drivers/net/wireguard/queueing.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 79b6d70de236..a5b76cecf429 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -75,6 +75,7 @@ static inline bool wg_check_packet_protocol(struct sk_buff *skb)
 
 static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
 {
+	unsigned int napi_id = skb_napi_id(skb);
 	u8 l4_hash = skb->l4_hash;
 	u8 sw_hash = skb->sw_hash;
 	u32 hash = skb->hash;
@@ -84,6 +85,10 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
 		skb->l4_hash = l4_hash;
 		skb->sw_hash = sw_hash;
 		skb->hash = hash;
+	} else {
+#ifdef CONFIG_NET_RX_BUSY_POLL
+		skb->napi_id = napi_id;
+#endif
 	}
 	skb->queue_mapping = 0;
 	skb->nohdr = 0;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next v2] wireguard: queuing: preserve napi_id on decapsulation
  2025-11-03 10:34 [PATCH net-next v2] wireguard: queuing: preserve napi_id on decapsulation Miroslav Lichvar
@ 2025-11-03 16:00 ` Jason A. Donenfeld
  2025-11-04  9:28   ` Miroslav Lichvar
  0 siblings, 1 reply; 3+ messages in thread
From: Jason A. Donenfeld @ 2025-11-03 16:00 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev, Willem de Bruijn

On Mon, Nov 3, 2025 at 11:34 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> +       } else {
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> +               skb->napi_id = napi_id;
> +#endif

It looks like io_uring has timestamping on tx, not just rx:
SOCKET_URING_OP_TX_TIMESTAMP -> io_uring_cmd_timestamp ->
io_process_timestamp_skb -> skb_get_tx_timestamp -> skb_napi_id

So are you sure this should be in an `else {` block?

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next v2] wireguard: queuing: preserve napi_id on decapsulation
  2025-11-03 16:00 ` Jason A. Donenfeld
@ 2025-11-04  9:28   ` Miroslav Lichvar
  0 siblings, 0 replies; 3+ messages in thread
From: Miroslav Lichvar @ 2025-11-04  9:28 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netdev, Willem de Bruijn

On Mon, Nov 03, 2025 at 05:00:19PM +0100, Jason A. Donenfeld wrote:
> On Mon, Nov 3, 2025 at 11:34 AM Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > +       } else {
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > +               skb->napi_id = napi_id;
> > +#endif
> 
> It looks like io_uring has timestamping on tx, not just rx:
> SOCKET_URING_OP_TX_TIMESTAMP -> io_uring_cmd_timestamp ->
> io_process_timestamp_skb -> skb_get_tx_timestamp -> skb_napi_id

That doesn't seem to be reachable. skb_get_tx_timestamp() calls
get_timestamp()->skb_napi_id() only when the SKBTX_HW_TSTAMP_NETDEV
flag is set, which is done only by two drivers (tsnep, igc) and only
in the RX path. Using a tx_flags field for RX is slightly confusing.

> So are you sure this should be in an `else {` block?

Yes, at least for the use case with the timestamping option I'm trying
to fix. If it was outside of the else block, it would be copying the
sender_cpu field of the union (used by XPS). Maybe it would make sense
to do that too, I know nothing about XPS, but in that case the value
shouldn't be read by skb_napi_id(), which is enabled only by
CONFIG_NET_RX_BUSY_POLL.

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-11-04  9:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-03 10:34 [PATCH net-next v2] wireguard: queuing: preserve napi_id on decapsulation Miroslav Lichvar
2025-11-03 16:00 ` Jason A. Donenfeld
2025-11-04  9:28   ` Miroslav Lichvar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).