include/net/busy_poll.h | 8 -------- include/net/xdp.h | 1 - include/net/xdp_sock_drv.h | 14 -------------- net/core/xdp.c | 1 - net/xdp/xsk.c | 14 +++++++++----- 5 files changed, 9 insertions(+), 29 deletions(-)
Commit 86e25f40aa1e ("net: napi: Add napi_config") moved napi->napi_id
assignment to a later point in time (napi_hash_add_with_id). This breaks
__xdp_rxq_info_reg which copies napi_id at an earlier time and now
stores 0 napi_id. It also makes sk_mark_napi_id_once_xdp and
__sk_mark_napi_id_once useless because they now work against 0 napi_id.
Since sk_busy_loop requires valid napi_id to busy-poll on, there is no way
to busy-poll AF_XDP sockets anymore.
Bring back the ability to busy-poll on XSK by resolving socket's napi_id
at bind time. This relies on relatively recent netif_queue_set_napi,
but (assume) at this point most popular drivers should have been converted.
This also removes per-tx/rx cycles which used to check and/or set
the napi_id value.
Confirmed by running a busy-polling AF_XDP socket
(github.com/fomichev/xskrtt) on mlx5 and looking at BusyPollRxPackets
from /proc/net/netstat.
Fixes: 86e25f40aa1e ("net: napi: Add napi_config")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
include/net/busy_poll.h | 8 --------
include/net/xdp.h | 1 -
include/net/xdp_sock_drv.h | 14 --------------
net/core/xdp.c | 1 -
net/xdp/xsk.c | 14 +++++++++-----
5 files changed, 9 insertions(+), 29 deletions(-)
diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
index c858270141bc..c39a426ebf52 100644
--- a/include/net/busy_poll.h
+++ b/include/net/busy_poll.h
@@ -174,12 +174,4 @@ static inline void sk_mark_napi_id_once(struct sock *sk,
#endif
}
-static inline void sk_mark_napi_id_once_xdp(struct sock *sk,
- const struct xdp_buff *xdp)
-{
-#ifdef CONFIG_NET_RX_BUSY_POLL
- __sk_mark_napi_id_once(sk, xdp->rxq->napi_id);
-#endif
-}
-
#endif /* _LINUX_NET_BUSY_POLL_H */
diff --git a/include/net/xdp.h b/include/net/xdp.h
index e6770dd40c91..b5b10f2b88e5 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -62,7 +62,6 @@ struct xdp_rxq_info {
u32 queue_index;
u32 reg_state;
struct xdp_mem_info mem;
- unsigned int napi_id;
u32 frag_size;
} ____cacheline_aligned; /* perf critical, avoid false-sharing */
diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index 40085afd9160..7a7316d9c0da 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -59,15 +59,6 @@ static inline void xsk_pool_fill_cb(struct xsk_buff_pool *pool,
xp_fill_cb(pool, desc);
}
-static inline unsigned int xsk_pool_get_napi_id(struct xsk_buff_pool *pool)
-{
-#ifdef CONFIG_NET_RX_BUSY_POLL
- return pool->heads[0].xdp.rxq->napi_id;
-#else
- return 0;
-#endif
-}
-
static inline void xsk_pool_dma_unmap(struct xsk_buff_pool *pool,
unsigned long attrs)
{
@@ -306,11 +297,6 @@ static inline void xsk_pool_fill_cb(struct xsk_buff_pool *pool,
{
}
-static inline unsigned int xsk_pool_get_napi_id(struct xsk_buff_pool *pool)
-{
- return 0;
-}
-
static inline void xsk_pool_dma_unmap(struct xsk_buff_pool *pool,
unsigned long attrs)
{
diff --git a/net/core/xdp.c b/net/core/xdp.c
index bcc5551c6424..2315feed94ef 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -186,7 +186,6 @@ int __xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
xdp_rxq_info_init(xdp_rxq);
xdp_rxq->dev = dev;
xdp_rxq->queue_index = queue_index;
- xdp_rxq->napi_id = napi_id;
xdp_rxq->frag_size = frag_size;
xdp_rxq->reg_state = REG_STATE_REGISTERED;
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 3fa70286c846..89d2bef96469 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -322,7 +322,6 @@ static int xsk_rcv_check(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
return -ENOSPC;
}
- sk_mark_napi_id_once_xdp(&xs->sk, xdp);
return 0;
}
@@ -908,11 +907,8 @@ static int __xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len
if (unlikely(!xs->tx))
return -ENOBUFS;
- if (sk_can_busy_loop(sk)) {
- if (xs->zc)
- __sk_mark_napi_id_once(sk, xsk_pool_get_napi_id(xs->pool));
+ if (sk_can_busy_loop(sk))
sk_busy_loop(sk, 1); /* only support non-blocking sockets */
- }
if (xs->zc && xsk_no_wakeup(sk))
return 0;
@@ -1298,6 +1294,14 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
xs->queue_id = qid;
xp_add_xsk(xs->pool, xs);
+ if (xs->zc && qid < dev->real_num_rx_queues) {
+ struct netdev_rx_queue *rxq;
+
+ rxq = __netif_get_rx_queue(dev, qid);
+ if (rxq->napi)
+ __sk_mark_napi_id_once(sk, rxq->napi->napi_id);
+ }
+
out_unlock:
if (err) {
dev_put(dev);
--
2.47.1On Wed, Jan 08, 2025 at 04:34:36PM -0800, Stanislav Fomichev wrote:
> Commit 86e25f40aa1e ("net: napi: Add napi_config") moved napi->napi_id
> assignment to a later point in time (napi_hash_add_with_id). This breaks
> __xdp_rxq_info_reg which copies napi_id at an earlier time and now
> stores 0 napi_id. It also makes sk_mark_napi_id_once_xdp and
> __sk_mark_napi_id_once useless because they now work against 0 napi_id.
> Since sk_busy_loop requires valid napi_id to busy-poll on, there is no way
> to busy-poll AF_XDP sockets anymore.
>
> Bring back the ability to busy-poll on XSK by resolving socket's napi_id
> at bind time. This relies on relatively recent netif_queue_set_napi,
> but (assume) at this point most popular drivers should have been converted.
> This also removes per-tx/rx cycles which used to check and/or set
> the napi_id value.
>
> Confirmed by running a busy-polling AF_XDP socket
> (github.com/fomichev/xskrtt) on mlx5 and looking at BusyPollRxPackets
> from /proc/net/netstat.
Thanks Stanislav for finding and fixing this.
I've CC'd Alex who reported a bug a couple weeks ago that might be
fixed by this change.
Alex: would you mind applying this patch to your tree to see if this
solves the issue you reported [1] ?
[1]: https://lore.kernel.org/netdev/DM8PR12MB5447837576EA58F490D6D4BFAD052@DM8PR12MB5447.namprd12.prod.outlook.com/
On Thu, 9 Jan 2025 at 01:35, Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Commit 86e25f40aa1e ("net: napi: Add napi_config") moved napi->napi_id
> assignment to a later point in time (napi_hash_add_with_id). This breaks
> __xdp_rxq_info_reg which copies napi_id at an earlier time and now
> stores 0 napi_id. It also makes sk_mark_napi_id_once_xdp and
> __sk_mark_napi_id_once useless because they now work against 0 napi_id.
> Since sk_busy_loop requires valid napi_id to busy-poll on, there is no way
> to busy-poll AF_XDP sockets anymore.
>
> Bring back the ability to busy-poll on XSK by resolving socket's napi_id
> at bind time. This relies on relatively recent netif_queue_set_napi,
> but (assume) at this point most popular drivers should have been converted.
> This also removes per-tx/rx cycles which used to check and/or set
> the napi_id value.
>
> Confirmed by running a busy-polling AF_XDP socket
> (github.com/fomichev/xskrtt) on mlx5 and looking at BusyPollRxPackets
> from /proc/net/netstat.
Thanks Stanislav for finding and fixing this. As a bonus, the
resulting code is much nicer too.
I just took a look at the Intel drivers and some of our drivers have
not been converted to use netif_queue_set_napi() yet. Just ice, e1000,
and e1000e use it. But that is on us to fix.
From the xsk point of view:
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Fixes: 86e25f40aa1e ("net: napi: Add napi_config")
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
> include/net/busy_poll.h | 8 --------
> include/net/xdp.h | 1 -
> include/net/xdp_sock_drv.h | 14 --------------
> net/core/xdp.c | 1 -
> net/xdp/xsk.c | 14 +++++++++-----
> 5 files changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
> index c858270141bc..c39a426ebf52 100644
> --- a/include/net/busy_poll.h
> +++ b/include/net/busy_poll.h
> @@ -174,12 +174,4 @@ static inline void sk_mark_napi_id_once(struct sock *sk,
> #endif
> }
>
> -static inline void sk_mark_napi_id_once_xdp(struct sock *sk,
> - const struct xdp_buff *xdp)
> -{
> -#ifdef CONFIG_NET_RX_BUSY_POLL
> - __sk_mark_napi_id_once(sk, xdp->rxq->napi_id);
> -#endif
> -}
> -
> #endif /* _LINUX_NET_BUSY_POLL_H */
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index e6770dd40c91..b5b10f2b88e5 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -62,7 +62,6 @@ struct xdp_rxq_info {
> u32 queue_index;
> u32 reg_state;
> struct xdp_mem_info mem;
> - unsigned int napi_id;
> u32 frag_size;
> } ____cacheline_aligned; /* perf critical, avoid false-sharing */
>
> diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> index 40085afd9160..7a7316d9c0da 100644
> --- a/include/net/xdp_sock_drv.h
> +++ b/include/net/xdp_sock_drv.h
> @@ -59,15 +59,6 @@ static inline void xsk_pool_fill_cb(struct xsk_buff_pool *pool,
> xp_fill_cb(pool, desc);
> }
>
> -static inline unsigned int xsk_pool_get_napi_id(struct xsk_buff_pool *pool)
> -{
> -#ifdef CONFIG_NET_RX_BUSY_POLL
> - return pool->heads[0].xdp.rxq->napi_id;
> -#else
> - return 0;
> -#endif
> -}
> -
> static inline void xsk_pool_dma_unmap(struct xsk_buff_pool *pool,
> unsigned long attrs)
> {
> @@ -306,11 +297,6 @@ static inline void xsk_pool_fill_cb(struct xsk_buff_pool *pool,
> {
> }
>
> -static inline unsigned int xsk_pool_get_napi_id(struct xsk_buff_pool *pool)
> -{
> - return 0;
> -}
> -
> static inline void xsk_pool_dma_unmap(struct xsk_buff_pool *pool,
> unsigned long attrs)
> {
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index bcc5551c6424..2315feed94ef 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -186,7 +186,6 @@ int __xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
> xdp_rxq_info_init(xdp_rxq);
> xdp_rxq->dev = dev;
> xdp_rxq->queue_index = queue_index;
> - xdp_rxq->napi_id = napi_id;
> xdp_rxq->frag_size = frag_size;
>
> xdp_rxq->reg_state = REG_STATE_REGISTERED;
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 3fa70286c846..89d2bef96469 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -322,7 +322,6 @@ static int xsk_rcv_check(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
> return -ENOSPC;
> }
>
> - sk_mark_napi_id_once_xdp(&xs->sk, xdp);
> return 0;
> }
>
> @@ -908,11 +907,8 @@ static int __xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len
> if (unlikely(!xs->tx))
> return -ENOBUFS;
>
> - if (sk_can_busy_loop(sk)) {
> - if (xs->zc)
> - __sk_mark_napi_id_once(sk, xsk_pool_get_napi_id(xs->pool));
> + if (sk_can_busy_loop(sk))
> sk_busy_loop(sk, 1); /* only support non-blocking sockets */
> - }
>
> if (xs->zc && xsk_no_wakeup(sk))
> return 0;
> @@ -1298,6 +1294,14 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> xs->queue_id = qid;
> xp_add_xsk(xs->pool, xs);
>
> + if (xs->zc && qid < dev->real_num_rx_queues) {
> + struct netdev_rx_queue *rxq;
> +
> + rxq = __netif_get_rx_queue(dev, qid);
> + if (rxq->napi)
> + __sk_mark_napi_id_once(sk, rxq->napi->napi_id);
> + }
> +
> out_unlock:
> if (err) {
> dev_put(dev);
> --
> 2.47.1
>
>
On Thu, Jan 09, 2025 at 04:22:16PM +0100, Magnus Karlsson wrote:
> On Thu, 9 Jan 2025 at 01:35, Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Commit 86e25f40aa1e ("net: napi: Add napi_config") moved napi->napi_id
> > assignment to a later point in time (napi_hash_add_with_id). This breaks
> > __xdp_rxq_info_reg which copies napi_id at an earlier time and now
> > stores 0 napi_id. It also makes sk_mark_napi_id_once_xdp and
> > __sk_mark_napi_id_once useless because they now work against 0 napi_id.
> > Since sk_busy_loop requires valid napi_id to busy-poll on, there is no way
> > to busy-poll AF_XDP sockets anymore.
> >
> > Bring back the ability to busy-poll on XSK by resolving socket's napi_id
> > at bind time. This relies on relatively recent netif_queue_set_napi,
> > but (assume) at this point most popular drivers should have been converted.
> > This also removes per-tx/rx cycles which used to check and/or set
> > the napi_id value.
> >
> > Confirmed by running a busy-polling AF_XDP socket
> > (github.com/fomichev/xskrtt) on mlx5 and looking at BusyPollRxPackets
> > from /proc/net/netstat.
>
> Thanks Stanislav for finding and fixing this. As a bonus, the
> resulting code is much nicer too.
>
> I just took a look at the Intel drivers and some of our drivers have
> not been converted to use netif_queue_set_napi() yet. Just ice, e1000,
> and e1000e use it. But that is on us to fix.
igc also supports it ;)
I tried to add support to i40e some time ago, but ran into some
issues and didn't hear back, so I gave up on i40e.
In case my previous attempt is helpful for anyone at Intel, see [1].
[1]: https://lore.kernel.org/lkml/20240410043936.206169-1-jdamato@fastly.com/
On Thu, 9 Jan 2025 16:22:16 +0100 Magnus Karlsson wrote: > > Confirmed by running a busy-polling AF_XDP socket > > (github.com/fomichev/xskrtt) on mlx5 and looking at BusyPollRxPackets > > from /proc/net/netstat. > > Thanks Stanislav for finding and fixing this. As a bonus, the > resulting code is much nicer too. > > I just took a look at the Intel drivers and some of our drivers have > not been converted to use netif_queue_set_napi() yet. Just ice, e1000, > and e1000e use it. But that is on us to fix. Yup, on a quick look yesterday I think I spotted a few embedded drivers (stmmac, tsnep, dpaa2), nfp and virtio_net which don't seem to link the NAPI to queues. But I can't think of a better fix, and updating those drivers to link NAPI to queues will be generally beneficial, so in case someone else applies this: Reviewed-by: Jakub Kicinski <kuba@kernel.org>
© 2016 - 2025 Red Hat, Inc.