Re: [PATCH] multifd: Implement yank for multifd send side

[Peter Xu]

On Wed, Sep 01, 2021 at 05:58:57PM +0200, Lukas Straub wrote:
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Tested-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
> ---
> 
> -v2:
>  -add Tested-by and Reviewed-by tags
> 
>  migration/multifd.c | 6 +++++-
>  migration/multifd.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..5a4f158f3c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>          Error *local_err = NULL;
>  
> +        if (p->registered_yank) {
> +            migration_ioc_unregister_yank(p->c);
> +        }
>          socket_send_channel_destroy(p->c);
>          p->c = NULL;
>          qemu_mutex_destroy(&p->mutex);
> @@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>                  return false;
>              }
>          } else {
> -            /* update for tls qio channel */
> +            migration_ioc_register_yank(ioc);
> +            p->registered_yank = true;
>              p->c = ioc;
>              qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>                                     QEMU_THREAD_JOINABLE);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8d6751f5ed..16c4d112d1 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -85,6 +85,8 @@ typedef struct {
>      bool running;
>      /* should this thread finish */
>      bool quit;
> +    /* is the yank function registered */
> +    bool registered_yank;
>      /* thread has work to do */
>      int pending_job;
>      /* array of pages to sent */
> -- 
> 2.32.0

This is probably yet another case that I'm wondering whether we made unregister
of yank function/instance too strict so we should loose them.

Logically a remove/unregister function doesn't need to assert and crash qemu if
the entry doesn't exist at all.  Then it's just something like "makes sure XXX
is removed", and noop if lookup failed.  The extra fields do not really help us
from anything else..

Thanks,

-- 
Peter Xu