Re: [Qemu-devel] [PATCH v4 10/12] migration: create a dedicated thread to release rdma resource

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

* Lidong Chen (jemmy858585@gmail.com) wrote:
> ibv_dereg_mr wait for a long time for big memory size virtual server.
> 
> The test result is:
>   10GB  326ms
>   20GB  699ms
>   30GB  1021ms
>   40GB  1387ms
>   50GB  1712ms
>   60GB  2034ms
>   70GB  2457ms
>   80GB  2807ms
>   90GB  3107ms
>   100GB 3474ms
>   110GB 3735ms
>   120GB 4064ms
>   130GB 4567ms
>   140GB 4886ms
> 
> this will cause the guest os hang for a while when migration finished.
> So create a dedicated thread to release rdma resource.
> 
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
>  migration/rdma.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index dfa4f77..1b9e261 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2979,12 +2979,12 @@ static void qio_channel_rdma_set_aio_fd_handler(QIOChannel *ioc,
>      }
>  }
>  
> -static int qio_channel_rdma_close(QIOChannel *ioc,
> -                                  Error **errp)
> +static void *qio_channel_rdma_close_thread(void *arg)
>  {
> -    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> +    QIOChannelRDMA *rioc = arg;
>      RDMAContext *rdmain, *rdmaout;
> -    trace_qemu_rdma_close();
> +
> +    rcu_register_thread();
>  
>      rdmain = rioc->rdmain;
>      if (rdmain) {
> @@ -3009,6 +3009,19 @@ static int qio_channel_rdma_close(QIOChannel *ioc,
>      g_free(rdmain);
>      g_free(rdmaout);
>  
> +    rcu_unregister_thread();
> +    return NULL;
> +}
> +
> +static int qio_channel_rdma_close(QIOChannel *ioc,
> +                                  Error **errp)
> +{
> +    QemuThread t;
> +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> +    trace_qemu_rdma_close();
> +
> +    qemu_thread_create(&t, "rdma cleanup", qio_channel_rdma_close_thread,
> +                           rioc, QEMU_THREAD_DETACHED);

I don't think this can be this simple; consider the lock in patch 4;
now that lock means qui_channel_rdma_close() can't be called in
parallel; but with this change it means:


     f->lock
       qemu_thread_create  (1)
    !f->lock
     f->lock
       qemu_thread_create
    !f->lock

so we don't really protect the thing you were trying to lock

Dave

>      return 0;
>  }
>  
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK