Re: [RFC PATCH v2 5/6] migration/multifd: Stop setting 'quit' outside of channels

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We shouldn't really be touching channel state from outside the
> channels except for the transfer of pages.
>
> Move the setting of p->quit into the channel. Keep posting the 'sem'
> from multifd_send_terminate_threads() so any channel waiting on the
> semaphore will unblock and see the 'exiting' flag and quit by itself.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/multifd.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4ffa67339c..b7ba3fe0e6 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -497,13 +497,11 @@ static void multifd_send_terminate_threads(Error *err)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
> -        qemu_mutex_lock(&p->mutex);
> -        p->quit = true;
> +        /* kick the channel if it was waiting for work */
>          qemu_sem_post(&p->sem);
>          if (p->c) {
>              qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>          }
> -        qemu_mutex_unlock(&p->mutex);

Can we do two qio_channel_* operations at the same time on different
threads?  mutex is also protecting that.  This is a question, I don't
know the answer.

>      }
>  }
>  
> @@ -690,6 +688,9 @@ static void *multifd_send_thread(void *opaque)
>          qemu_sem_wait(&p->sem);
>  
>          if (qatomic_read(&multifd_send_state->exiting)) {
> +            qemu_mutex_lock(&p->mutex);
> +            p->quit = true;
> +            qemu_mutex_unlock(&p->mutex);
>              break;
>          }
>          qemu_mutex_lock(&p->mutex);
> @@ -765,6 +766,11 @@ static void *multifd_send_thread(void *opaque)

If we are going this route, we can just put it here, just in one place.

>  out:
>      if (local_err) {
>          trace_multifd_send_error(p->id);
> +
> +        qemu_mutex_lock(&p->mutex);
> +        p->quit = true;
> +        qemu_mutex_unlock(&p->mutex);
> +
>          multifd_send_terminate_threads(local_err);
>          error_free(local_err);
>      }

But now ->quit has basically the same meaning that ->running, so we
could probably merge both.

Later, Juan.