[PATCH] net: mark skb NAPI IDs independently of busy polling

[PATCH] net: mark skb NAPI IDs independently of busy polling

[Ding Meng]

Commit 97dc7cd92ac6 ("ptp: Support late timestamp determination")
added support for deferring RX timestamp conversion to
ndo_get_tstamp(). When the timestamp is consumed, get_timestamp()
uses the skb's NAPI ID to find the device that owns the timestamp
cookie and invoke its callback.

Commit e68b6e50fa35 ("udp: enable busy polling for all sockets")
gated NAPI ID bookkeeping with CONFIG_NET_RX_BUSY_POLL. As a result,
when busy polling is disabled, the skb may not carry a usable NAPI ID.
The device lookup then fails, so the driver callback is not invoked.
get_timestamp() falls back to hwtstamp, which does not contain the
converted timestamp, resulting in an incorrect RX hardware timestamp.

Record skb NAPI IDs independently of busy-poll support to support
drivers that use deferred timestamp conversion and future driver
features that need to look up a device by its NAPI ID.

Signed-off-by: Ding Meng <meng.ding@siemens.com>
Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
In the discussion of the issue reported at
https://lore.kernel.org/netdev/20260630112056.885071-1-meng.ding@siemens.com/
this previous patch added an igc-specific fallback for configurations
with CONFIG_NET_RX_BUSY_POLL disabled. It populated skb's hwtstamp
field with a timestamp converted using the default hardware timer,
allowing get_timestamp() to return it without invoking ndo_get_tstamp().
But Jakub Kicinski noted that the proposed fix appeared to be a
driver-specific workaround for a generic problem. The engleder driver
would also be affected by the same issue.

This patch introduces a new configuration option to enable NAPI ID
bookkeeping independently of busy polling. The option is enabled
whenever busy polling is enabled, preserving the existing behavior.
It is also selected by the igc and engleder drivers, ensuring that
skbs carry a NAPI ID even when busy polling is disabled.

Another approach considered was to require drivers using deferred
timestamp conversion, such as igc and engleder, to enable busy polling.
However, this would introduce an unnecessary dependency between two
independent features: hardware timestamp reporting should not require
busy polling to be enabled.

Timestamping has been tested only with igc; it has not been tested
with engleder.

 Documentation/networking/timestamping.rst | 11 ++++++++---
 drivers/net/ethernet/engleder/Kconfig     |  1 +
 drivers/net/ethernet/intel/Kconfig        |  1 +
 include/linux/skbuff.h                    |  4 ++--
 include/net/busy_poll.h                   |  2 +-
 include/trace/events/net.h                |  2 +-
 net/Kconfig                               |  4 ++++
 net/core/filter.c                         |  2 +-
 net/core/skbuff.c                         |  2 +-
 9 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index 2162c4f2b28a6..dbe40a1972639 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -278,9 +278,14 @@ SOF_TIMESTAMPING_OPT_PKTINFO:
   packets with hardware timestamps. The message contains struct
   scm_ts_pktinfo, which supplies the index of the real interface which
   received the packet and its length at layer 2. A valid (non-zero)
-  interface index will be returned only if CONFIG_NET_RX_BUSY_POLL is
-  enabled and the driver is using NAPI. The struct contains also two
-  other fields, but they are reserved and undefined.
+  interface index will be returned only if CONFIG_SKB_MARK_NAPI_ID is
+  enabled and the driver is using NAPI. This hidden configuration is
+  selected automatically by busy-poll support and by drivers that need
+  skb NAPI IDs to resolve deferred hardware timestamps. Drivers that
+  use deferred timestamp conversion must select it so the networking
+  stack can find the device and call its timestamp callback even when
+  busy polling is disabled. The struct contains also two other fields,
+  but they are reserved and undefined.
 
 SOF_TIMESTAMPING_OPT_TX_SWHW:
   Request both hardware and software timestamps for outgoing packets
diff --git a/drivers/net/ethernet/engleder/Kconfig b/drivers/net/ethernet/engleder/Kconfig
index 3df6bf476ae7e..9700a243bea11 100644
--- a/drivers/net/ethernet/engleder/Kconfig
+++ b/drivers/net/ethernet/engleder/Kconfig
@@ -22,6 +22,7 @@ config TSNEP
 	depends on PTP_1588_CLOCK_OPTIONAL
 	select PHYLIB
 	select PAGE_POOL
+	select SKB_MARK_NAPI_ID
 	help
 	  Support for the Engleder TSN endpoint Ethernet MAC IP Core.
 
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 780f113986ea8..7fb63f351fc20 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -376,6 +376,7 @@ config IGC
 	depends on PCI
 	depends on PTP_1588_CLOCK_OPTIONAL
 	depends on ETHTOOL_NETLINK
+	select SKB_MARK_NAPI_ID
 	help
 	  This driver supports Intel(R) Ethernet Controller I225-LM/I225-V
 	  family of adapters.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6787fcf6ac3e4..164c862f7a403 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1059,7 +1059,7 @@ struct sk_buff {
 			__u16	vlan_tci;
 		};
 	};
