Re: [PATCH v4 18/18] migration/rdma: RDMA cleanup for multifd migration

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/multifd.c |  6 ++++++
>  migration/multifd.h |  1 +
>  migration/rdma.c    | 16 +++++++++++++++-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 1186246..4031648 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -577,6 +577,9 @@ void multifd_save_cleanup(void)
>          p->packet_len = 0;
>          g_free(p->packet);
>          p->packet = NULL;
> +#ifdef CONFIG_RDMA
> +        multifd_rdma_cleanup(p->rdma);
> +#endif

You may find it easier to add an entry into stubs/ for
multifd_rdma_cleanup; it then avoids the need for the ifdef.

>          multifd_send_state->ops->send_cleanup(p, &local_err);
>          if (local_err) {
>              migrate_set_error(migrate_get_current(), local_err);
> @@ -1039,6 +1042,9 @@ int multifd_load_cleanup(Error **errp)
>          p->packet_len = 0;
>          g_free(p->packet);
>          p->packet = NULL;
> +#ifdef CONFIG_RDMA
> +        multifd_rdma_cleanup(p->rdma);
> +#endif
>          multifd_recv_state->ops->recv_cleanup(p);
>      }
>      qemu_sem_destroy(&multifd_recv_state->sem_sync);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 26d4489..0ecec5e 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -183,6 +183,7 @@ typedef struct {
>  
>  #ifdef CONFIG_RDMA
>  extern MultiFDSetup multifd_rdma_ops;
> +void multifd_rdma_cleanup(void *opaque);
>  #endif
>  void multifd_send_terminate_threads(Error *err);
>  int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp);
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c19a91f..f14357f 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2369,7 +2369,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>  {
>      int idx;
>  
> -    if (rdma->cm_id && rdma->connected) {
> +    if (rdma->channel && rdma->cm_id && rdma->connected) {
>          if ((rdma->error_state ||
>               migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) &&
>              !rdma->received_error) {
> @@ -4599,6 +4599,20 @@ static void multifd_rdma_recv_channel_setup(QIOChannel *ioc,
>      return;
>  }
>  
> +void multifd_rdma_cleanup(void *opaque)

I think you need to make it clear that this is only to cleanup one
channel, rather than the whole multifd-rdma connection;
multifd_load_cleanup for example cleans up all the channels, where as I
think this is only doing one?

Don't use a 'void *opaque' except for something that's called via
a registration/callback scheme that's designed to be generic
(e.g. multifd_send_thread does it because it's called from
qemu_thread_create that doesn't know the type).  Where you know
the type, use it!

> +{
> +    RDMAContext *rdma = (RDMAContext *)opaque;
> +
> +    if (!migrate_use_rdma()) {
> +        return;
> +    }
> +
> +    rdma->listen_id = NULL;
> +    rdma->channel = NULL;
> +    qemu_rdma_cleanup(rdma);
> +    g_free(rdma);
> +}
> +
>  MultiFDSetup multifd_rdma_ops = {
>      .send_thread = multifd_rdma_send_thread,
>      .recv_thread = multifd_rdma_recv_thread,
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK