Re: [PATCH] coroutine: resize pool periodically instead of limiting size

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Thu, Sep 09, 2021 at 05:08:08PM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 09, 2021 at 04:51:44PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 09, 2021 at 09:37:20AM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Sep 01, 2021 at 05:09:23PM +0100, Stefan Hajnoczi wrote:
> > > > It was reported that enabling SafeStack reduces IOPS significantly
> > > > (>25%) with the following fio benchmark on virtio-blk using a NVMe host
> > > > block device:
> > > > 
> > > >   # fio --rw=randrw --bs=4k --iodepth=64 --runtime=1m --direct=1 \
> > > > 	--filename=/dev/vdb --name=job1 --ioengine=libaio --thread \
> > > > 	--group_reporting --numjobs=16 --time_based \
> > > >         --output=/tmp/fio_result
> > > > 
> > > > Serge Guelton and I found that SafeStack is not really at fault, it just
> > > > increases the cost of coroutine creation. This fio workload exhausts the
> > > > coroutine pool and coroutine creation becomes a bottleneck. Previous
> > > > work by Honghao Wang also pointed to excessive coroutine creation.
> > > > 
> > > > Creating new coroutines is expensive due to allocating new stacks with
> > > > mmap(2) and mprotect(2). Currently there are thread-local and global
> > > > pools that recycle old Coroutine objects and their stacks but the
> > > > hardcoded size limit of 64 for thread-local pools and 128 for the global
> > > > pool is insufficient for the fio benchmark shown above.
> > > 
> > > Rather than keeping around a thread local pool of coroutine
> > > instances, did you ever consider keeping around a pool of
> > > allocated stacks ? Essentially it seems like you're syaing
> > > the stack allocation is the problem due to it using mmap()
> > > instead of malloc() and thus not benefiting from any of the
> > > performance tricks malloc() impls use to avoid repeated
> > > syscalls on every allocation.  If 'qemu_alloc_stack' and
> > > qemu_free_stack could be made more intelligent by caching
> > > stacks, then perhaps the coroutine side can be left "dumb" ?
> > 
> > What is the advantage of doing that? Then the Coroutine struct needs to
> > be malloced each time. Coroutines are the only users of
> > qemu_alloc_stack(), so I think pooling the Coroutines is optimal.
> 
> I mostly thought it might lead itself to cleaner implementation if the
> pooling logic is separate from the main coroutine logic. It could be
> easier to experiment with different allocation strategies if the code
> related to pooling is well isolated.

There is an advantage to pooling the Coroutine struct in addition to the
stack, so I'm not sure if it's worth reducing the scope of the pool.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]