[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC 4/7] migration: Drop MultiFDSendParams.quit and cleanup e
From: |
Fabiano Rosas |
Subject: |
Re: [PATCH RFC 4/7] migration: Drop MultiFDSendParams.quit and cleanup error paths |
Date: |
Mon, 23 Oct 2023 11:42:28 -0300 |
Peter Xu <peterx@redhat.com> writes:
> Multifd send side has two fields to indicate error quits:
>
> - MultiFDSendParams.quit
> - &multifd_send_state->exiting
>
> Merge them into the global one. The replacement is done by changing all
> p->quit checks into the global var check. The global check doesn't need
> any lock.
>
> A few more things done on top of this altogether:
>
> - multifd_send_terminate_threads()
>
> Moving the xchg() of &multifd_send_state->exiting upper, so as to cover
> the tracepoint, migrate_set_error() and migrate_set_state().
>
> - multifd_send_sync_main()
>
> In the 2nd loop, add one more check over the global var to make sure we
> don't keep the looping if QEMU already decided to quit.
>
> - multifd_tls_outgoing_handshake()
>
> Use multifd_send_terminate_threads() to set the error state. That has
> a benefit of updating MigrationState.error to that error too, so we can
> persist that 1st error we hit in that specific channel.
>
> - multifd_new_send_channel_async()
>
> Take similar approach like above, drop the migrate_set_error() because
> multifd_send_terminate_threads() already covers that. Unwrap the helper
> multifd_new_send_channel_cleanup() along the way; not really needed.
This all looks good to me. I had a very similar patch in the works. Just
one comment below.
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/multifd.h | 2 --
> migration/multifd.c | 82 ++++++++++++++-------------------------------
> 2 files changed, 26 insertions(+), 58 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index a835643b48..2acf400085 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -97,8 +97,6 @@ typedef struct {
> QemuMutex mutex;
> /* is this channel thread running */
> bool running;
> - /* should this thread finish */
> - bool quit;
> /* multifd flags for each packet */
> uint32_t flags;
> /* global number of generated multifd packets */
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 33fb21d0e4..9d458914a9 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -411,10 +411,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;
> - }
> -
I'd keep this. This function can be called from outside of multifd code
so the channels could be completely gone already.
[PATCH RFC 5/7] migration: Modulize multifd send threads with a few helpers, Peter Xu, 2023/10/22
[PATCH RFC 7/7] migration: Further unify paths for multifd normal or sync requests, Peter Xu, 2023/10/22
[PATCH RFC 6/7] migration: Split multifd pending_job into two booleans, Peter Xu, 2023/10/22