-#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
+#if defined(CONFIG_SKB_MARK_NAPI_ID) || defined(CONFIG_XPS)
 	union {
 		unsigned int	napi_id;
 		unsigned int	sender_cpu;
@@ -1267,7 +1267,7 @@ static inline bool skb_pkt_type_ok(u32 ptype)
  */
 static inline unsigned int skb_napi_id(const struct sk_buff *skb)
 {
-#ifdef CONFIG_NET_RX_BUSY_POLL
+#ifdef CONFIG_SKB_MARK_NAPI_ID
 	return skb->napi_id;
 #else
 	return 0;
diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
index 6e172d0f6ef5f..f7c0e62c6a7d7 100644
--- a/include/net/busy_poll.h
+++ b/include/net/busy_poll.h
@@ -130,7 +130,7 @@ static inline void sk_busy_loop(struct sock *sk, int nonblock)
 static inline void __skb_mark_napi_id(struct sk_buff *skb,
 				      const struct gro_node *gro)
 {
-#ifdef CONFIG_NET_RX_BUSY_POLL
+#ifdef CONFIG_SKB_MARK_NAPI_ID
 	/* If the skb was already marked with a valid NAPI ID, avoid overwriting
 	 * it.
 	 */
diff --git a/include/trace/events/net.h b/include/trace/events/net.h
index dbc2c5598e35a..99ed7645242eb 100644
--- a/include/trace/events/net.h
+++ b/include/trace/events/net.h
@@ -208,7 +208,7 @@ DECLARE_EVENT_CLASS(net_dev_rx_verbose_template,
 
 	TP_fast_assign(
 		__assign_str(name);
-#ifdef CONFIG_NET_RX_BUSY_POLL
+#ifdef CONFIG_SKB_MARK_NAPI_ID
 		__entry->napi_id = napi_id_valid(skb->napi_id) ?
 				   skb->napi_id : 0;
 #else
diff --git a/net/Kconfig b/net/Kconfig
index 76ab44aa439a6..8fa7586dc08ca 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -346,9 +346,13 @@ config CGROUP_NET_CLASSID
 	  Cgroup subsystem for use as general purpose socket classid marker that is
 	  being used in cls_cgroup and for netfilter matching.
 
+config SKB_MARK_NAPI_ID
+	bool
+
 config NET_RX_BUSY_POLL
 	bool
 	default y if !PREEMPT_RT || (PREEMPT_RT && !NETCONSOLE)
+	select SKB_MARK_NAPI_ID
 
 config BQL
 	bool
diff --git a/net/core/filter.c b/net/core/filter.c
index b7ac145b68b1f..e2218c18622b8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10274,7 +10274,7 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
 		break;
 
 	case offsetof(struct __sk_buff, napi_id):
-#if defined(CONFIG_NET_RX_BUSY_POLL)
+#if defined(CONFIG_SKB_MARK_NAPI_ID)
 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
 				      bpf_target_off(struct sk_buff, napi_id, 4,
 						     target_size));
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index eb9fcc4b1fd1a..386a32cdc0cc8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1581,7 +1581,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 #ifdef CONFIG_NETWORK_SECMARK
 	CHECK_SKB_FIELD(secmark);
 #endif
-#ifdef CONFIG_NET_RX_BUSY_POLL
+#ifdef CONFIG_SKB_MARK_NAPI_ID
 	CHECK_SKB_FIELD(napi_id);
 #endif
 #ifdef CONFIG_XPS

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.47.3

Re: [PATCH] net: mark skb NAPI IDs independently of busy polling

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

> net: mark skb NAPI IDs independently of busy polling

TL;DR: Commit message only: it blames e68b6e50fa35 for gating skb NAPI IDs on
busy polling, but that gating predates it; 97dc7cd92ac6 introduced the
dependency. Suggests rewording.

>     net: mark skb NAPI IDs independently of busy polling
>
>     Commit 97dc7cd92ac6 ("ptp: Support late timestamp determination")
>     added support for deferring RX timestamp conversion to
>     ndo_get_tstamp(). When the timestamp is consumed, get_timestamp()
>     uses the skb's NAPI ID to find the device that owns the timestamp
>     cookie and invoke its callback.
>     Commit e68b6e50fa35 ("udp: enable busy polling for all sockets")
>     gated NAPI ID bookkeeping with CONFIG_NET_RX_BUSY_POLL. As a result,
>     when busy polling is disabled, the skb may not carry a usable NAPI ID.
>     The device lookup then fails, so the driver callback is not invoked.
>     get_timestamp() falls back to hwtstamp, which does not contain the
>     converted timestamp, resulting in an incorrect RX hardware timestamp.
>     Record skb NAPI IDs independently of busy-poll support to support
>     drivers that use deferred timestamp conversion and future driver
>     features that need to look up a device by its NAPI ID.
>     Signed-off-by: Ding Meng <meng.ding@siemens.com>
>     Reviewed-by: Florian Bezdeka <florian.bezdeka@siemens.com>

This isn't a bug, but is e68b6e50fa35 the right commit to name here?

Before that commit, __skb_mark_napi_id() / skb_mark_napi_id() was already
inside '#ifdef CONFIG_NET_RX_BUSY_POLL' with an empty stub in the #else
branch, and struct sk_buff already had napi_id under
'#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)'.  That gating
goes back to 8b80cda536ea ("net: rename ll methods to busy-poll").

e68b6e50fa35 only moved the socket-side sk_mark_napi_id() out of the ifdef
block and added sk_mark_napi_id_once().  Both act on sk->sk_napi_id, not on
the skb.

It is also about six years older than 97dc7cd92ac6, so the message reads as
if a later change broke the deferred-timestamp lookup.  In fact
get_timestamp() has depended on a busy-poll-only skb field (skb_napi_id()
returns 0 when CONFIG_NET_RX_BUSY_POLL=n) since 97dc7cd92ac6 added it.

Could the message say that the skb NAPI ID was always busy-poll-only and
that 97dc7cd92ac6 started depending on it?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41989099174