Re: [PATCH v2] net: skbuff: set FLAG_SKB_NO_MERGE for skbuff_fclone_cache
From: Shijie Huang
Date: Wed Feb 28 2024 - 02:06:28 EST
在 2024/2/27 20:55, Eric Dumazet 写道:
On Tue, Feb 27, 2024 at 7:29 AM Huang Shijie
<shijie@xxxxxxxxxxxxxxxxxxxxxx> wrote:
Since we do not set FLAG_SKB_NO_MERGE for skbuff_fclone_cache,
the current skbuff_fclone_cache maybe not really allocated, it maybe
used an exist old kmem_cache. In NUMA, the fclone allocated by
alloc_skb_fclone() maybe in remote node.
Why is this happening in the first place ? Whab about skb->head ?
I tested the fclone firstly. I did not test others yet.
I did not check the skb->head yet.
But I ever checked the pfrag's page, it is okay.
Jesper patch [1] motivation was not about NUMA., but about
fragmentation and bulk allocations/freeing.
TCP fclones are not bulk allocated/freed, so I do not understand what
your patch is doing.
You need to give more details, and experimental results.
1.) My NUMA machine:
node 0 (CPU 0 ~ CPU79):
CPU 0 ~ CPU 39 are used as memcached's server
CPU 40 ~ CPU 79 are used as memcached's client
node 1 (CPU 80 ~ CPU160):
CPU 80 ~ CPU 119 are used as memcached's server
CPU 120 ~ CPU 179 are used as memcached's client
the kernel is linux-next 20240227
2.) My private patches:
patch 1 is for slub:
---
mm/slub.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/slub.c b/mm/slub.c
index 5d838ebfa35e..d2ab1e36fd6b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5691,6 +5691,7 @@ __kmem_cache_alias(const char *name, unsigned int
size, unsigned int align,
s = find_mergeable(size, align, flags, name, ctor);
if (s) {
+ printk("[%s] origin:%s, shared :%s\n", __func__, name,
s->name);
if (sysfs_slab_alias(s, name))
return NULL;
---------
This patch is used the check which is the sharing kmem_cache for
"skbuff_fclone_cache".
I cannot find the "skbuff_fclone_cache" in /proc/slabinfo.
From my test, the "pool_workqueue" is the real working kmem_cache.
The "skbuff_fclone_cache" is just a pointer to "pool_workqueue"
(pwq_cache).
The following private patch is used to record the fclone allocation:
---
net/ipv4/tcp.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index c82dc42f57c6..6f31ddcfc017 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -864,6 +864,24 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t
*ppos,
}
EXPORT_SYMBOL(tcp_splice_read);
+unsigned long right_num, wrong_num;
+static void check_fclone(struct sk_buff *skb)
+{
+ int n = numa_mem_id(); /* current node */
+ int node2 = page_to_nid(virt_to_page((unsigned long) skb));
+
+ if (n != node2) {
+ wrong_num++;
+ if ((wrong_num % 1000) == 999)
+ printk(KERN_DEBUG "[%s] current:%d, get from
:%d, (%ld, %ld, %ld)\n",
+ __func__, n, node2, wrong_num,
right_num, wrong_num * 100 / (wrong_num + right_num));
+ } else {
+ right_num++;
+ if ((right_num % 1000000) == 9999)
+ printk("[%s] we received :%ld, %ld\n", __func__,
right_num, wrong_num);
+ }
+}
+
struct sk_buff *tcp_stream_alloc_skb(struct sock *sk, gfp_t gfp,
bool force_schedule)
{
@@ -884,6 +902,7 @@ struct sk_buff *tcp_stream_alloc_skb(struct sock
*sk, gfp_t gfp,
skb_reserve(skb, MAX_TCP_HEADER);
skb->ip_summed = CHECKSUM_PARTIAL;
INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);
+ check_fclone(skb);
return skb;
}
__kfree_skb(skb);
--
Without this v2 patch, I can get the result after the memcached test:
[ 1027.317645] [check_fclone] current:0, get from :1, (7112999, 9076711, 43)
[ 1027.317653] [check_fclone] current:0, get from :1, (7112999, 9076707, 43)
[ 1027.804110] [check_fclone] we received :10009999, 7113326
It means nearly 43% fclone is allocated in the remote node.
With this v2 patch, I can find the "skbuff_fclone_cache" in
/proc/slabinfo.
The test result shows below:
[ 503.357293] [check_fclone] we received :8009999, 0
[ 503.357293] [check_fclone] we received :8009999, 0
[ 503.357305] [check_fclone] we received :8009999, 0
After v2 patch, I cannot see the wrong fclone in remote node.
Using SLAB_NO_MERGE does not help, I am still seeing wrong allocations
on a dual socket
host with plenty of available memory.
(either sk_buff or skb->head being allocated on the other node).
Do you mean you still can see the wrong fclone after using SLAB_NO_MERGE?
If so, I guess there is bug in the slub.
fclones might be allocated from a cpu running on node A, and freed
from a cpu running on node B.
Maybe SLUB is not properly handling this case ?
Maybe.
SLAB_NO_MERGE will avoid merging fclone with kmalloc-512, it does not
really help.
I think we need help from mm/slub experts, instead of trying to 'fix'
networking stacks.
@Christopher
Any idea about this?
Thanks
Huang Shijie
Perhaps we could augment trace_kmem_cache_alloc() to record/print the nodes
of the allocated chunk (we already have the cpu number giving us the
local node).
That would give us more confidence on any fixes.
BTW SLUB is gone, time to remove FLAG_SKB_NO_MERGE and simply use SLAB_NO_MERGE
[1]
commit 0a0643164da4a1976455aa12f0a96d08ee290752
Author: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
Date: Tue Aug 15 17:17:36 2023 +0200
net: use SLAB_NO_MERGE for kmem_cache skbuff_head_cache
So set FLAG_SKB_NO_MERGE for skbuff_fclone_cache to fix it.
Signed-off-by: Huang Shijie <shijie@xxxxxxxxxxxxxxxxxxxxxx>
---
v1 --> v2:
set FLAG_SKB_NO_MERGE for skbuff_fclone_cache in initialization.
v1: https://lkml.org/lkml/2024/2/20/121
---
net/core/skbuff.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1f918e602bc4..5e3e130fb57a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5013,7 +5013,8 @@ void __init skb_init(void)
skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
sizeof(struct sk_buff_fclones),
0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+ SLAB_HWCACHE_ALIGN|SLAB_PANIC|
+ FLAG_SKB_NO_MERGE,
NULL);
/* usercopy should only access first SKB_SMALL_HEAD_HEADROOM bytes.
* struct skb_shared_info is located at the end of skb->head,
--
2.40.1