| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89iKwCPfHmEkjUn7Xedz+=maFArk5Nx8cNS_fDKgWZ4_Nvw@mail.gmail.com>
Date: Thu, 6 Mar 2025 09:17:50 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Kuniyuki Iwashima <kuniyu@...zon.com>, davem@...emloft.net, eric.dumazet@...il.com,
horms@...nel.org, kernelxing@...cent.com, kuba@...nel.org,
ncardwell@...gle.com, netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next] tcp: bring back NUMA dispersion in inet_ehash_locks_alloc()
On Thu, Mar 6, 2025 at 9:04 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> On Thu, Mar 6, 2025 at 3:26 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Thu, Mar 6, 2025 at 7:35 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > >
> > > On Thu, Mar 6, 2025 at 2:22 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > > >
> > > > From: Jason Xing <kerneljasonxing@...il.com>
> > > > Date: Thu, 6 Mar 2025 12:59:03 +0800
> > > > > On Thu, Mar 6, 2025 at 12:12 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > > > > >
> > > > > > From: Jason Xing <kerneljasonxing@...il.com>
> > > > > > Date: Thu, 6 Mar 2025 11:35:27 +0800
> > > > > > > On Wed, Mar 5, 2025 at 9:06 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > > > > > > >
> > > > > > > > We have platforms with 6 NUMA nodes and 480 cpus.
> > > > > > > >
> > > > > > > > inet_ehash_locks_alloc() currently allocates a single 64KB page
> > > > > > > > to hold all ehash spinlocks. This adds more pressure on a single node.
> > > > > > > >
> > > > > > > > Change inet_ehash_locks_alloc() to use vmalloc() to spread
> > > > > > > > the spinlocks on all online nodes, driven by NUMA policies.
> > > > > > > >
> > > > > > > > At boot time, NUMA policy is interleave=all, meaning that
> > > > > > > > tcp_hashinfo.ehash_locks gets hash dispersion on all nodes.
> > > > > > > >
> > > > > > > > Tested:
> > > > > > > >
> > > > > > > > lack5:~# grep inet_ehash_locks_alloc /proc/vmallocinfo
> > > > > > > > 0x00000000d9aec4d1-0x00000000a828b652 69632 inet_ehash_locks_alloc+0x90/0x100 pages=16 vmalloc N0=2 N1=3 N2=3 N3=3 N4=3 N5=2
> > > > > > > >
> > > > > > > > lack5:~# echo 8192 >/proc/sys/net/ipv4/tcp_child_ehash_entries
> > > > > > > > lack5:~# numactl --interleave=all unshare -n bash -c "grep inet_ehash_locks_alloc /proc/vmallocinfo"
> > > > > > > > 0x000000004e99d30c-0x00000000763f3279 36864 inet_ehash_locks_alloc+0x90/0x100 pages=8 vmalloc N0=1 N1=2 N2=2 N3=1 N4=1 N5=1
> > > > > > > > 0x00000000d9aec4d1-0x00000000a828b652 69632 inet_ehash_locks_alloc+0x90/0x100 pages=16 vmalloc N0=2 N1=3 N2=3 N3=3 N4=3 N5=2
> > > > > > > >
> > > > > > > > lack5:~# numactl --interleave=0,5 unshare -n bash -c "grep inet_ehash_locks_alloc /proc/vmallocinfo"
> > > > > > > > 0x00000000fd73a33e-0x0000000004b9a177 36864 inet_ehash_locks_alloc+0x90/0x100 pages=8 vmalloc N0=4 N5=4
> > > > > > > > 0x00000000d9aec4d1-0x00000000a828b652 69632 inet_ehash_locks_alloc+0x90/0x100 pages=16 vmalloc N0=2 N1=3 N2=3 N3=3 N4=3 N5=2
> > > > > > > >
> > > > > > > > lack5:~# echo 1024 >/proc/sys/net/ipv4/tcp_child_ehash_entries
> > > > > > > > lack5:~# numactl --interleave=all unshare -n bash -c "grep inet_ehash_locks_alloc /proc/vmallocinfo"
> > > > > > > > 0x00000000db07d7a2-0x00000000ad697d29 8192 inet_ehash_locks_alloc+0x90/0x100 pages=1 vmalloc N2=1
> > > > > > > > 0x00000000d9aec4d1-0x00000000a828b652 69632 inet_ehash_locks_alloc+0x90/0x100 pages=16 vmalloc N0=2 N1=3 N2=3 N3=3 N4=3 N5=2
> > > > > > > >
> > > > > > > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > > > > >
> > > > > > > Tested-by: Jason Xing <kerneljasonxing@...il.com>
> > > > > >
> > > > > > Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > > net/ipv4/inet_hashtables.c | 37 ++++++++++++++++++++++++++-----------
> > > > > > > > 1 file changed, 26 insertions(+), 11 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > > > > > > > index 9bfcfd016e18275fb50fea8d77adc8a64fb12494..2b4a588247639e0c7b2e70d1fc9b3b9b60256ef7 100644
> > > > > > > > --- a/net/ipv4/inet_hashtables.c
> > > > > > > > +++ b/net/ipv4/inet_hashtables.c
> > > > > > > > @@ -1230,22 +1230,37 @@ int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
> > > > > > > > {
> > > > > > > > unsigned int locksz = sizeof(spinlock_t);
> > > > > > > > unsigned int i, nblocks = 1;
> > > > > > > > + spinlock_t *ptr = NULL;
> > > > > > > >
> > > > > > > > - if (locksz != 0) {
> > > > > > > > - /* allocate 2 cache lines or at least one spinlock per cpu */
> > > > > > > > - nblocks = max(2U * L1_CACHE_BYTES / locksz, 1U);
> > > > > > > > - nblocks = roundup_pow_of_two(nblocks * num_possible_cpus());
> > > > > > > > + if (locksz == 0)
> > > > > > > > + goto set_mask;
> > > > > > > >
> > > > > > > > - /* no more locks than number of hash buckets */
> > > > > > > > - nblocks = min(nblocks, hashinfo->ehash_mask + 1);
> > > > > > > > + /* Allocate 2 cache lines or at least one spinlock per cpu. */
> > > > > > > > + nblocks = max(2U * L1_CACHE_BYTES / locksz, 1U) * num_possible_cpus();
> > > > > > > >
> > > > > > > > - hashinfo->ehash_locks = kvmalloc_array(nblocks, locksz, GFP_KERNEL);
> > > > > > > > - if (!hashinfo->ehash_locks)
> > > > > > > > - return -ENOMEM;
> > > > > > > > + /* At least one page per NUMA node. */
> > > > > > > > + nblocks = max(nblocks, num_online_nodes() * PAGE_SIZE / locksz);
> > > > > > > > +
> > > > > > > > + nblocks = roundup_pow_of_two(nblocks);
> > > > > > > > +
> > > > > > > > + /* No more locks than number of hash buckets. */
> > > > > > > > + nblocks = min(nblocks, hashinfo->ehash_mask + 1);
> > > > > > > >
> > > > > > > > - for (i = 0; i < nblocks; i++)
> > > > > > > > - spin_lock_init(&hashinfo->ehash_locks[i]);
> > > > > > > > + if (num_online_nodes() > 1) {
> > > > > > > > + /* Use vmalloc() to allow NUMA policy to spread pages
> > > > > > > > + * on all available nodes if desired.
> > > > > > > > + */
> > > > > > > > + ptr = vmalloc_array(nblocks, locksz);
> > > > > > >
> > > > > > > I wonder if at this point the memory shortage occurs, is it necessary
> > > > > > > to fall back to kvmalloc() later
> > > > > >
> > > > > > If ptr is NULL here, kvmalloc_array() is called below.
> > > > >
> > > > > My point is why not return with -ENOMEM directly? Or else It looks meaningless.
> > > > >
> > > >
> > > > Ah, I misread. I'm not sure how likely such a case happens, but I
> > > > think vmalloc() and kmalloc() failure do not always correlate, the
> > > > former uses node_alloc() and the latter use the page allocator.
> > >
> > > Sure, it is unlikely to happen.
> > >
> > > As to memory allocation, we usually try kmalloc() for less than page
> > > size memory allocation while vmalloc() for larger one. The same logic
> > > can be seen in kvmalloc(): try kmalloc() first, then fall back to
> > > vmalloc(). Since we fail to allocate non-contiguous memory, there is
> > > no need to try kvmalloc() (which will call kmalloc and vmalloc one
> > > more round).
> >
> > I chose to not add code, because:
> >
> > if (num_online_nodes() > 1) {
> > /* Use vmalloc() to allow NUMA policy to spread pages
> > * on all available nodes if desired.
> > */
> > ptr = vmalloc_array(nblocks, locksz);
> >
> > << adding here a test is pointless, we already have correct code if
> > ptr == NULLL >>
> >
> > }
> > if (!ptr) {
> > ptr = kvmalloc_array(nblocks, locksz, GFP_KERNEL);
> > if (!ptr)
> > return -ENOMEM;
> > }
> >
> >
> > Sure, this could be written in a different way, but ultimately it is a
> > matter of taste.
>
> Sorry that I didn't make myself clear enough. I mean if
> vmalloc_array() fails, then it will fall back to kvmalloc_array()
> which will call kmalloc() or even vmalloc() to allocate memory again.
> My intention is to return with an error code when the first time
> allocation fails.
>
> Code like this on top of your patch:
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 3edbe2dad8ba..d026918319d2 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -1282,12 +1282,12 @@ int inet_ehash_locks_alloc(struct
> inet_hashinfo *hashinfo)
> * on all available nodes if desired.
> */
> ptr = vmalloc_array(nblocks, locksz);
> - }
> - if (!ptr) {
> + } else {
> ptr = kvmalloc_array(nblocks, locksz, GFP_KERNEL);
> - if (!ptr)
> - return -ENOMEM;
> }
> + if (!ptr)
> + return -ENOMEM;
> +
I think I understood pretty well, and said it was a matter of taste.
I wish we could move on to more interesting stuff, like the main ehash
table, which currently
uses 2 huge pages, they can all land into one physical socket.
Powered by blists - more mailing lists