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>
---
 migration/multifd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index e26f5f246d..92ae61a50f 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -731,9 +731,6 @@ static void *multifd_send_thread(void *opaque)
             if (flags & MULTIFD_FLAG_SYNC) {
                 qemu_sem_post(&p->sem_sync);
             }
-        } else if (p->quit) {
-            qemu_mutex_unlock(&p->mutex);
-            break;
         } else {
             qemu_mutex_unlock(&p->mutex);
             /* sometimes there are spurious wakeups */
-- 
2.35.3

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?

> ---
>  migration/multifd.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index e26f5f246d..92ae61a50f 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -731,9 +731,6 @@ static void *multifd_send_thread(void *opaque)
>              if (flags & MULTIFD_FLAG_SYNC) {
>                  qemu_sem_post(&p->sem_sync);
>              }
> -        } else if (p->quit) {
> -            qemu_mutex_unlock(&p->mutex);
> -            break;
>          } else {
>              qemu_mutex_unlock(&p->mutex);
>              /* sometimes there are spurious wakeups */

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.

Fabiano Rosas <farosas@suse.de> 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.

At the point that we are putting p->quit = true, we are really next to
where we put p->running = false.

I think we can move to use only p->running.

Later, Juan.

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