Re: [PATCH v3 11/24] migration/multifd: Make multifd_send() thread safe

[Peter Xu]

On Sun, Nov 17, 2024 at 08:20:06PM +0100, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> multifd_send() function is currently not thread safe, make it thread safe
> by holding a lock during its execution.
> 
> This way it will be possible to safely call it concurrently from multiple
> threads.
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

One nitpick:

> ---
>  migration/multifd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 9578a985449b..4575495c8816 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -50,6 +50,10 @@ typedef struct {
>  
>  struct {
>      MultiFDSendParams *params;
> +
> +    /* multifd_send() body is not thread safe, needs serialization */
> +    QemuMutex multifd_send_mutex;
> +
>      /*
>       * Global number of generated multifd packets.
>       *
> @@ -331,6 +335,7 @@ static void multifd_send_kick_main(MultiFDSendParams *p)
>   */
>  bool multifd_send(MultiFDSendData **send_data)
>  {
> +    QEMU_LOCK_GUARD(&multifd_send_state->multifd_send_mutex);

Better move this after the varaible declarations to be clear..

Perhaps even after multifd_send_should_exit() because reading that doesn't
need a lock, just in case something wanna quit but keep stuck with the
mutex.

>      int i;
>      static int next_channel;
>      MultiFDSendParams *p = NULL; /* make happy gcc */
> @@ -508,6 +513,7 @@ static void multifd_send_cleanup_state(void)
>      socket_cleanup_outgoing_migration();
>      qemu_sem_destroy(&multifd_send_state->channels_created);
>      qemu_sem_destroy(&multifd_send_state->channels_ready);
> +    qemu_mutex_destroy(&multifd_send_state->multifd_send_mutex);
>      g_free(multifd_send_state->params);
>      multifd_send_state->params = NULL;
>      g_free(multifd_send_state);
> @@ -853,6 +859,7 @@ bool multifd_send_setup(void)
>      thread_count = migrate_multifd_channels();
>      multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
>      multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
> +    qemu_mutex_init(&multifd_send_state->multifd_send_mutex);
>      qemu_sem_init(&multifd_send_state->channels_created, 0);
>      qemu_sem_init(&multifd_send_state->channels_ready, 0);
>      qatomic_set(&multifd_send_state->exiting, 0);
> 

-- 
Peter Xu