* [PATCH net-next 0/2] net: devmem: improve cpu cost of RX token management
@ 2025-09-02 21:36 Bobby Eshleman
2025-09-02 21:36 ` [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding Bobby Eshleman
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Bobby Eshleman @ 2025-09-02 21:36 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell,
David Ahern
Cc: netdev, linux-kernel, Stanislav Fomichev, Mina Almasry,
Bobby Eshleman
This series improves the CPU cost of RX token management by replacing
the xarray allocator with a normal array of atomics. Similar to devmem
TX's page-index lookup scheme for niovs, RX also uses page indices to
lookup the corresponding atomic in the array.
Improvement is ~5% per RX user thread.
Two other approaches were tested, but with no improvement. Namely, 1)
using a hashmap for tokens and 2) keeping an xarray of atomic counters
but using RCU so that the hotpath could be mostly lockless. Neither of
these approaches proved better than the simple array in terms of CPU.
Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
---
Bobby Eshleman (2):
net: devmem: rename tx_vec to vec in dmabuf binding
net: devmem: use niov array for token management
include/net/sock.h | 5 ++-
net/core/devmem.c | 31 +++++++-------
net/core/devmem.h | 4 +-
net/core/sock.c | 24 +++++++----
net/ipv4/tcp.c | 107 +++++++++++++++--------------------------------
net/ipv4/tcp_ipv4.c | 40 +++++++++++++++---
net/ipv4/tcp_minisocks.c | 2 -
7 files changed, 107 insertions(+), 106 deletions(-)
---
base-commit: cd8a4cfa6bb43a441901e82f5c222dddc75a18a3
change-id: 20250829-scratch-bobbyeshleman-devmem-tcp-token-upstream-292be174d503
Best regards,
--
Bobby Eshleman <bobbyeshleman@meta.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding 2025-09-02 21:36 [PATCH net-next 0/2] net: devmem: improve cpu cost of RX token management Bobby Eshleman @ 2025-09-02 21:36 ` Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 2/2] net: devmem: use niov array for token management Bobby Eshleman 2025-09-03 17:46 ` [syzbot ci] Re: net: devmem: improve cpu cost of RX " syzbot ci 2 siblings, 0 replies; 7+ messages in thread From: Bobby Eshleman @ 2025-09-02 21:36 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell, David Ahern Cc: netdev, linux-kernel, Stanislav Fomichev, Mina Almasry, Bobby Eshleman From: Bobby Eshleman <bobbyeshleman@meta.com> Rename the 'tx_vec' field in struct net_devmem_dmabuf_binding to 'vec'. This field holds pointers to net_iov structures. The rename prepares for reusing 'vec' for both TX and RX directions. No functional change intended. Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com> --- net/core/devmem.c | 22 +++++++++++----------- net/core/devmem.h | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/net/core/devmem.c b/net/core/devmem.c index d9de31a6cc7f..b4c570d4f37a 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c @@ -74,7 +74,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq) dma_buf_detach(binding->dmabuf, binding->attachment); dma_buf_put(binding->dmabuf); xa_destroy(&binding->bound_rxqs); - kvfree(binding->tx_vec); + kvfree(binding->vec); kfree(binding); } @@ -231,10 +231,10 @@ net_devmem_bind_dmabuf(struct net_device *dev, } if (direction == DMA_TO_DEVICE) { - binding->tx_vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, - sizeof(struct net_iov *), - GFP_KERNEL); - if (!binding->tx_vec) { + binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, + sizeof(struct net_iov *), + GFP_KERNEL); + if (!binding->vec) { err = -ENOMEM; goto err_unmap; } @@ -248,7 +248,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, dev_to_node(&dev->dev)); if (!binding->chunk_pool) { err = -ENOMEM; - goto err_tx_vec; + goto err_vec; } virtual = 0; @@ -294,7 +294,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, page_pool_set_dma_addr_netmem(net_iov_to_netmem(niov), net_devmem_get_dma_addr(niov)); if (direction == DMA_TO_DEVICE) - binding->tx_vec[owner->area.base_virtual / PAGE_SIZE + i] = niov; + binding->vec[owner->area.base_virtual / PAGE_SIZE + i] = niov; } virtual += len; @@ -314,8 +314,8 @@ net_devmem_bind_dmabuf(struct net_device *dev, gen_pool_for_each_chunk(binding->chunk_pool, net_devmem_dmabuf_free_chunk_owner, NULL); gen_pool_destroy(binding->chunk_pool); -err_tx_vec: - kvfree(binding->tx_vec); +err_vec: + kvfree(binding->vec); err_unmap: dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt, direction); @@ -361,7 +361,7 @@ struct net_devmem_dmabuf_binding *net_devmem_get_binding(struct sock *sk, int err = 0; binding = net_devmem_lookup_dmabuf(dmabuf_id); - if (!binding || !binding->tx_vec) { + if (!binding || !binding->vec) { err = -EINVAL; goto out_err; } @@ -393,7 +393,7 @@ net_devmem_get_niov_at(struct net_devmem_dmabuf_binding *binding, *off = virt_addr % PAGE_SIZE; *size = PAGE_SIZE - *off; - return binding->tx_vec[virt_addr / PAGE_SIZE]; + return binding->vec[virt_addr / PAGE_SIZE]; } /*** "Dmabuf devmem memory provider" ***/ diff --git a/net/core/devmem.h b/net/core/devmem.h index 101150d761af..2ada54fb63d7 100644 --- a/net/core/devmem.h +++ b/net/core/devmem.h @@ -63,7 +63,7 @@ struct net_devmem_dmabuf_binding { * address. This array is convenient to map the virtual addresses to * net_iovs in the TX path. */ - struct net_iov **tx_vec; + struct net_iov **vec; struct work_struct unbind_w; }; -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/2] net: devmem: use niov array for token management 2025-09-02 21:36 [PATCH net-next 0/2] net: devmem: improve cpu cost of RX token management Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding Bobby Eshleman @ 2025-09-02 21:36 ` Bobby Eshleman 2025-09-03 10:50 ` kernel test robot 2025-09-03 20:20 ` Mina Almasry 2025-09-03 17:46 ` [syzbot ci] Re: net: devmem: improve cpu cost of RX " syzbot ci 2 siblings, 2 replies; 7+ messages in thread From: Bobby Eshleman @ 2025-09-02 21:36 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell, David Ahern Cc: netdev, linux-kernel, Stanislav Fomichev, Mina Almasry, Bobby Eshleman From: Bobby Eshleman <bobbyeshleman@meta.com> Improve CPU performance of devmem token management by using page offsets as dmabuf tokens and using them for direct array access lookups instead of xarray lookups. Consequently, the xarray can be removed. The result is an average 5% reduction in CPU cycles spent by devmem RX user threads. This patch changes the meaning of tokens. Tokens previously referred to unique fragments of pages. In this patch tokens instead represent references to pages, not fragments. Because of this, multiple tokens may refer to the same page and so have identical value (e.g., two small fragments may coexist on the same page). The token and offset pair that the user receives uniquely identifies fragments if needed. This assumes that the user is not attempting to sort / uniq the token list using tokens alone. A new restriction is added to the implementation: devmem RX sockets cannot switch dmabuf bindings. In practice, this is a symptom of invalid configuration as a flow would have to be steered to a different queue or device where there is a different binding, which is generally bad for TCP flows. This restriction is necessary because the 32-bit dmabuf token does not have enough bits to represent both the pages in a large dmabuf and also a binding or dmabuf ID. For example, a system with 8 NICs and 32 queues requires 8 bits for a binding / queue ID (8 NICs * 32 queues == 256 queues total == 2^8), which leaves only 24 bits for dmabuf pages (2^24 * 4096 / (1<<30) == 64GB). This is insufficient for the device and queue numbers on many current systems or systems that may need larger GPU dmabufs (as for hard limits, my current H100 has 80GB GPU memory per device). Using kperf[1] with 4 flows and workers, this patch improves receive worker CPU util by ~4.9% with slightly better throughput. Before, mean cpu util for rx workers ~83.6%: Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle Average: 4 2.30 0.00 79.43 0.00 0.65 0.21 0.00 0.00 0.00 17.41 Average: 5 2.27 0.00 80.40 0.00 0.45 0.21 0.00 0.00 0.00 16.67 Average: 6 2.28 0.00 80.47 0.00 0.46 0.25 0.00 0.00 0.00 16.54 Average: 7 2.42 0.00 82.05 0.00 0.46 0.21 0.00 0.00 0.00 14.86 After, mean cpu util % for rx workers ~78.7%: Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle Average: 4 2.61 0.00 73.31 0.00 0.76 0.11 0.00 0.00 0.00 23.20 Average: 5 2.95 0.00 74.24 0.00 0.66 0.22 0.00 0.00 0.00 21.94 Average: 6 2.81 0.00 73.38 0.00 0.97 0.11 0.00 0.00 0.00 22.73 Average: 7 3.05 0.00 78.76 0.00 0.76 0.11 0.00 0.00 0.00 17.32 Mean throughput improves, but falls within a standard deviation (~45GB/s for 4 flows on a 50GB/s NIC, one hop). This patch adds an array of atomics for counting the tokens returned to the user for a given page. There is a 4-byte atomic per page in the dmabuf per socket. Given a 2GB dmabuf, this array is 2MB. [1]: https://github.com/facebookexperimental/kperf Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com> --- include/net/sock.h | 5 ++- net/core/devmem.c | 17 ++++---- net/core/devmem.h | 2 +- net/core/sock.c | 24 +++++++---- net/ipv4/tcp.c | 107 +++++++++++++++-------------------------------- net/ipv4/tcp_ipv4.c | 40 +++++++++++++++--- net/ipv4/tcp_minisocks.c | 2 - 7 files changed, 99 insertions(+), 98 deletions(-) diff --git a/include/net/sock.h b/include/net/sock.h index 1e7f124871d2..70c97880229d 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -573,7 +573,10 @@ struct sock { #endif struct rcu_head sk_rcu; netns_tracker ns_tracker; - struct xarray sk_user_frags; + struct { + struct net_devmem_dmabuf_binding *binding; + atomic_t *urefs; + } sk_user_frags; #if IS_ENABLED(CONFIG_PROVE_LOCKING) && IS_ENABLED(CONFIG_MODULES) struct module *sk_owner; diff --git a/net/core/devmem.c b/net/core/devmem.c index b4c570d4f37a..50e92dcf5bf1 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c @@ -187,6 +187,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, struct dma_buf *dmabuf; unsigned int sg_idx, i; unsigned long virtual; + gfp_t flags; int err; if (!dma_dev) { @@ -230,14 +231,14 @@ net_devmem_bind_dmabuf(struct net_device *dev, goto err_detach; } - if (direction == DMA_TO_DEVICE) { - binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, - sizeof(struct net_iov *), - GFP_KERNEL); - if (!binding->vec) { - err = -ENOMEM; - goto err_unmap; - } + flags = (direction == DMA_FROM_DEVICE) ? __GFP_ZERO : 0; + + binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, + sizeof(struct net_iov *), + GFP_KERNEL | flags); + if (!binding->vec) { + err = -ENOMEM; + goto err_unmap; } /* For simplicity we expect to make PAGE_SIZE allocations, but the diff --git a/net/core/devmem.h b/net/core/devmem.h index 2ada54fb63d7..d4eb28d079bb 100644 --- a/net/core/devmem.h +++ b/net/core/devmem.h @@ -61,7 +61,7 @@ struct net_devmem_dmabuf_binding { /* Array of net_iov pointers for this binding, sorted by virtual * address. This array is convenient to map the virtual addresses to - * net_iovs in the TX path. + * net_iovs. */ struct net_iov **vec; diff --git a/net/core/sock.c b/net/core/sock.c index 9a8290fcc35d..3a5cb4e10519 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -87,6 +87,7 @@ #include <linux/unaligned.h> #include <linux/capability.h> +#include <linux/dma-buf.h> #include <linux/errno.h> #include <linux/errqueue.h> #include <linux/types.h> @@ -151,6 +152,7 @@ #include <uapi/linux/pidfd.h> #include "dev.h" +#include "devmem.h" static DEFINE_MUTEX(proto_list_mutex); static LIST_HEAD(proto_list); @@ -1100,32 +1102,40 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) return -EFAULT; } - xa_lock_bh(&sk->sk_user_frags); for (i = 0; i < num_tokens; i++) { for (j = 0; j < tokens[i].token_count; j++) { + struct net_iov *niov; + unsigned int token; + netmem_ref netmem; + + token = tokens[i].token_start + j; + if (WARN_ONCE(token >= sk->sk_user_frags.binding->dmabuf->size / PAGE_SIZE, + "invalid token passed from user")) + break; + if (++num_frags > MAX_DONTNEED_FRAGS) goto frag_limit_reached; - - netmem_ref netmem = (__force netmem_ref)__xa_erase( - &sk->sk_user_frags, tokens[i].token_start + j); + niov = sk->sk_user_frags.binding->vec[token]; + netmem = net_iov_to_netmem(niov); if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem))) continue; + if (WARN_ONCE(atomic_dec_if_positive(&sk->sk_user_frags.urefs[token]) + < 0, "user released token too many times")) + continue; + netmems[netmem_num++] = netmem; if (netmem_num == ARRAY_SIZE(netmems)) { - xa_unlock_bh(&sk->sk_user_frags); for (k = 0; k < netmem_num; k++) WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); netmem_num = 0; - xa_lock_bh(&sk->sk_user_frags); } ret++; } } frag_limit_reached: - xa_unlock_bh(&sk->sk_user_frags); for (k = 0; k < netmem_num; k++) WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 40b774b4f587..585b50fa8c00 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -261,6 +261,7 @@ #include <linux/memblock.h> #include <linux/highmem.h> #include <linux/cache.h> +#include <linux/dma-buf.h> #include <linux/err.h> #include <linux/time.h> #include <linux/slab.h> @@ -475,7 +476,8 @@ void tcp_init_sock(struct sock *sk) set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags); sk_sockets_allocated_inc(sk); - xa_init_flags(&sk->sk_user_frags, XA_FLAGS_ALLOC1); + sk->sk_user_frags.binding = NULL; + sk->sk_user_frags.urefs = NULL; } EXPORT_IPV6_MOD(tcp_init_sock); @@ -2386,68 +2388,6 @@ static int tcp_inq_hint(struct sock *sk) return inq; } -/* batch __xa_alloc() calls and reduce xa_lock()/xa_unlock() overhead. */ -struct tcp_xa_pool { - u8 max; /* max <= MAX_SKB_FRAGS */ - u8 idx; /* idx <= max */ - __u32 tokens[MAX_SKB_FRAGS]; - netmem_ref netmems[MAX_SKB_FRAGS]; -}; - -static void tcp_xa_pool_commit_locked(struct sock *sk, struct tcp_xa_pool *p) -{ - int i; - - /* Commit part that has been copied to user space. */ - for (i = 0; i < p->idx; i++) - __xa_cmpxchg(&sk->sk_user_frags, p->tokens[i], XA_ZERO_ENTRY, - (__force void *)p->netmems[i], GFP_KERNEL); - /* Rollback what has been pre-allocated and is no longer needed. */ - for (; i < p->max; i++) - __xa_erase(&sk->sk_user_frags, p->tokens[i]); - - p->max = 0; - p->idx = 0; -} - -static void tcp_xa_pool_commit(struct sock *sk, struct tcp_xa_pool *p) -{ - if (!p->max) - return; - - xa_lock_bh(&sk->sk_user_frags); - - tcp_xa_pool_commit_locked(sk, p); - - xa_unlock_bh(&sk->sk_user_frags); -} - -static int tcp_xa_pool_refill(struct sock *sk, struct tcp_xa_pool *p, - unsigned int max_frags) -{ - int err, k; - - if (p->idx < p->max) - return 0; - - xa_lock_bh(&sk->sk_user_frags); - - tcp_xa_pool_commit_locked(sk, p); - - for (k = 0; k < max_frags; k++) { - err = __xa_alloc(&sk->sk_user_frags, &p->tokens[k], - XA_ZERO_ENTRY, xa_limit_31b, GFP_KERNEL); - if (err) - break; - } - - xa_unlock_bh(&sk->sk_user_frags); - - p->max = k; - p->idx = 0; - return k ? 0 : err; -} - /* On error, returns the -errno. On success, returns number of bytes sent to the * user. May not consume all of @remaining_len. */ @@ -2456,14 +2396,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, int remaining_len) { struct dmabuf_cmsg dmabuf_cmsg = { 0 }; - struct tcp_xa_pool tcp_xa_pool; unsigned int start; int i, copy, n; int sent = 0; int err = 0; - tcp_xa_pool.max = 0; - tcp_xa_pool.idx = 0; do { start = skb_headlen(skb); @@ -2510,8 +2447,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, */ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; + struct net_devmem_dmabuf_binding *binding; struct net_iov *niov; u64 frag_offset; + size_t size; + u32 token; int end; /* !skb_frags_readable() should indicate that ALL the @@ -2544,13 +2484,35 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, start; dmabuf_cmsg.frag_offset = frag_offset; dmabuf_cmsg.frag_size = copy; - err = tcp_xa_pool_refill(sk, &tcp_xa_pool, - skb_shinfo(skb)->nr_frags - i); - if (err) + + binding = net_devmem_iov_binding(niov); + + if (!sk->sk_user_frags.binding) { + sk->sk_user_frags.binding = binding; + + size = binding->dmabuf->size / PAGE_SIZE; + sk->sk_user_frags.urefs = kzalloc(size, + GFP_KERNEL); + if (!sk->sk_user_frags.urefs) { + sk->sk_user_frags.binding = NULL; + err = -ENOMEM; + goto out; + } + + net_devmem_dmabuf_binding_get(binding); + } + + if (WARN_ONCE(sk->sk_user_frags.binding != binding, + "binding changed for devmem socket")) { + err = -EFAULT; goto out; + } + + token = net_iov_virtual_addr(niov) >> PAGE_SHIFT; + binding->vec[token] = niov; + dmabuf_cmsg.frag_token = token; /* Will perform the exchange later */ - dmabuf_cmsg.frag_token = tcp_xa_pool.tokens[tcp_xa_pool.idx]; dmabuf_cmsg.dmabuf_id = net_devmem_iov_binding_id(niov); offset += copy; @@ -2563,8 +2525,9 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, if (err) goto out; + atomic_inc(&sk->sk_user_frags.urefs[token]); + atomic_long_inc(&niov->pp_ref_count); - tcp_xa_pool.netmems[tcp_xa_pool.idx++] = skb_frag_netmem(frag); sent += copy; @@ -2574,7 +2537,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, start = end; } - tcp_xa_pool_commit(sk, &tcp_xa_pool); if (!remaining_len) goto out; @@ -2592,7 +2554,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, } out: - tcp_xa_pool_commit(sk, &tcp_xa_pool); if (!sent) sent = err; diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 1e58a8a9ff7a..bdcb8cc003af 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -87,6 +87,9 @@ #include <crypto/hash.h> #include <linux/scatterlist.h> +#include <linux/dma-buf.h> +#include "../core/devmem.h" + #include <trace/events/tcp.h> #ifdef CONFIG_TCP_MD5SIG @@ -2529,11 +2532,38 @@ static void tcp_md5sig_info_free_rcu(struct rcu_head *head) static void tcp_release_user_frags(struct sock *sk) { #ifdef CONFIG_PAGE_POOL - unsigned long index; - void *netmem; + struct net_devmem_dmabuf_binding *binding; + struct net_iov *niov; + unsigned int token; + netmem_ref netmem; + + if (!sk->sk_user_frags.urefs) + return; + + binding = sk->sk_user_frags.binding; + if (!binding || !binding->vec) + return; + + for (token = 0; token < binding->dmabuf->size / PAGE_SIZE; token++) { + niov = binding->vec[token]; + + /* never used by recvmsg() */ + if (!niov) + continue; + + if (!net_is_devmem_iov(niov)) + continue; + + netmem = net_iov_to_netmem(niov); - xa_for_each(&sk->sk_user_frags, index, netmem) - WARN_ON_ONCE(!napi_pp_put_page((__force netmem_ref)netmem)); + while (atomic_dec_return(&sk->sk_user_frags.urefs[token]) >= 0) + WARN_ON_ONCE(!napi_pp_put_page(netmem)); + } + + net_devmem_dmabuf_binding_put(binding); + sk->sk_user_frags.binding = NULL; + kvfree(sk->sk_user_frags.urefs); + sk->sk_user_frags.urefs = NULL; #endif } @@ -2543,8 +2573,6 @@ void tcp_v4_destroy_sock(struct sock *sk) tcp_release_user_frags(sk); - xa_destroy(&sk->sk_user_frags); - trace_tcp_destroy_sock(sk); tcp_clear_xmit_timers(sk); diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index d1c9e4088646..4e8ea73daab7 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -639,8 +639,6 @@ struct sock *tcp_create_openreq_child(const struct sock *sk, __TCP_INC_STATS(sock_net(sk), TCP_MIB_PASSIVEOPENS); - xa_init_flags(&newsk->sk_user_frags, XA_FLAGS_ALLOC1); - return newsk; } EXPORT_SYMBOL(tcp_create_openreq_child); -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net: devmem: use niov array for token management 2025-09-02 21:36 ` [PATCH net-next 2/2] net: devmem: use niov array for token management Bobby Eshleman @ 2025-09-03 10:50 ` kernel test robot 2025-09-03 20:20 ` Mina Almasry 1 sibling, 0 replies; 7+ messages in thread From: kernel test robot @ 2025-09-03 10:50 UTC (permalink / raw) To: Bobby Eshleman, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell, David Ahern Cc: oe-kbuild-all, netdev, linux-kernel, Stanislav Fomichev, Mina Almasry, Bobby Eshleman Hi Bobby, kernel test robot noticed the following build errors: [auto build test ERROR on cd8a4cfa6bb43a441901e82f5c222dddc75a18a3] url: https://github.com/intel-lab-lkp/linux/commits/Bobby-Eshleman/net-devmem-rename-tx_vec-to-vec-in-dmabuf-binding/20250903-054553 base: cd8a4cfa6bb43a441901e82f5c222dddc75a18a3 patch link: https://lore.kernel.org/r/20250902-scratch-bobbyeshleman-devmem-tcp-token-upstream-v1-2-d946169b5550%40meta.com patch subject: [PATCH net-next 2/2] net: devmem: use niov array for token management config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250903/202509031855.54vuvsX1-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-12) 11.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250903/202509031855.54vuvsX1-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202509031855.54vuvsX1-lkp@intel.com/ All errors (new ones prefixed by >>): net/ipv4/tcp.c: In function 'tcp_recvmsg_dmabuf': >> net/ipv4/tcp.c:2502:41: error: implicit declaration of function 'net_devmem_dmabuf_binding_get'; did you mean 'net_devmem_dmabuf_binding_put'? [-Werror=implicit-function-declaration] 2502 | net_devmem_dmabuf_binding_get(binding); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net_devmem_dmabuf_binding_put cc1: some warnings being treated as errors vim +2502 net/ipv4/tcp.c 2390 2391 /* On error, returns the -errno. On success, returns number of bytes sent to the 2392 * user. May not consume all of @remaining_len. 2393 */ 2394 static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, 2395 unsigned int offset, struct msghdr *msg, 2396 int remaining_len) 2397 { 2398 struct dmabuf_cmsg dmabuf_cmsg = { 0 }; 2399 unsigned int start; 2400 int i, copy, n; 2401 int sent = 0; 2402 int err = 0; 2403 2404 do { 2405 start = skb_headlen(skb); 2406 2407 if (skb_frags_readable(skb)) { 2408 err = -ENODEV; 2409 goto out; 2410 } 2411 2412 /* Copy header. */ 2413 copy = start - offset; 2414 if (copy > 0) { 2415 copy = min(copy, remaining_len); 2416 2417 n = copy_to_iter(skb->data + offset, copy, 2418 &msg->msg_iter); 2419 if (n != copy) { 2420 err = -EFAULT; 2421 goto out; 2422 } 2423 2424 offset += copy; 2425 remaining_len -= copy; 2426 2427 /* First a dmabuf_cmsg for # bytes copied to user 2428 * buffer. 2429 */ 2430 memset(&dmabuf_cmsg, 0, sizeof(dmabuf_cmsg)); 2431 dmabuf_cmsg.frag_size = copy; 2432 err = put_cmsg_notrunc(msg, SOL_SOCKET, 2433 SO_DEVMEM_LINEAR, 2434 sizeof(dmabuf_cmsg), 2435 &dmabuf_cmsg); 2436 if (err) 2437 goto out; 2438 2439 sent += copy; 2440 2441 if (remaining_len == 0) 2442 goto out; 2443 } 2444 2445 /* after that, send information of dmabuf pages through a 2446 * sequence of cmsg 2447 */ 2448 for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { 2449 skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; 2450 struct net_devmem_dmabuf_binding *binding; 2451 struct net_iov *niov; 2452 u64 frag_offset; 2453 size_t size; 2454 u32 token; 2455 int end; 2456 2457 /* !skb_frags_readable() should indicate that ALL the 2458 * frags in this skb are dmabuf net_iovs. We're checking 2459 * for that flag above, but also check individual frags 2460 * here. If the tcp stack is not setting 2461 * skb_frags_readable() correctly, we still don't want 2462 * to crash here. 2463 */ 2464 if (!skb_frag_net_iov(frag)) { 2465 net_err_ratelimited("Found non-dmabuf skb with net_iov"); 2466 err = -ENODEV; 2467 goto out; 2468 } 2469 2470 niov = skb_frag_net_iov(frag); 2471 if (!net_is_devmem_iov(niov)) { 2472 err = -ENODEV; 2473 goto out; 2474 } 2475 2476 end = start + skb_frag_size(frag); 2477 copy = end - offset; 2478 2479 if (copy > 0) { 2480 copy = min(copy, remaining_len); 2481 2482 frag_offset = net_iov_virtual_addr(niov) + 2483 skb_frag_off(frag) + offset - 2484 start; 2485 dmabuf_cmsg.frag_offset = frag_offset; 2486 dmabuf_cmsg.frag_size = copy; 2487 2488 binding = net_devmem_iov_binding(niov); 2489 2490 if (!sk->sk_user_frags.binding) { 2491 sk->sk_user_frags.binding = binding; 2492 2493 size = binding->dmabuf->size / PAGE_SIZE; 2494 sk->sk_user_frags.urefs = kzalloc(size, 2495 GFP_KERNEL); 2496 if (!sk->sk_user_frags.urefs) { 2497 sk->sk_user_frags.binding = NULL; 2498 err = -ENOMEM; 2499 goto out; 2500 } 2501 > 2502 net_devmem_dmabuf_binding_get(binding); 2503 } 2504 2505 if (WARN_ONCE(sk->sk_user_frags.binding != binding, 2506 "binding changed for devmem socket")) { 2507 err = -EFAULT; 2508 goto out; 2509 } 2510 2511 token = net_iov_virtual_addr(niov) >> PAGE_SHIFT; 2512 binding->vec[token] = niov; 2513 dmabuf_cmsg.frag_token = token; 2514 2515 /* Will perform the exchange later */ 2516 dmabuf_cmsg.dmabuf_id = net_devmem_iov_binding_id(niov); 2517 2518 offset += copy; 2519 remaining_len -= copy; 2520 2521 err = put_cmsg_notrunc(msg, SOL_SOCKET, 2522 SO_DEVMEM_DMABUF, 2523 sizeof(dmabuf_cmsg), 2524 &dmabuf_cmsg); 2525 if (err) 2526 goto out; 2527 2528 atomic_inc(&sk->sk_user_frags.urefs[token]); 2529 2530 atomic_long_inc(&niov->pp_ref_count); 2531 2532 sent += copy; 2533 2534 if (remaining_len == 0) 2535 goto out; 2536 } 2537 start = end; 2538 } 2539 2540 if (!remaining_len) 2541 goto out; 2542 2543 /* if remaining_len is not satisfied yet, we need to go to the 2544 * next frag in the frag_list to satisfy remaining_len. 2545 */ 2546 skb = skb_shinfo(skb)->frag_list ?: skb->next; 2547 2548 offset = offset - start; 2549 } while (skb); 2550 2551 if (remaining_len) { 2552 err = -EFAULT; 2553 goto out; 2554 } 2555 2556 out: 2557 if (!sent) 2558 sent = err; 2559 2560 return sent; 2561 } 2562 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net: devmem: use niov array for token management 2025-09-02 21:36 ` [PATCH net-next 2/2] net: devmem: use niov array for token management Bobby Eshleman 2025-09-03 10:50 ` kernel test robot @ 2025-09-03 20:20 ` Mina Almasry 2025-09-04 0:15 ` Bobby Eshleman 1 sibling, 1 reply; 7+ messages in thread From: Mina Almasry @ 2025-09-03 20:20 UTC (permalink / raw) To: Bobby Eshleman Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell, David Ahern, netdev, linux-kernel, Stanislav Fomichev, Bobby Eshleman On Tue, Sep 2, 2025 at 2:36 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote: > > From: Bobby Eshleman <bobbyeshleman@meta.com> > > Improve CPU performance of devmem token management by using page offsets > as dmabuf tokens and using them for direct array access lookups instead > of xarray lookups. Consequently, the xarray can be removed. The result > is an average 5% reduction in CPU cycles spent by devmem RX user > threads. > Great! > This patch changes the meaning of tokens. Tokens previously referred to > unique fragments of pages. In this patch tokens instead represent > references to pages, not fragments. Because of this, multiple tokens > may refer to the same page and so have identical value (e.g., two small > fragments may coexist on the same page). The token and offset pair that > the user receives uniquely identifies fragments if needed. This assumes > that the user is not attempting to sort / uniq the token list using > tokens alone. > > A new restriction is added to the implementation: devmem RX sockets > cannot switch dmabuf bindings. In practice, this is a symptom of invalid > configuration as a flow would have to be steered to a different queue or > device where there is a different binding, which is generally bad for > TCP flows. Please do not assume configurations you don't use/care about are invalid. Currently reconfiguring flow steering while a flow is active works as intended today. This is a regression that needs to be resolved. But more importantly, looking at your code, I don't think this is a restriction you need to introduce? > This restriction is necessary because the 32-bit dmabuf token > does not have enough bits to represent both the pages in a large dmabuf > and also a binding or dmabuf ID. For example, a system with 8 NICs and > 32 queues requires 8 bits for a binding / queue ID (8 NICs * 32 queues > == 256 queues total == 2^8), which leaves only 24 bits for dmabuf pages > (2^24 * 4096 / (1<<30) == 64GB). This is insufficient for the device and > queue numbers on many current systems or systems that may need larger > GPU dmabufs (as for hard limits, my current H100 has 80GB GPU memory per > device). > > Using kperf[1] with 4 flows and workers, this patch improves receive > worker CPU util by ~4.9% with slightly better throughput. > > Before, mean cpu util for rx workers ~83.6%: > > Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle > Average: 4 2.30 0.00 79.43 0.00 0.65 0.21 0.00 0.00 0.00 17.41 > Average: 5 2.27 0.00 80.40 0.00 0.45 0.21 0.00 0.00 0.00 16.67 > Average: 6 2.28 0.00 80.47 0.00 0.46 0.25 0.00 0.00 0.00 16.54 > Average: 7 2.42 0.00 82.05 0.00 0.46 0.21 0.00 0.00 0.00 14.86 > > After, mean cpu util % for rx workers ~78.7%: > > Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle > Average: 4 2.61 0.00 73.31 0.00 0.76 0.11 0.00 0.00 0.00 23.20 > Average: 5 2.95 0.00 74.24 0.00 0.66 0.22 0.00 0.00 0.00 21.94 > Average: 6 2.81 0.00 73.38 0.00 0.97 0.11 0.00 0.00 0.00 22.73 > Average: 7 3.05 0.00 78.76 0.00 0.76 0.11 0.00 0.00 0.00 17.32 > I think effectively all you're doing in this patch is removing xarray with a regular array, right? I'm surprised an xarray account for 5% cpu utilization. I wonder if you have debug configs turned on during these experiments. Can you perf trace what about the xarray is taking so long? I wonder if we're just using xarrays improperly (maybe hitting constant resizing slow paths or something), and a similar improvement can be gotten by adjusting the xarray flags or what not. > Mean throughput improves, but falls within a standard deviation (~45GB/s > for 4 flows on a 50GB/s NIC, one hop). > > This patch adds an array of atomics for counting the tokens returned to > the user for a given page. There is a 4-byte atomic per page in the > dmabuf per socket. Given a 2GB dmabuf, this array is 2MB. > > [1]: https://github.com/facebookexperimental/kperf > > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com> > --- > include/net/sock.h | 5 ++- > net/core/devmem.c | 17 ++++---- > net/core/devmem.h | 2 +- > net/core/sock.c | 24 +++++++---- > net/ipv4/tcp.c | 107 +++++++++++++++-------------------------------- > net/ipv4/tcp_ipv4.c | 40 +++++++++++++++--- > net/ipv4/tcp_minisocks.c | 2 - > 7 files changed, 99 insertions(+), 98 deletions(-) > > diff --git a/include/net/sock.h b/include/net/sock.h > index 1e7f124871d2..70c97880229d 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -573,7 +573,10 @@ struct sock { > #endif > struct rcu_head sk_rcu; > netns_tracker ns_tracker; > - struct xarray sk_user_frags; > + struct { > + struct net_devmem_dmabuf_binding *binding; > + atomic_t *urefs; > + } sk_user_frags; > AFAIU, if you made sk_user_frags an array of (unref, binding) tuples instead of just an array of urefs then you can remove the single-binding restriction. Although, I wonder what happens if the socket receives the netmem at the same index on 2 different dmabufs. At that point I assume the wrong uref gets incremented? :( One way or another the single-binding restriction needs to be removed I think. It's regressing a UAPI that currently works. > #if IS_ENABLED(CONFIG_PROVE_LOCKING) && IS_ENABLED(CONFIG_MODULES) > struct module *sk_owner; > diff --git a/net/core/devmem.c b/net/core/devmem.c > index b4c570d4f37a..50e92dcf5bf1 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -187,6 +187,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, > struct dma_buf *dmabuf; > unsigned int sg_idx, i; > unsigned long virtual; > + gfp_t flags; > int err; > > if (!dma_dev) { > @@ -230,14 +231,14 @@ net_devmem_bind_dmabuf(struct net_device *dev, > goto err_detach; > } > > - if (direction == DMA_TO_DEVICE) { > - binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, > - sizeof(struct net_iov *), > - GFP_KERNEL); > - if (!binding->vec) { > - err = -ENOMEM; > - goto err_unmap; > - } > + flags = (direction == DMA_FROM_DEVICE) ? __GFP_ZERO : 0; > + Why not pass __GFP_ZERO unconditionally? > + binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, > + sizeof(struct net_iov *), > + GFP_KERNEL | flags); > + if (!binding->vec) { > + err = -ENOMEM; > + goto err_unmap; > } > > /* For simplicity we expect to make PAGE_SIZE allocations, but the > diff --git a/net/core/devmem.h b/net/core/devmem.h > index 2ada54fb63d7..d4eb28d079bb 100644 > --- a/net/core/devmem.h > +++ b/net/core/devmem.h > @@ -61,7 +61,7 @@ struct net_devmem_dmabuf_binding { > > /* Array of net_iov pointers for this binding, sorted by virtual > * address. This array is convenient to map the virtual addresses to > - * net_iovs in the TX path. > + * net_iovs. > */ > struct net_iov **vec; > > diff --git a/net/core/sock.c b/net/core/sock.c > index 9a8290fcc35d..3a5cb4e10519 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -87,6 +87,7 @@ > > #include <linux/unaligned.h> > #include <linux/capability.h> > +#include <linux/dma-buf.h> > #include <linux/errno.h> > #include <linux/errqueue.h> > #include <linux/types.h> > @@ -151,6 +152,7 @@ > #include <uapi/linux/pidfd.h> > > #include "dev.h" > +#include "devmem.h" > > static DEFINE_MUTEX(proto_list_mutex); > static LIST_HEAD(proto_list); > @@ -1100,32 +1102,40 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) > return -EFAULT; > } > > - xa_lock_bh(&sk->sk_user_frags); > for (i = 0; i < num_tokens; i++) { > for (j = 0; j < tokens[i].token_count; j++) { > + struct net_iov *niov; > + unsigned int token; > + netmem_ref netmem; > + > + token = tokens[i].token_start + j; > + if (WARN_ONCE(token >= sk->sk_user_frags.binding->dmabuf->size / PAGE_SIZE, > + "invalid token passed from user")) > + break; WARNs on invalid user behavior are a non-starter AFAIU. For one, syzbot trivially reproduces them and files a bug. Please remove all of them. pr_err may be acceptable for extremely bad errors. Invalid user input is not worthy of WARN or pr_err. > + > if (++num_frags > MAX_DONTNEED_FRAGS) > goto frag_limit_reached; > - > - netmem_ref netmem = (__force netmem_ref)__xa_erase( > - &sk->sk_user_frags, tokens[i].token_start + j); > + niov = sk->sk_user_frags.binding->vec[token]; > + netmem = net_iov_to_netmem(niov); So token is the index to both vec and sk->sk_user_frags.binding->vec? xarrays are a resizable array. AFAIU what you're doing abstractly here is replacing a resizable array with an array of max size, no? (I didn't read too closely yet, I may be missing something). Which makes me think either due to a bug or due to specifics of your setup, xarray is unreasonably expensive. Without investigating the details I wonder if we're constantly running into a resizing slowpath in xarray code and I think this needs some investigation. > > if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem))) > continue; > > + if (WARN_ONCE(atomic_dec_if_positive(&sk->sk_user_frags.urefs[token]) > + < 0, "user released token too many times")) Here and everywhere, please remove the WARNs for weird user behavior. > + continue; > + > netmems[netmem_num++] = netmem; > if (netmem_num == ARRAY_SIZE(netmems)) { > - xa_unlock_bh(&sk->sk_user_frags); > for (k = 0; k < netmem_num; k++) > WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); > netmem_num = 0; > - xa_lock_bh(&sk->sk_user_frags); You remove the locking but it's not clear to me why we don't need it anymore. What's stopping 2 dontneeds from freeing at the same time? I'm guessing it's because urefs are atomic so we don't need any extra sync? > } > ret++; > } > } > > frag_limit_reached: > - xa_unlock_bh(&sk->sk_user_frags); > for (k = 0; k < netmem_num; k++) > WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 40b774b4f587..585b50fa8c00 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -261,6 +261,7 @@ > #include <linux/memblock.h> > #include <linux/highmem.h> > #include <linux/cache.h> > +#include <linux/dma-buf.h> > #include <linux/err.h> > #include <linux/time.h> > #include <linux/slab.h> > @@ -475,7 +476,8 @@ void tcp_init_sock(struct sock *sk) > > set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags); > sk_sockets_allocated_inc(sk); > - xa_init_flags(&sk->sk_user_frags, XA_FLAGS_ALLOC1); > + sk->sk_user_frags.binding = NULL; > + sk->sk_user_frags.urefs = NULL; > } > EXPORT_IPV6_MOD(tcp_init_sock); > > @@ -2386,68 +2388,6 @@ static int tcp_inq_hint(struct sock *sk) > return inq; > } > > -/* batch __xa_alloc() calls and reduce xa_lock()/xa_unlock() overhead. */ > -struct tcp_xa_pool { > - u8 max; /* max <= MAX_SKB_FRAGS */ > - u8 idx; /* idx <= max */ > - __u32 tokens[MAX_SKB_FRAGS]; > - netmem_ref netmems[MAX_SKB_FRAGS]; > -}; > - > -static void tcp_xa_pool_commit_locked(struct sock *sk, struct tcp_xa_pool *p) > -{ > - int i; > - > - /* Commit part that has been copied to user space. */ > - for (i = 0; i < p->idx; i++) > - __xa_cmpxchg(&sk->sk_user_frags, p->tokens[i], XA_ZERO_ENTRY, > - (__force void *)p->netmems[i], GFP_KERNEL); > - /* Rollback what has been pre-allocated and is no longer needed. */ > - for (; i < p->max; i++) > - __xa_erase(&sk->sk_user_frags, p->tokens[i]); > - > - p->max = 0; > - p->idx = 0; > -} > - > -static void tcp_xa_pool_commit(struct sock *sk, struct tcp_xa_pool *p) > -{ > - if (!p->max) > - return; > - > - xa_lock_bh(&sk->sk_user_frags); > - > - tcp_xa_pool_commit_locked(sk, p); > - > - xa_unlock_bh(&sk->sk_user_frags); > -} > - > -static int tcp_xa_pool_refill(struct sock *sk, struct tcp_xa_pool *p, > - unsigned int max_frags) > -{ > - int err, k; > - > - if (p->idx < p->max) > - return 0; > - > - xa_lock_bh(&sk->sk_user_frags); > - > - tcp_xa_pool_commit_locked(sk, p); > - > - for (k = 0; k < max_frags; k++) { > - err = __xa_alloc(&sk->sk_user_frags, &p->tokens[k], > - XA_ZERO_ENTRY, xa_limit_31b, GFP_KERNEL); > - if (err) > - break; > - } > - > - xa_unlock_bh(&sk->sk_user_frags); > - > - p->max = k; > - p->idx = 0; > - return k ? 0 : err; > -} > - > /* On error, returns the -errno. On success, returns number of bytes sent to the > * user. May not consume all of @remaining_len. > */ > @@ -2456,14 +2396,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > int remaining_len) > { > struct dmabuf_cmsg dmabuf_cmsg = { 0 }; > - struct tcp_xa_pool tcp_xa_pool; > unsigned int start; > int i, copy, n; > int sent = 0; > int err = 0; > > - tcp_xa_pool.max = 0; > - tcp_xa_pool.idx = 0; > do { > start = skb_headlen(skb); > > @@ -2510,8 +2447,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > */ > for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; > + struct net_devmem_dmabuf_binding *binding; > struct net_iov *niov; > u64 frag_offset; > + size_t size; > + u32 token; > int end; > > /* !skb_frags_readable() should indicate that ALL the > @@ -2544,13 +2484,35 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > start; > dmabuf_cmsg.frag_offset = frag_offset; > dmabuf_cmsg.frag_size = copy; > - err = tcp_xa_pool_refill(sk, &tcp_xa_pool, > - skb_shinfo(skb)->nr_frags - i); > - if (err) > + > + binding = net_devmem_iov_binding(niov); > + > + if (!sk->sk_user_frags.binding) { > + sk->sk_user_frags.binding = binding; > + > + size = binding->dmabuf->size / PAGE_SIZE; > + sk->sk_user_frags.urefs = kzalloc(size, > + GFP_KERNEL); > + if (!sk->sk_user_frags.urefs) { > + sk->sk_user_frags.binding = NULL; > + err = -ENOMEM; > + goto out; > + } > + > + net_devmem_dmabuf_binding_get(binding); It's not clear to me why we need to get the binding. AFAIR the way it works is that we grab a reference on the net_iov, which guarantees that the associated pp is alive, which in turn guarantees that the binding remains alive and we don't need to get the binding for every frag. > + } > + > + if (WARN_ONCE(sk->sk_user_frags.binding != binding, > + "binding changed for devmem socket")) { Remove WARN_ONCE. It's not reasonable to kernel splat because the user reconfigured flow steering me thinks. > + err = -EFAULT; > goto out; > + } > + > + token = net_iov_virtual_addr(niov) >> PAGE_SHIFT; > + binding->vec[token] = niov; I don't think you can do this? I thought vec[token] was already == niov. binding->vec should be initialized when teh binding is initialized, not re-written on every recvmsg, no? > + dmabuf_cmsg.frag_token = token; > > /* Will perform the exchange later */ > - dmabuf_cmsg.frag_token = tcp_xa_pool.tokens[tcp_xa_pool.idx]; > dmabuf_cmsg.dmabuf_id = net_devmem_iov_binding_id(niov); > > offset += copy; > @@ -2563,8 +2525,9 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > if (err) > goto out; > > + atomic_inc(&sk->sk_user_frags.urefs[token]); > + > atomic_long_inc(&niov->pp_ref_count); > - tcp_xa_pool.netmems[tcp_xa_pool.idx++] = skb_frag_netmem(frag); > > sent += copy; > > @@ -2574,7 +2537,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > start = end; > } > > - tcp_xa_pool_commit(sk, &tcp_xa_pool); > if (!remaining_len) > goto out; > > @@ -2592,7 +2554,6 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > } > > out: > - tcp_xa_pool_commit(sk, &tcp_xa_pool); > if (!sent) > sent = err; > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 1e58a8a9ff7a..bdcb8cc003af 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -87,6 +87,9 @@ > #include <crypto/hash.h> > #include <linux/scatterlist.h> > > +#include <linux/dma-buf.h> > +#include "../core/devmem.h" > + > #include <trace/events/tcp.h> > > #ifdef CONFIG_TCP_MD5SIG > @@ -2529,11 +2532,38 @@ static void tcp_md5sig_info_free_rcu(struct rcu_head *head) > static void tcp_release_user_frags(struct sock *sk) > { > #ifdef CONFIG_PAGE_POOL > - unsigned long index; > - void *netmem; > + struct net_devmem_dmabuf_binding *binding; > + struct net_iov *niov; > + unsigned int token; > + netmem_ref netmem; > + > + if (!sk->sk_user_frags.urefs) > + return; > + > + binding = sk->sk_user_frags.binding; > + if (!binding || !binding->vec) > + return; > + > + for (token = 0; token < binding->dmabuf->size / PAGE_SIZE; token++) { > + niov = binding->vec[token]; > + > + /* never used by recvmsg() */ > + if (!niov) > + continue; > + > + if (!net_is_devmem_iov(niov)) > + continue; > + > + netmem = net_iov_to_netmem(niov); > > - xa_for_each(&sk->sk_user_frags, index, netmem) > - WARN_ON_ONCE(!napi_pp_put_page((__force netmem_ref)netmem)); > + while (atomic_dec_return(&sk->sk_user_frags.urefs[token]) >= 0) > + WARN_ON_ONCE(!napi_pp_put_page(netmem)); > + } > + > + net_devmem_dmabuf_binding_put(binding); > + sk->sk_user_frags.binding = NULL; > + kvfree(sk->sk_user_frags.urefs); > + sk->sk_user_frags.urefs = NULL; > #endif > } > > @@ -2543,8 +2573,6 @@ void tcp_v4_destroy_sock(struct sock *sk) > > tcp_release_user_frags(sk); > > - xa_destroy(&sk->sk_user_frags); > - > trace_tcp_destroy_sock(sk); > > tcp_clear_xmit_timers(sk); > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > index d1c9e4088646..4e8ea73daab7 100644 > --- a/net/ipv4/tcp_minisocks.c > +++ b/net/ipv4/tcp_minisocks.c > @@ -639,8 +639,6 @@ struct sock *tcp_create_openreq_child(const struct sock *sk, > > __TCP_INC_STATS(sock_net(sk), TCP_MIB_PASSIVEOPENS); > > - xa_init_flags(&newsk->sk_user_frags, XA_FLAGS_ALLOC1); > - > return newsk; > } > EXPORT_SYMBOL(tcp_create_openreq_child); > > -- > 2.47.3 > -- Thanks, Mina ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net: devmem: use niov array for token management 2025-09-03 20:20 ` Mina Almasry @ 2025-09-04 0:15 ` Bobby Eshleman 0 siblings, 0 replies; 7+ messages in thread From: Bobby Eshleman @ 2025-09-04 0:15 UTC (permalink / raw) To: Mina Almasry Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Willem de Bruijn, Neal Cardwell, David Ahern, netdev, linux-kernel, Stanislav Fomichev, Bobby Eshleman On Wed, Sep 03, 2025 at 01:20:57PM -0700, Mina Almasry wrote: > On Tue, Sep 2, 2025 at 2:36 PM Bobby Eshleman <bobbyeshleman@gmail.com> wrote: > > > > From: Bobby Eshleman <bobbyeshleman@meta.com> > > > > Improve CPU performance of devmem token management by using page offsets > > as dmabuf tokens and using them for direct array access lookups instead > > of xarray lookups. Consequently, the xarray can be removed. The result > > is an average 5% reduction in CPU cycles spent by devmem RX user > > threads. > > > > Great! > Hey Mina, thanks for the feedback! > > This patch changes the meaning of tokens. Tokens previously referred to > > unique fragments of pages. In this patch tokens instead represent > > references to pages, not fragments. Because of this, multiple tokens > > may refer to the same page and so have identical value (e.g., two small > > fragments may coexist on the same page). The token and offset pair that > > the user receives uniquely identifies fragments if needed. This assumes > > that the user is not attempting to sort / uniq the token list using > > tokens alone. > > > > A new restriction is added to the implementation: devmem RX sockets > > cannot switch dmabuf bindings. In practice, this is a symptom of invalid > > configuration as a flow would have to be steered to a different queue or > > device where there is a different binding, which is generally bad for > > TCP flows. > > Please do not assume configurations you don't use/care about are > invalid. Currently reconfiguring flow steering while a flow is active > works as intended today. This is a regression that needs to be > resolved. But more importantly, looking at your code, I don't think > this is a restriction you need to introduce? > That's fair, let's see if we can lift it. > > This restriction is necessary because the 32-bit dmabuf token > > does not have enough bits to represent both the pages in a large dmabuf > > and also a binding or dmabuf ID. For example, a system with 8 NICs and > > 32 queues requires 8 bits for a binding / queue ID (8 NICs * 32 queues > > == 256 queues total == 2^8), which leaves only 24 bits for dmabuf pages > > (2^24 * 4096 / (1<<30) == 64GB). This is insufficient for the device and > > queue numbers on many current systems or systems that may need larger > > GPU dmabufs (as for hard limits, my current H100 has 80GB GPU memory per > > device). > > > > Using kperf[1] with 4 flows and workers, this patch improves receive > > worker CPU util by ~4.9% with slightly better throughput. > > > > Before, mean cpu util for rx workers ~83.6%: > > > > Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle > > Average: 4 2.30 0.00 79.43 0.00 0.65 0.21 0.00 0.00 0.00 17.41 > > Average: 5 2.27 0.00 80.40 0.00 0.45 0.21 0.00 0.00 0.00 16.67 > > Average: 6 2.28 0.00 80.47 0.00 0.46 0.25 0.00 0.00 0.00 16.54 > > Average: 7 2.42 0.00 82.05 0.00 0.46 0.21 0.00 0.00 0.00 14.86 > > > > After, mean cpu util % for rx workers ~78.7%: > > > > Average: CPU %usr %nice %sys %iowait %irq %soft %steal %guest %gnice %idle > > Average: 4 2.61 0.00 73.31 0.00 0.76 0.11 0.00 0.00 0.00 23.20 > > Average: 5 2.95 0.00 74.24 0.00 0.66 0.22 0.00 0.00 0.00 21.94 > > Average: 6 2.81 0.00 73.38 0.00 0.97 0.11 0.00 0.00 0.00 22.73 > > Average: 7 3.05 0.00 78.76 0.00 0.76 0.11 0.00 0.00 0.00 17.32 > > > > I think effectively all you're doing in this patch is removing xarray > with a regular array, right? I'm surprised an xarray account for 5% > cpu utilization. I wonder if you have debug configs turned on during > these experiments. Can you perf trace what about the xarray is taking > so long? I wonder if we're just using xarrays improperly (maybe > hitting constant resizing slow paths or something), and a similar > improvement can be gotten by adjusting the xarray flags or what not. > That is right. Here is some perf data gathered from: perf record -a -g -F 99 -C 0-7 -- sleep 5 RX queues pinned to 0-3 and kperf server pinned to 4-7. 11.25% server [kernel.kallsyms] [k] tcp_recvmsg | --10.98%--tcp_recvmsg bpf_trampoline_6442594803 tcp_recvmsg inet6_recvmsg ____sys_recvmsg ___sys_recvmsg __x64_sys_recvmsg do_syscall_64 entry_SYSCALL_64_after_hwframe __libc_recvmsg | --2.74%--0x100000082 5.65% server [kernel.kallsyms] [k] xas_store | --5.63%--xas_store | |--3.92%--__xa_erase | sock_devmem_dontneed | sk_setsockopt | __x64_sys_setsockopt | do_syscall_64 | entry_SYSCALL_64_after_hwframe | __GI___setsockopt | 0x4f00000001 | |--0.94%--__xa_alloc | tcp_recvmsg | bpf_trampoline_6442594803 | tcp_recvmsg | inet6_recvmsg | ____sys_recvmsg | ___sys_recvmsg | __x64_sys_recvmsg | do_syscall_64 | entry_SYSCALL_64_after_hwframe | __libc_recvmsg | --0.76%--__xa_cmpxchg tcp_xa_pool_commit_locked tcp_xa_pool_commit tcp_recvmsg bpf_trampoline_6442594803 tcp_recvmsg inet6_recvmsg ____sys_recvmsg ___sys_recvmsg __x64_sys_recvmsg do_syscall_64 entry_SYSCALL_64_after_hwframe __libc_recvmsg [...] 1.22% server [kernel.kallsyms] [k] xas_find_marked | --1.19%--xas_find_marked __xa_alloc tcp_recvmsg bpf_trampoline_6442594803 tcp_recvmsg inet6_recvmsg ____sys_recvmsg ___sys_recvmsg __x64_sys_recvmsg do_syscall_64 entry_SYSCALL_64_after_hwframe __libc_recvmsg Here is the output from zcat /proc/config.gz | grep DEBUG | grep =y, I'm not 100% sure which may be worth toggling. I'm happy to rerun the experiments if any of these are suspicious looking: CONFIG_X86_DEBUGCTLMSR=y CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y CONFIG_BLK_DEBUG_FS=y CONFIG_BFQ_CGROUP_DEBUG=y CONFIG_CMA_DEBUGFS=y CONFIG_FW_LOADER_DEBUG=y CONFIG_PNP_DEBUG_MESSAGES=y CONFIG_SCSI_LPFC_DEBUG_FS=y CONFIG_MLX4_DEBUG=y CONFIG_INFINIBAND_MTHCA_DEBUG=y CONFIG_NFS_DEBUG=y CONFIG_SUNRPC_DEBUG=y CONFIG_DYNAMIC_DEBUG=y CONFIG_DYNAMIC_DEBUG_CORE=y CONFIG_DEBUG_BUGVERBOSE=y CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_MISC=y CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_DWARF4=y CONFIG_DEBUG_INFO_COMPRESSED_NONE=y CONFIG_DEBUG_INFO_BTF=y CONFIG_DEBUG_INFO_BTF_MODULES=y CONFIG_DEBUG_FS=y CONFIG_DEBUG_FS_ALLOW_ALL=y CONFIG_SLUB_DEBUG=y CONFIG_DEBUG_PAGE_REF=y CONFIG_ARCH_HAS_DEBUG_WX=y CONFIG_HAVE_DEBUG_KMEMLEAK=y CONFIG_ARCH_HAS_DEBUG_VM_PGTABLE=y CONFIG_ARCH_HAS_DEBUG_VIRTUAL=y CONFIG_DEBUG_MEMORY_INIT=y CONFIG_SCHED_DEBUG=y CONFIG_LOCK_DEBUGGING_SUPPORT=y CONFIG_CSD_LOCK_WAIT_DEBUG=y CONFIG_CSD_LOCK_WAIT_DEBUG_DEFAULT=y CONFIG_DEBUG_CGROUP_REF=y CONFIG_FAULT_INJECTION_DEBUG_FS=y For more context, I did a few other experiments before eventually landing on this patch: 1) hacky approach - don't do any token management at all, replace dmabuf token with 64-bit pointer to niov, teach recvmsg() / sock_devmem_dontneed() to inc/dec the niov reference directly... this actually reduced the CPU util by a very consistent 10%+ per worker (the largest delta of all my experiements). 2) keep xarray, but use RCU/lockless lookups in both recvmsg/dontneed. Use page indices + xa_insert instead of xa_alloc. Acquire lock only if lockless lookup returns null in recvmsg path. Don't erase in dontneed, only take rcu_read_lock() and do lookup. Let xarray grow according to usage and cleanup when the socket is destroyed. Surprisingly, this didn't offer noticeable improvement. 3) use normal array but no atomics -- incorrect, but saw good improvement, despite possibility of leaked references. 4) use a hashmap + bucket locks instead of xarray -- performed way worse than xarray, no change to token > > Mean throughput improves, but falls within a standard deviation (~45GB/s > > for 4 flows on a 50GB/s NIC, one hop). > > > > This patch adds an array of atomics for counting the tokens returned to > > the user for a given page. There is a 4-byte atomic per page in the > > dmabuf per socket. Given a 2GB dmabuf, this array is 2MB. > > > > [1]: https://github.com/facebookexperimental/kperf > > > > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com> > > --- > > include/net/sock.h | 5 ++- > > net/core/devmem.c | 17 ++++---- > > net/core/devmem.h | 2 +- > > net/core/sock.c | 24 +++++++---- > > net/ipv4/tcp.c | 107 +++++++++++++++-------------------------------- > > net/ipv4/tcp_ipv4.c | 40 +++++++++++++++--- > > net/ipv4/tcp_minisocks.c | 2 - > > 7 files changed, 99 insertions(+), 98 deletions(-) > > > > diff --git a/include/net/sock.h b/include/net/sock.h > > index 1e7f124871d2..70c97880229d 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -573,7 +573,10 @@ struct sock { > > #endif > > struct rcu_head sk_rcu; > > netns_tracker ns_tracker; > > - struct xarray sk_user_frags; > > + struct { > > + struct net_devmem_dmabuf_binding *binding; > > + atomic_t *urefs; > > + } sk_user_frags; > > > > AFAIU, if you made sk_user_frags an array of (unref, binding) tuples > instead of just an array of urefs then you can remove the > single-binding restriction. > > Although, I wonder what happens if the socket receives the netmem at > the same index on 2 different dmabufs. At that point I assume the > wrong uref gets incremented? :( > Right. We need some bits to differentiate bindings. Here are some ideas I've had about this, I wonder what your thoughts are on them: 1) Encode a subset of bindings and wait for availability if the encoding space becomes exhausted. For example, we could encode the binding in 5 bits for outstanding references across 32 bindings and 27 bits (512 GB) of dmabuf. If recvmsg wants to return a reference to a 33rd binding, it waits until the user returns enough tokens to release one of the binding encoding bits (at which point it could be reused for the new reference). 2) opt into an extended token (dmabuf_token_v2) via sockopts, and add the binding ID or other needed information there. > One way or another the single-binding restriction needs to be removed > I think. It's regressing a UAPI that currently works. > > > #if IS_ENABLED(CONFIG_PROVE_LOCKING) && IS_ENABLED(CONFIG_MODULES) > > struct module *sk_owner; > > diff --git a/net/core/devmem.c b/net/core/devmem.c > > index b4c570d4f37a..50e92dcf5bf1 100644 > > --- a/net/core/devmem.c > > +++ b/net/core/devmem.c > > @@ -187,6 +187,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, > > struct dma_buf *dmabuf; > > unsigned int sg_idx, i; > > unsigned long virtual; > > + gfp_t flags; > > int err; > > > > if (!dma_dev) { > > @@ -230,14 +231,14 @@ net_devmem_bind_dmabuf(struct net_device *dev, > > goto err_detach; > > } > > > > - if (direction == DMA_TO_DEVICE) { > > - binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, > > - sizeof(struct net_iov *), > > - GFP_KERNEL); > > - if (!binding->vec) { > > - err = -ENOMEM; > > - goto err_unmap; > > - } > > + flags = (direction == DMA_FROM_DEVICE) ? __GFP_ZERO : 0; > > + > > Why not pass __GFP_ZERO unconditionally? > Seemed unnecessary for the TX side, though I see no problem adding it for the next rev. > > + binding->vec = kvmalloc_array(dmabuf->size / PAGE_SIZE, > > + sizeof(struct net_iov *), > > + GFP_KERNEL | flags); > > + if (!binding->vec) { > > + err = -ENOMEM; > > + goto err_unmap; > > } > > > > /* For simplicity we expect to make PAGE_SIZE allocations, but the > > diff --git a/net/core/devmem.h b/net/core/devmem.h > > index 2ada54fb63d7..d4eb28d079bb 100644 > > --- a/net/core/devmem.h > > +++ b/net/core/devmem.h > > @@ -61,7 +61,7 @@ struct net_devmem_dmabuf_binding { > > > > /* Array of net_iov pointers for this binding, sorted by virtual > > * address. This array is convenient to map the virtual addresses to > > - * net_iovs in the TX path. > > + * net_iovs. > > */ > > struct net_iov **vec; > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 9a8290fcc35d..3a5cb4e10519 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -87,6 +87,7 @@ > > > > #include <linux/unaligned.h> > > #include <linux/capability.h> > > +#include <linux/dma-buf.h> > > #include <linux/errno.h> > > #include <linux/errqueue.h> > > #include <linux/types.h> > > @@ -151,6 +152,7 @@ > > #include <uapi/linux/pidfd.h> > > > > #include "dev.h" > > +#include "devmem.h" > > > > static DEFINE_MUTEX(proto_list_mutex); > > static LIST_HEAD(proto_list); > > @@ -1100,32 +1102,40 @@ sock_devmem_dontneed(struct sock *sk, sockptr_t optval, unsigned int optlen) > > return -EFAULT; > > } > > > > - xa_lock_bh(&sk->sk_user_frags); > > for (i = 0; i < num_tokens; i++) { > > for (j = 0; j < tokens[i].token_count; j++) { > > + struct net_iov *niov; > > + unsigned int token; > > + netmem_ref netmem; > > + > > + token = tokens[i].token_start + j; > > + if (WARN_ONCE(token >= sk->sk_user_frags.binding->dmabuf->size / PAGE_SIZE, > > + "invalid token passed from user")) > > + break; > > WARNs on invalid user behavior are a non-starter AFAIU. For one, > syzbot trivially reproduces them and files a bug. Please remove all of > them. pr_err may be acceptable for extremely bad errors. Invalid user > input is not worthy of WARN or pr_err. > Gotcha, I did not know that. Sounds good to me. > > + > > if (++num_frags > MAX_DONTNEED_FRAGS) > > goto frag_limit_reached; > > - > > - netmem_ref netmem = (__force netmem_ref)__xa_erase( > > - &sk->sk_user_frags, tokens[i].token_start + j); > > + niov = sk->sk_user_frags.binding->vec[token]; > > + netmem = net_iov_to_netmem(niov); > > So token is the index to both vec and sk->sk_user_frags.binding->vec? > > xarrays are a resizable array. AFAIU what you're doing abstractly here > is replacing a resizable array with an array of max size, no? (I > didn't read too closely yet, I may be missing something). > Yes, 100%. > Which makes me think either due to a bug or due to specifics of your > setup, xarray is unreasonably expensive. Without investigating the > details I wonder if we're constantly running into a resizing slowpath > in xarray code and I think this needs some investigation. > Certainly possible, I found the result surprising myself, hence my experiment with a more read-mostly usage of xarray. > > > > if (!netmem || WARN_ON_ONCE(!netmem_is_net_iov(netmem))) > > continue; > > > > + if (WARN_ONCE(atomic_dec_if_positive(&sk->sk_user_frags.urefs[token]) > > + < 0, "user released token too many times")) > > Here and everywhere, please remove the WARNs for weird user behavior. > > > + continue; > > + > > netmems[netmem_num++] = netmem; > > if (netmem_num == ARRAY_SIZE(netmems)) { > > - xa_unlock_bh(&sk->sk_user_frags); > > for (k = 0; k < netmem_num; k++) > > WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); > > netmem_num = 0; > > - xa_lock_bh(&sk->sk_user_frags); > > You remove the locking but it's not clear to me why we don't need it > anymore. What's stopping 2 dontneeds from freeing at the same time? > I'm guessing it's because urefs are atomic so we don't need any extra > sync? > Yes, atomic uref. The array itself doesn't change until the socket is destroyed. > > } > > ret++; > > } > > } > > > > frag_limit_reached: > > - xa_unlock_bh(&sk->sk_user_frags); > > for (k = 0; k < netmem_num; k++) > > WARN_ON_ONCE(!napi_pp_put_page(netmems[k])); > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 40b774b4f587..585b50fa8c00 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -261,6 +261,7 @@ > > #include <linux/memblock.h> > > #include <linux/highmem.h> > > #include <linux/cache.h> > > +#include <linux/dma-buf.h> > > #include <linux/err.h> > > #include <linux/time.h> > > #include <linux/slab.h> > > @@ -475,7 +476,8 @@ void tcp_init_sock(struct sock *sk) > > > > set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags); > > sk_sockets_allocated_inc(sk); > > - xa_init_flags(&sk->sk_user_frags, XA_FLAGS_ALLOC1); > > + sk->sk_user_frags.binding = NULL; > > + sk->sk_user_frags.urefs = NULL; > > } > > EXPORT_IPV6_MOD(tcp_init_sock); > > > > @@ -2386,68 +2388,6 @@ static int tcp_inq_hint(struct sock *sk) > > return inq; > > } > > > > -/* batch __xa_alloc() calls and reduce xa_lock()/xa_unlock() overhead. */ > > -struct tcp_xa_pool { > > - u8 max; /* max <= MAX_SKB_FRAGS */ > > - u8 idx; /* idx <= max */ > > - __u32 tokens[MAX_SKB_FRAGS]; > > - netmem_ref netmems[MAX_SKB_FRAGS]; > > -}; > > - > > -static void tcp_xa_pool_commit_locked(struct sock *sk, struct tcp_xa_pool *p) > > -{ > > - int i; > > - > > - /* Commit part that has been copied to user space. */ > > - for (i = 0; i < p->idx; i++) > > - __xa_cmpxchg(&sk->sk_user_frags, p->tokens[i], XA_ZERO_ENTRY, > > - (__force void *)p->netmems[i], GFP_KERNEL); > > - /* Rollback what has been pre-allocated and is no longer needed. */ > > - for (; i < p->max; i++) > > - __xa_erase(&sk->sk_user_frags, p->tokens[i]); > > - > > - p->max = 0; > > - p->idx = 0; > > -} > > - > > -static void tcp_xa_pool_commit(struct sock *sk, struct tcp_xa_pool *p) > > -{ > > - if (!p->max) > > - return; > > - > > - xa_lock_bh(&sk->sk_user_frags); > > - > > - tcp_xa_pool_commit_locked(sk, p); > > - > > - xa_unlock_bh(&sk->sk_user_frags); > > -} > > - > > -static int tcp_xa_pool_refill(struct sock *sk, struct tcp_xa_pool *p, > > - unsigned int max_frags) > > -{ > > - int err, k; > > - > > - if (p->idx < p->max) > > - return 0; > > - > > - xa_lock_bh(&sk->sk_user_frags); > > - > > - tcp_xa_pool_commit_locked(sk, p); > > - > > - for (k = 0; k < max_frags; k++) { > > - err = __xa_alloc(&sk->sk_user_frags, &p->tokens[k], > > - XA_ZERO_ENTRY, xa_limit_31b, GFP_KERNEL); > > - if (err) > > - break; > > - } > > - > > - xa_unlock_bh(&sk->sk_user_frags); > > - > > - p->max = k; > > - p->idx = 0; > > - return k ? 0 : err; > > -} > > - > > /* On error, returns the -errno. On success, returns number of bytes sent to the > > * user. May not consume all of @remaining_len. > > */ > > @@ -2456,14 +2396,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > > int remaining_len) > > { > > struct dmabuf_cmsg dmabuf_cmsg = { 0 }; > > - struct tcp_xa_pool tcp_xa_pool; > > unsigned int start; > > int i, copy, n; > > int sent = 0; > > int err = 0; > > > > - tcp_xa_pool.max = 0; > > - tcp_xa_pool.idx = 0; > > do { > > start = skb_headlen(skb); > > > > @@ -2510,8 +2447,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > > */ > > for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > > skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; > > + struct net_devmem_dmabuf_binding *binding; > > struct net_iov *niov; > > u64 frag_offset; > > + size_t size; > > + u32 token; > > int end; > > > > /* !skb_frags_readable() should indicate that ALL the > > @@ -2544,13 +2484,35 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb, > > start; > > dmabuf_cmsg.frag_offset = frag_offset; > > dmabuf_cmsg.frag_size = copy; > > - err = tcp_xa_pool_refill(sk, &tcp_xa_pool, > > - skb_shinfo(skb)->nr_frags - i); > > - if (err) > > + > > + binding = net_devmem_iov_binding(niov); > > + > > + if (!sk->sk_user_frags.binding) { > > + sk->sk_user_frags.binding = binding; > > + > > + size = binding->dmabuf->size / PAGE_SIZE; > > + sk->sk_user_frags.urefs = kzalloc(size, > > + GFP_KERNEL); > > + if (!sk->sk_user_frags.urefs) { > > + sk->sk_user_frags.binding = NULL; > > + err = -ENOMEM; > > + goto out; > > + } > > + > > + net_devmem_dmabuf_binding_get(binding); > > It's not clear to me why we need to get the binding. AFAIR the way it > works is that we grab a reference on the net_iov, which guarantees > that the associated pp is alive, which in turn guarantees that the > binding remains alive and we don't need to get the binding for every > frag. > Oh you are right, the chain of references will keep it alive at least until dontneed. > > + } > > + > > + if (WARN_ONCE(sk->sk_user_frags.binding != binding, > > + "binding changed for devmem socket")) { > > Remove WARN_ONCE. It's not reasonable to kernel splat because the user > reconfigured flow steering me thinks. > Sounds good. > > + err = -EFAULT; > > goto out; > > + } > > + > > + token = net_iov_virtual_addr(niov) >> PAGE_SHIFT; > > + binding->vec[token] = niov; > > I don't think you can do this? I thought vec[token] was already == niov. > > binding->vec should be initialized when teh binding is initialized, > not re-written on every recvmsg, no? > Currently binding->vec is only initialized like that if direction == DMA_TO_DEVICE, but now that you mention it I don't see why RX shouldn't also initialize upfront too. [...] > Thanks, > Mina Thanks again for the review, much appreciated! Best, Bobby ^ permalink raw reply [flat|nested] 7+ messages in thread
* [syzbot ci] Re: net: devmem: improve cpu cost of RX token management 2025-09-02 21:36 [PATCH net-next 0/2] net: devmem: improve cpu cost of RX token management Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 2/2] net: devmem: use niov array for token management Bobby Eshleman @ 2025-09-03 17:46 ` syzbot ci 2 siblings, 0 replies; 7+ messages in thread From: syzbot ci @ 2025-09-03 17:46 UTC (permalink / raw) To: almasrymina, bobbyeshleman, bobbyeshleman, davem, dsahern, edumazet, horms, kuba, kuniyu, linux-kernel, ncardwell, netdev, pabeni, sdf, willemb Cc: syzbot, syzkaller-bugs syzbot ci has tested the following series [v1] net: devmem: improve cpu cost of RX token management https://lore.kernel.org/all/20250902-scratch-bobbyeshleman-devmem-tcp-token-upstream-v1-0-d946169b5550@meta.com * [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding * [PATCH net-next 2/2] net: devmem: use niov array for token management and found the following issue: general protection fault in sock_devmem_dontneed Full report is available here: https://ci.syzbot.org/series/c0dc7223-4222-461c-b04b-b6f0004c7509 *** general protection fault in sock_devmem_dontneed tree: net-next URL: https://kernel.googlesource.com/pub/scm/linux/kernel/git/netdev/net-next.git base: 864ecc4a6dade82d3f70eab43dad0e277aa6fc78 arch: amd64 compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8 config: https://ci.syzbot.org/builds/6c521908-0a73-48bc-a0fd-2576cf868361/config C repro: https://ci.syzbot.org/findings/9374f388-d643-42ea-831b-872eb5000b3c/c_repro syz repro: https://ci.syzbot.org/findings/9374f388-d643-42ea-831b-872eb5000b3c/syz_repro Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN PTI KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] CPU: 0 UID: 0 PID: 5993 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 RIP: 0010:sock_devmem_dontneed+0x3fd/0x960 net/core/sock.c:1112 Code: 8b 44 24 40 44 8b 28 44 03 6c 24 14 48 8b 44 24 20 42 80 3c 20 00 74 08 4c 89 ff e8 4d eb c9 f8 4d 8b 3f 4c 89 f8 48 c1 e8 03 <42> 80 3c 20 00 74 08 4c 89 ff e8 34 eb c9 f8 4d 8b 3f 4c 89 f8 48 RSP: 0018:ffffc90001c4fac0 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffff11004ddcfe0 RDX: ffff88801f6a8000 RSI: 0000000000000400 RDI: 0000000000000000 RBP: ffffc90001c4fc50 R08: ffffc90001c4fbdf R09: 0000000000000000 R10: ffffc90001c4fb60 R11: fffff52000389f7c R12: dffffc0000000000 R13: 00000000ffffffff R14: 0000000000000000 R15: 0000000000000000 FS: 000055558634d500(0000) GS:ffff8880b8614000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b30e63fff CR3: 0000000026b68000 CR4: 00000000000006f0 Call Trace: <TASK> sk_setsockopt+0x682/0x2dc0 net/core/sock.c:1302 do_sock_setsockopt+0x11b/0x1b0 net/socket.c:2340 __sys_setsockopt net/socket.c:2369 [inline] __do_sys_setsockopt net/socket.c:2375 [inline] __se_sys_setsockopt net/socket.c:2372 [inline] __x64_sys_setsockopt+0x13f/0x1b0 net/socket.c:2372 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7fea2bb8ebe9 Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007ffc81c63348 EFLAGS: 00000246 ORIG_RAX: 0000000000000036 RAX: ffffffffffffffda RBX: 00007fea2bdc5fa0 RCX: 00007fea2bb8ebe9 RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000003 RBP: 00007fea2bc11e19 R08: 0000000000000048 R09: 0000000000000000 R10: 0000200000000100 R11: 0000000000000246 R12: 0000000000000000 R13: 00007fea2bdc5fa0 R14: 00007fea2bdc5fa0 R15: 0000000000000005 </TASK> Modules linked in: ---[ end trace 0000000000000000 ]--- RIP: 0010:sock_devmem_dontneed+0x3fd/0x960 net/core/sock.c:1112 Code: 8b 44 24 40 44 8b 28 44 03 6c 24 14 48 8b 44 24 20 42 80 3c 20 00 74 08 4c 89 ff e8 4d eb c9 f8 4d 8b 3f 4c 89 f8 48 c1 e8 03 <42> 80 3c 20 00 74 08 4c 89 ff e8 34 eb c9 f8 4d 8b 3f 4c 89 f8 48 RSP: 0018:ffffc90001c4fac0 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffff11004ddcfe0 RDX: ffff88801f6a8000 RSI: 0000000000000400 RDI: 0000000000000000 RBP: ffffc90001c4fc50 R08: ffffc90001c4fbdf R09: 0000000000000000 R10: ffffc90001c4fb60 R11: fffff52000389f7c R12: dffffc0000000000 R13: 00000000ffffffff R14: 0000000000000000 R15: 0000000000000000 FS: 000055558634d500(0000) GS:ffff8880b8614000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b30e63fff CR3: 0000000026b68000 CR4: 00000000000006f0 ---------------- Code disassembly (best guess): 0: 8b 44 24 40 mov 0x40(%rsp),%eax 4: 44 8b 28 mov (%rax),%r13d 7: 44 03 6c 24 14 add 0x14(%rsp),%r13d c: 48 8b 44 24 20 mov 0x20(%rsp),%rax 11: 42 80 3c 20 00 cmpb $0x0,(%rax,%r12,1) 16: 74 08 je 0x20 18: 4c 89 ff mov %r15,%rdi 1b: e8 4d eb c9 f8 call 0xf8c9eb6d 20: 4d 8b 3f mov (%r15),%r15 23: 4c 89 f8 mov %r15,%rax 26: 48 c1 e8 03 shr $0x3,%rax * 2a: 42 80 3c 20 00 cmpb $0x0,(%rax,%r12,1) <-- trapping instruction 2f: 74 08 je 0x39 31: 4c 89 ff mov %r15,%rdi 34: e8 34 eb c9 f8 call 0xf8c9eb6d 39: 4d 8b 3f mov (%r15),%r15 3c: 4c 89 f8 mov %r15,%rax 3f: 48 rex.W *** If these findings have caused you to resend the series or submit a separate fix, please add the following tag to your commit message: Tested-by: syzbot@syzkaller.appspotmail.com --- This report is generated by a bot. It may contain errors. syzbot ci engineers can be reached at syzkaller@googlegroups.com. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-04 0:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-02 21:36 [PATCH net-next 0/2] net: devmem: improve cpu cost of RX token management Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 1/2] net: devmem: rename tx_vec to vec in dmabuf binding Bobby Eshleman 2025-09-02 21:36 ` [PATCH net-next 2/2] net: devmem: use niov array for token management Bobby Eshleman 2025-09-03 10:50 ` kernel test robot 2025-09-03 20:20 ` Mina Almasry 2025-09-04 0:15 ` Bobby Eshleman 2025-09-03 17:46 ` [syzbot ci] Re: net: devmem: improve cpu cost of RX " syzbot ci
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).