Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Mar 2021 20:44:41 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v2] Increase page and bit waitqueue hash size
To:     linux-kernel@vger.kernel.org,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Anton Blanchard <anton@ozlabs.org>, linux-mm@kvack.org,
        Ingo Molnar <mingo@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20210317075427.587806-1-npiggin@gmail.com>
        <89cb49c0-2736-dd4c-f401-4b88c22cc11f@rasmusvillemoes.dk>
In-Reply-To: <89cb49c0-2736-dd4c-f401-4b88c22cc11f@rasmusvillemoes.dk>
MIME-Version: 1.0
Message-Id: <1615977781.fijkhk7ep5.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Rasmus Villemoes's message of March 17, 2021 8:12 pm:
> On 17/03/2021 08.54, Nicholas Piggin wrote:
>=20
>> +#if CONFIG_BASE_SMALL
>> +static const unsigned int page_wait_table_bits =3D 4;
>>  static wait_queue_head_t page_wait_table[PAGE_WAIT_TABLE_SIZE] __cachel=
ine_aligned;
>=20
>> =20
>> +	if (!CONFIG_BASE_SMALL) {
>> +		page_wait_table =3D alloc_large_system_hash("page waitqueue hash",
>> +							sizeof(wait_queue_head_t),
>> +							0,
>=20
> So, how does the compiler not scream at you for assigning to an array,
> even if it's inside an if (0)?
>=20

Argh, because I didn't test small. Sorry I had the BASE_SMALL setting in=20
another patch and thought it would be a good idea to mash them together.=20
In hindsight probably not even if it did build.

Thanks,
Nick

--
[PATCH v3] Increase page and bit waitqueue hash size

The page waitqueue hash is a bit small (256 entries) on very big systems. A
16 socket 1536 thread POWER9 system was found to encounter hash collisions
and excessive time in waitqueue locking at times. This was intermittent and
hard to reproduce easily with the setup we had (very little real IO
capacity). The theory is that sometimes (depending on allocation luck)
important pages would happen to collide a lot in the hash, slowing down pag=
e
locking, causing the problem to snowball.

An small test case was made where threads would write and fsync different
pages, generating just a small amount of contention across many pages.

Increasing page waitqueue hash size to 262144 entries increased throughput
by 182% while also reducing standard deviation 3x. perf before the increase=
:

  36.23%  [k] _raw_spin_lock_irqsave                -      -
              |
              |--34.60%--wake_up_page_bit
              |          0
              |          iomap_write_end.isra.38
              |          iomap_write_actor
              |          iomap_apply
              |          iomap_file_buffered_write
              |          xfs_file_buffered_aio_write
              |          new_sync_write

  17.93%  [k] native_queued_spin_lock_slowpath      -      -
              |
              |--16.74%--_raw_spin_lock_irqsave
              |          |
              |           --16.44%--wake_up_page_bit
              |                     iomap_write_end.isra.38
              |                     iomap_write_actor
              |                     iomap_apply
              |                     iomap_file_buffered_write
              |                     xfs_file_buffered_aio_write

This patch uses alloc_large_system_hash to allocate a bigger system hash
that scales somewhat with memory size. The bit/var wait-queue is also
changed to keep code matching, albiet with a smaller scale factor.

This hash could be made per-node, which may help reduce remote accesses
on well localised workloads, but that adds some complexity with indexing
and hotplug, so until we get a less artificial workload to test with,
keep it simple.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 kernel/sched/wait_bit.c | 25 ++++++++++++++++++-------
 mm/filemap.c            | 16 ++++++++++++++--
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
index 02ce292b9bc0..3cc5fa552516 100644
--- a/kernel/sched/wait_bit.c
+++ b/kernel/sched/wait_bit.c
@@ -2,19 +2,20 @@
 /*
  * The implementation of the wait_bit*() and related waiting APIs:
  */
+#include <linux/memblock.h>
 #include "sched.h"
=20
-#define WAIT_TABLE_BITS 8
-#define WAIT_TABLE_SIZE (1 << WAIT_TABLE_BITS)
-
-static wait_queue_head_t bit_wait_table[WAIT_TABLE_SIZE] __cacheline_align=
ed;
+#define BIT_WAIT_TABLE_SIZE (1 << BIT_WAIT_TABLE_BITS)
+#define BIT_WAIT_TABLE_BITS bit_wait_table_bits
+static unsigned int bit_wait_table_bits __ro_after_init;
+static wait_queue_head_t *bit_wait_table __ro_after_init;
=20
 wait_queue_head_t *bit_waitqueue(void *word, int bit)
 {
 	const int shift =3D BITS_PER_LONG =3D=3D 32 ? 5 : 6;
 	unsigned long val =3D (unsigned long)word << shift | bit;
=20
-	return bit_wait_table + hash_long(val, WAIT_TABLE_BITS);
+	return bit_wait_table + hash_long(val, BIT_WAIT_TABLE_BITS);
 }
 EXPORT_SYMBOL(bit_waitqueue);
=20
@@ -152,7 +153,7 @@ EXPORT_SYMBOL(wake_up_bit);
=20
 wait_queue_head_t *__var_waitqueue(void *p)
 {
-	return bit_wait_table + hash_ptr(p, WAIT_TABLE_BITS);
+	return bit_wait_table + hash_ptr(p, BIT_WAIT_TABLE_BITS);
 }
 EXPORT_SYMBOL(__var_waitqueue);
=20
@@ -246,6 +247,16 @@ void __init wait_bit_init(void)
 {
 	int i;
=20
-	for (i =3D 0; i < WAIT_TABLE_SIZE; i++)
+	bit_wait_table =3D alloc_large_system_hash("bit waitqueue hash",
+						sizeof(wait_queue_head_t),
+						0,
+						22,
+						0,
+						&bit_wait_table_bits,
+						NULL,
+						0,
+						0);
+
+	for (i =3D 0; i < BIT_WAIT_TABLE_SIZE; i++)
 		init_waitqueue_head(bit_wait_table + i);
 }
diff --git a/mm/filemap.c b/mm/filemap.c
index 43700480d897..df5a3ef4031b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -34,6 +34,7 @@
 #include <linux/security.h>
 #include <linux/cpuset.h>
 #include <linux/hugetlb.h>
+#include <linux/memblock.h>
 #include <linux/memcontrol.h>
 #include <linux/cleancache.h>
 #include <linux/shmem_fs.h>
@@ -990,9 +991,10 @@ EXPORT_SYMBOL(__page_cache_alloc);
  * at a cost of "thundering herd" phenomena during rare hash
  * collisions.
  */
-#define PAGE_WAIT_TABLE_BITS 8
 #define PAGE_WAIT_TABLE_SIZE (1 << PAGE_WAIT_TABLE_BITS)
-static wait_queue_head_t page_wait_table[PAGE_WAIT_TABLE_SIZE] __cacheline=
_aligned;
+#define PAGE_WAIT_TABLE_BITS page_wait_table_bits
+static unsigned int page_wait_table_bits __ro_after_init;
+static wait_queue_head_t *page_wait_table __ro_after_init;
=20
 static wait_queue_head_t *page_waitqueue(struct page *page)
 {
@@ -1003,6 +1005,16 @@ void __init pagecache_init(void)
 {
 	int i;
=20
+	page_wait_table =3D alloc_large_system_hash("page waitqueue hash",
+						sizeof(wait_queue_head_t),
+						0,
+						21,
+						0,
+						&page_wait_table_bits,
+						NULL,
+						0,
+						0);
+
 	for (i =3D 0; i < PAGE_WAIT_TABLE_SIZE; i++)
 		init_waitqueue_head(&page_wait_table[i]);
=20
--=20
2.23.0

> Rasmus
>=20