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:02:18 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v2] Increase page and bit waitqueue hash size
To:     Ingo Molnar <mingo@kernel.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Anton Blanchard <anton@ozlabs.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20210317075427.587806-1-npiggin@gmail.com>
        <20210317083830.GC3881262@gmail.com>
In-Reply-To: <20210317083830.GC3881262@gmail.com>
MIME-Version: 1.0
Message-Id: <1615974423.0rc8elykcq.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Ingo Molnar's message of March 17, 2021 6:38 pm:
>=20
> * Nicholas Piggin <npiggin@gmail.com> wrote:
>=20
>> 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 collisio=
ns
>> 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 =
page
>> locking, causing the problem to snowball.
>>=20
>> An small test case was made where threads would write and fsync differen=
t
>> pages, generating just a small amount of contention across many pages.
>>=20
>> Increasing page waitqueue hash size to 262144 entries increased throughp=
ut
>> by 182% while also reducing standard deviation 3x. perf before the incre=
ase:
>>=20
>>   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
>>=20
>>   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
>>=20
>> 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.
>>=20
>> A very small CONFIG_BASE_SMALL option is also added because these are tw=
o
>> of the biggest static objects in the image on very small systems.
>>=20
>> 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.
>>=20
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  kernel/sched/wait_bit.c | 30 +++++++++++++++++++++++-------
>>  mm/filemap.c            | 24 +++++++++++++++++++++---
>>  2 files changed, 44 insertions(+), 10 deletions(-)
>>=20
>> diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
>> index 02ce292b9bc0..dba73dec17c4 100644
>> --- a/kernel/sched/wait_bit.c
>> +++ b/kernel/sched/wait_bit.c
>> @@ -2,19 +2,24 @@
>>  /*
>>   * 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)
>=20
> Ugh, 256 entries is almost embarrassingly small indeed.
>=20
> I've put your patch into sched/core, unless Andrew is objecting.

Thanks. Andrew and Linux might have some opinions on it, but if it's=20
just in a testing branch for now that's okay.


>=20
>> -	for (i =3D 0; i < WAIT_TABLE_SIZE; i++)
>> +	if (!CONFIG_BASE_SMALL) {
>> +		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);
>=20
>=20
> Meta suggestion: maybe the CONFIG_BASE_SMALL ugliness could be folded=20
> into alloc_large_system_hash() itself?

I don't like the ugliness and that's a good suggestion in some ways, but=20
having the constant size and table is nice for the small system. I don't=20
know, maybe we need to revise the alloc_large_system_hash API slightly.

Having some kind of DEFINE_LARGE_ARRAY perhaps then you could have both
static and dynamic? I'll think about it.

>=20
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>=20
>>  static wait_queue_head_t *page_waitqueue(struct page *page)
>>  {
>> -	return &page_wait_table[hash_ptr(page, PAGE_WAIT_TABLE_BITS)];
>> +	return &page_wait_table[hash_ptr(page, page_wait_table_bits)];
>>  }
>=20
> I'm wondering whether you've tried to make this NUMA aware through=20
> page->node?
>=20
> Seems like another useful step when having a global hash ...

Yes I have patches for that on the back burner. Just wanted to try one
step at a time, but I think we should be able to justify it (a well=20
NUMAified workload will tend to store mostly to local page waitqueue so=20
keep cacheline contention within the node). We need to get some access=20
to a big system again and try get some more IO on it at some point, so=20
stay tuned for that.

We actually used to have similar to this, but Linux removed it with
9dcb8b685fc30.

The difference now is that the page waitqueue has been split out from
the bit waitqueue. Doing the page waitqueue is much easier because we
don't have the vmalloc problem to deal with. But still it's some
complexity.

We also do have the page contention bit that Linus refers to which takes=20
pressure off the waitqueues (which is probably why 256 entries has held=20
up surprisingly well), but as we can see we do need larger at the high
end.

Thanks,
Nick