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