On 11/5/2026 12:04 AM, [email protected] wrote: >> 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 <[email protected]> >> Reviewed-by: Florian Bezdeka <[email protected]> > > 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? >
I think the AI is reading a bit much into the commit message. Clearly the bug is that 97dc7cd92ac6 depends on busy polling. The wording could be slightly more precise to not imply that e68b6e50fa35 actually introduced a problem, but I think that is a bit of a nitpick, and not worth a reroll of this work unless something else needs to change. Reviewed-by: Jacob Keller <[email protected]> > > --- > 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
