Re: [RFC PATCH v2 2/6] migration/multifd: Stop checking p->quit in multifd_send_thread

[Peter Xu <peterx@redhat.com>]

On Thu, Oct 19, 2023 at 11:58:13AM -0300, Fabiano Rosas wrote:
> Juan Quintela <quintela@redhat.com> writes:
> 
> > Fabiano Rosas <farosas@suse.de> wrote:
> >> We don't need to check p->quit in the multifd_send_thread() because it
> >> is shadowed by the 'exiting' flag. Ever since that flag was added
> >> p->quit became obsolete as a way to stop the thread.
> >>
> >> Since p->quit is set at multifd_send_terminate_threads() under the
> >> p->mutex lock, the thread will only see it once it loops, so 'exiting'
> >> will always be seen first.
> >>
> >> Note that setting p->quit at multifd_send_terminate_threads() still
> >> makes sense because we need a way to inform multifd_send_pages() that
> >> the channel has stopped.
> >>
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> >
> > But then should we remove the quit altogether?
> >
> 
> It still serves a purpose to allow multifd_send_pages() to see that the
> channel has exited. While that function does also check
> multifd_send_state->exiting, it could already be waiting at the mutex
> when the channel aborts. So we need to either check 'exiting' again or
> keep p->quit.

Sounds better to just move multifd_send_state->exiting check into the loop,
then drop the per-channel ->quit?

diff --git a/migration/multifd.c b/migration/multifd.c
index 1fe53d3b98..381a9a14e4 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -399,10 +399,6 @@ static int multifd_send_pages(QEMUFile *f)
     MultiFDSendParams *p = NULL; /* make happy gcc */
     MultiFDPages_t *pages = multifd_send_state->pages;
 
-    if (qatomic_read(&multifd_send_state->exiting)) {
-        return -1;
-    }
-
     qemu_sem_wait(&multifd_send_state->channels_ready);
     /*
      * next_channel can remain from a previous migration that was
@@ -413,12 +409,11 @@ static int multifd_send_pages(QEMUFile *f)
     for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
         p = &multifd_send_state->params[i];
 
-        qemu_mutex_lock(&p->mutex);
-        if (p->quit) {
-            error_report("%s: channel %d has already quit!", __func__, i);
-            qemu_mutex_unlock(&p->mutex);
+        if (qatomic_read(&multifd_send_state->exiting)) {
             return -1;
         }
+
+        qemu_mutex_lock(&p->mutex);
         if (!p->pending_job) {
             p->pending_job++;
             next_channel = (i + 1) % migrate_multifd_channels();

Thanks,

-- 
Peter Xu