[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] migration/multifd: Protect accesses to migration_threads
From: |
Juan Quintela |
Subject: |
Re: [PATCH 2/3] migration/multifd: Protect accesses to migration_threads |
Date: |
Wed, 07 Jun 2023 10:26:09 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Fabiano Rosas <farosas@suse.de> wrote:
> This doubly linked list is common for all the multifd and migration
> threads so we need to avoid concurrent access.
>
> Add a mutex to protect the data from concurrent access. This fixes a
> crash when removing two MigrationThread objects from the list at the
> same time during cleanup of multifd threads.
>
> To avoid destroying the mutex before the last element has been
> removed, move calls to qmp_migration_thread_remove so they run before
> multifd_save_cleanup joins the threads.
>
> Fixes: 671326201d ("migration: Introduce interface query-migrationthreads")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
I agree with Peter here. Why don't you have to protect the walking?
> ---
> migration/migration.c | 5 ++++-
> migration/multifd.c | 3 ++-
> migration/threadinfo.c | 19 ++++++++++++++++++-
> migration/threadinfo.h | 5 +++--
> 4 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index e731fc98a1..b3b8345eb2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1146,6 +1146,7 @@ static void migrate_fd_cleanup(MigrationState *s)
> qemu_mutex_lock_iothread();
>
> multifd_save_cleanup();
> + qmp_migration_threads_cleanup();
I think I will spare this one as the mutex is static, so we are not
winning any memory back.
> }
>
> trace_migration_thread_after_loop();
> + qmp_migration_threads_remove(thread);
> migration_iteration_finish(s);
I can understand moving it here, but why before migration_iteration_finish?
> object_unref(OBJECT(s));
> rcu_unregister_thread();
> - qmp_migration_threads_remove(thread);
> return NULL;
> }
> + qmp_migration_threads_remove(thread);
> +
> qemu_mutex_lock(&p->mutex);
> p->running = false;
> qemu_mutex_unlock(&p->mutex);
>
> rcu_unregister_thread();
> - qmp_migration_threads_remove(thread);
> trace_multifd_send_thread_end(p->id, p->num_packets,
> p->total_normal_pages);
>
> return NULL;
Here it looks like the right place.
> +#include "qemu/osdep.h"
> +#include "qemu/queue.h"
> +#include "qemu/lockable.h"
> #include "threadinfo.h"
Ouch, it missed Markus cleanup. Thanks.
For the rest it looks good.
Later, Juan.
[PATCH 3/3] tests/qtest: Re-enable multifd cancel test, Fabiano Rosas, 2023/06/06