From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Emanuele Giuseppe Esposito <eesposit@redhat.com>, qemu-block@nongnu.org
Cc: John Snow <jsnow@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
qemu-devel@nongnu.org, "Denis V. Lunev" <den@openvz.org>
Subject: Re: [PATCH 6/6] aiopool: protect with a mutex
Date: Mon, 10 May 2021 14:56:33 +0300 [thread overview]
Message-ID: <bb9be29f-a3ef-3869-ba6f-20a7acc1fe2a@virtuozzo.com> (raw)
In-Reply-To: <20210510085941.22769-7-eesposit@redhat.com>
10.05.2021 11:59, Emanuele Giuseppe Esposito wrote:
> Divide the fields in AioTaskPool in IN and Status, and
> introduce a CoQueue instead of .wait to take care of suspending
> and resuming the calling coroutine, and a lock to protect the
> busy_tasks counter accesses and the AioTask .ret field.
"and" in commit message is almost always a sign, that there should be several commits :)
Please, do at least refactoring to drop "main_co" in separate of adding a mutex.
Hmm, actually, that was done in Denis's series "[PATCH v8 0/6] block: seriously improve savevm/loadvm performance". (https://patchew.org/QEMU/20200709132644.28470-1-den@openvz.org/)
Probably, you could reuse patches 01,02 of it.
(add Den to cc)
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> block/aio_task.c | 63 ++++++++++++++++++++++++----------------
> include/block/aio_task.h | 2 +-
> 2 files changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/block/aio_task.c b/block/aio_task.c
> index 88989fa248..7ac6b5dd72 100644
> --- a/block/aio_task.c
> +++ b/block/aio_task.c
> @@ -27,62 +27,70 @@
> #include "block/aio_task.h"
>
> struct AioTaskPool {
> - Coroutine *main_co;
> - int status;
> + /* IN: just set in aio_task_pool_new and never modified */
> int max_busy_tasks;
> +
> + /* Status: either atomic or protected by the lock */
> + int status;
> int busy_tasks;
> - bool waiting;
> + CoQueue queue;
> + CoMutex lock;
> };
>
> static void coroutine_fn aio_task_co(void *opaque)
> {
> + int ret;
> AioTask *task = opaque;
> AioTaskPool *pool = task->pool;
>
> - assert(pool->busy_tasks < pool->max_busy_tasks);
> - pool->busy_tasks++;
> + WITH_QEMU_LOCK_GUARD(&pool->lock) {
> + assert(pool->busy_tasks < pool->max_busy_tasks);
> + pool->busy_tasks++;
>
> - task->ret = task->func(task);
> + ret = task->func(task);
> + task->ret = ret;
>
> - pool->busy_tasks--;
> + pool->busy_tasks--;
> + }
>
> - if (task->ret < 0 && pool->status == 0) {
> - pool->status = task->ret;
> + if (ret < 0) {
> + qatomic_cmpxchg(&pool->status, 0, ret);
> }
Can we just do it inside critical section above and avoid extra cmpxchg? We'll need just qatomic_set as a pair to qatomic_read()
>
> g_free(task);
>
> - if (pool->waiting) {
> - pool->waiting = false;
> - aio_co_wake(pool->main_co);
> - }
> + qemu_co_queue_next(&pool->queue);
this call doesn't need mutex protection? Then we should modify comment insid AioTaskPool structure.
Anyway, I think it's simpler to just have one QEMU_MUTEX_GUARD() for the whole function.
> }
>
> -void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
> +/* Called with lock held */
again, such things usually called _locked().
> +static void coroutine_fn aio_task_pool_wait_one_unlocked(AioTaskPool *pool)
> {
> assert(pool->busy_tasks > 0);
> - assert(qemu_coroutine_self() == pool->main_co);
> -
> - pool->waiting = true;
> - qemu_coroutine_yield();
> -
> - assert(!pool->waiting);
> + qemu_co_queue_wait(&pool->queue, &pool->lock);
> assert(pool->busy_tasks < pool->max_busy_tasks);
> }
>
> +void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
> +{
> + QEMU_LOCK_GUARD(&pool->lock);
> + aio_task_pool_wait_one_unlocked(pool);
> +}
> +
> void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
> {
> + QEMU_LOCK_GUARD(&pool->lock);
> if (pool->busy_tasks < pool->max_busy_tasks) {
> return;
> }
>
> - aio_task_pool_wait_one(pool);
> + aio_task_pool_wait_one_unlocked(pool);
> }
>
> void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
> {
> + QEMU_LOCK_GUARD(&pool->lock);
> while (pool->busy_tasks > 0) {
> - aio_task_pool_wait_one(pool);
> + aio_task_pool_wait_one_unlocked(pool);
> }
> }
>
> @@ -98,8 +106,8 @@ AioTaskPool *coroutine_fn aio_task_pool_new(int max_busy_tasks)
> {
> AioTaskPool *pool = g_new0(AioTaskPool, 1);
>
> - pool->main_co = qemu_coroutine_self();
> pool->max_busy_tasks = max_busy_tasks;
> + qemu_co_queue_init(&pool->queue);
>
> return pool;
> }
> @@ -115,10 +123,15 @@ int aio_task_pool_status(AioTaskPool *pool)
> return 0; /* Sugar for lazy allocation of aio pool */
> }
>
> - return pool->status;
> + return qatomic_read(&pool->status);
> }
>
> bool aio_task_pool_empty(AioTaskPool *pool)
> {
> - return pool->busy_tasks == 0;
> + int tasks;
> +
> + qemu_co_mutex_lock(&pool->lock);
> + tasks = pool->busy_tasks;
> + qemu_co_mutex_unlock(&pool->lock);
> + return tasks == 0;
> }
> diff --git a/include/block/aio_task.h b/include/block/aio_task.h
> index 50bc1e1817..b22a4310aa 100644
> --- a/include/block/aio_task.h
> +++ b/include/block/aio_task.h
> @@ -33,7 +33,7 @@ typedef int coroutine_fn (*AioTaskFunc)(AioTask *task);
> struct AioTask {
> AioTaskPool *pool;
> AioTaskFunc func;
> - int ret;
> + int ret; /* atomic */
> };
>
> AioTaskPool *coroutine_fn aio_task_pool_new(int max_busy_tasks);
>
--
Best regards,
Vladimir
next prev parent reply other threads:[~2021-05-10 12:06 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-10 8:59 [PATCH 0/6] block-copy: make helper APIs thread safe Emanuele Giuseppe Esposito
2021-05-10 8:59 ` [PATCH 1/6] ratelimit: treat zero speed as unlimited Emanuele Giuseppe Esposito
2021-05-10 11:00 ` Vladimir Sementsov-Ogievskiy
2021-05-10 8:59 ` [PATCH 2/6] block-copy: let ratelimit handle a speed of 0 Emanuele Giuseppe Esposito
2021-05-10 11:06 ` Vladimir Sementsov-Ogievskiy
2021-05-10 8:59 ` [PATCH 3/6] blockjob: " Emanuele Giuseppe Esposito
2021-05-10 11:17 ` Vladimir Sementsov-Ogievskiy
2021-05-10 8:59 ` [PATCH 4/6] progressmeter: protect with a mutex Emanuele Giuseppe Esposito
2021-05-10 11:28 ` Vladimir Sementsov-Ogievskiy
2021-05-10 16:52 ` Emanuele Giuseppe Esposito
2021-05-10 17:17 ` Vladimir Sementsov-Ogievskiy
2021-05-10 17:57 ` Emanuele Giuseppe Esposito
2021-05-11 12:28 ` Paolo Bonzini
2021-05-12 7:09 ` Vladimir Sementsov-Ogievskiy
2021-05-12 15:53 ` Stefan Hajnoczi
2021-05-10 8:59 ` [PATCH 5/6] co-shared-resource: " Emanuele Giuseppe Esposito
2021-05-10 11:40 ` Vladimir Sementsov-Ogievskiy
2021-05-11 8:34 ` Paolo Bonzini
2021-05-12 15:44 ` Stefan Hajnoczi
2021-05-12 18:30 ` Paolo Bonzini
2021-05-14 14:10 ` Emanuele Giuseppe Esposito
2021-05-14 14:26 ` Vladimir Sementsov-Ogievskiy via
2021-05-14 14:32 ` Emanuele Giuseppe Esposito
2021-05-14 15:30 ` Vladimir Sementsov-Ogievskiy via
2021-05-14 17:28 ` Emanuele Giuseppe Esposito
2021-05-14 21:15 ` Vladimir Sementsov-Ogievskiy via
2021-05-14 21:53 ` Emanuele Giuseppe Esposito
2021-05-15 7:11 ` Vladimir Sementsov-Ogievskiy via
2021-05-14 17:55 ` Paolo Bonzini
2021-05-10 8:59 ` [PATCH 6/6] aiopool: " Emanuele Giuseppe Esposito
2021-05-10 11:56 ` Vladimir Sementsov-Ogievskiy [this message]
2021-05-11 8:34 ` Paolo Bonzini
2021-05-12 15:19 ` Stefan Hajnoczi
2021-05-10 10:55 ` [PATCH 0/6] block-copy: make helper APIs thread safe Vladimir Sementsov-Ogievskiy
2021-05-12 14:24 ` Stefan Hajnoczi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb9be29f-a3ef-3869-ba6f-20a7acc1fe2a@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=den@openvz.org \
--cc=eesposit@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).