Message ID | 20231012140651.13122-3-farosas@suse.de |
---|---|
State | New |
Headers | show |
Series | migration/multifd: Locking changes | expand |
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.
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,
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.
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 */
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(-)