[v3,04/18] migration/rdma: add multifd_setup_ops for rdma

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/multifd.c |  6 ++++
 migration/multifd.h |  4 +++
 migration/rdma.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/multifd.c |  6 ++++
>  migration/multifd.h |  4 +++
>  migration/rdma.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 1f82307..0d494df 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -1210,6 +1210,12 @@ MultiFDSetup *multifd_setup_ops_init(void)
>  {
>      MultiFDSetup *ops;
>  
> +#ifdef CONFIG_RDMA
> +    if (migrate_use_rdma()) {
> +        ops = multifd_rdma_setup();
> +        return ops;
> +    }
> +#endif
>      ops = &multifd_socket_ops;
>      return ops;
>  }
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 446315b..62a0b2a 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -173,6 +173,10 @@ typedef struct {
>      void (*recv_channel_setup)(QIOChannel *ioc, MultiFDRecvParams *p);
>  } MultiFDSetup;
>  
> +#ifdef CONFIG_RDMA
> +MultiFDSetup *multifd_rdma_setup(void);
> +#endif
> +MultiFDSetup *multifd_setup_ops_init(void);
>  void multifd_register_ops(int method, MultiFDMethods *ops);
>  
>  #endif
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 0340841..ad4e4ba 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -19,6 +19,7 @@
>  #include "qemu/cutils.h"
>  #include "rdma.h"
>  #include "migration.h"
> +#include "multifd.h"
>  #include "qemu-file.h"
>  #include "ram.h"
>  #include "qemu-file-channel.h"
> @@ -4138,3 +4139,84 @@ err:
>      g_free(rdma);
>      g_free(rdma_return_path);
>  }
> +
> +static void *multifd_rdma_send_thread(void *opaque)
> +{
> +    MultiFDSendParams *p = opaque;
> +
> +    while (true) {
> +        qemu_mutex_lock(&p->mutex);
> +        if (p->quit) {
> +            qemu_mutex_unlock(&p->mutex);
> +            break;
> +        }
> +        qemu_mutex_unlock(&p->mutex);
> +        qemu_sem_wait(&p->sem);
> +    }
> +
> +    qemu_mutex_lock(&p->mutex);
> +    p->running = false;
> +    qemu_mutex_unlock(&p->mutex);
> +
> +    return NULL;
> +}

You might like to consider using WITH_QEMU_LOCK_GUARD, I think that
would become:

  while (true) {
      WITH_QEMU_LOCK_GUARD(&p->mutex) {
          if (p->quit) {
              break;
          }
      }
      qemu_sem_wait(&p->sem);
  }
  WITH_QEMU_LOCK_GUARD(&p->mutex) {
      p->running = false;
  }

> +
> +static void multifd_rdma_send_channel_setup(MultiFDSendParams *p)
> +{
> +    Error *local_err = NULL;
> +
> +    if (p->quit) {
> +        error_setg(&local_err, "multifd: send id %d already quit", p->id);
> +        return ;
> +    }
> +    p->running = true;
> +
> +    qemu_thread_create(&p->thread, p->name, multifd_rdma_send_thread, p,
> +                       QEMU_THREAD_JOINABLE);
> +}
> +
> +static void *multifd_rdma_recv_thread(void *opaque)
> +{
> +    MultiFDRecvParams *p = opaque;
> +
> +    while (true) {
> +        qemu_mutex_lock(&p->mutex);
> +        if (p->quit) {
> +            qemu_mutex_unlock(&p->mutex);
> +            break;
> +        }
> +        qemu_mutex_unlock(&p->mutex);
> +        qemu_sem_wait(&p->sem_sync);
> +    }
> +
> +    qemu_mutex_lock(&p->mutex);
> +    p->running = false;
> +    qemu_mutex_unlock(&p->mutex);
> +
> +    return NULL;
> +}
> +
> +static void multifd_rdma_recv_channel_setup(QIOChannel *ioc,
> +                                            MultiFDRecvParams *p)
> +{
> +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> +
> +    p->file = rioc->file;
> +    return;
> +}
> +
> +static MultiFDSetup multifd_rdma_ops = {
> +    .send_thread_setup = multifd_rdma_send_thread,
> +    .recv_thread_setup = multifd_rdma_recv_thread,
> +    .send_channel_setup = multifd_rdma_send_channel_setup,
> +    .recv_channel_setup = multifd_rdma_recv_channel_setup
> +};
> +
> +MultiFDSetup *multifd_rdma_setup(void)
> +{
> +    MultiFDSetup *rdma_ops;
> +
> +    rdma_ops = &multifd_rdma_ops;
> +
> +    return rdma_ops;

Why bother making this a function - just export multifd_rdma_ops ?

Dave

> +}
> -- 
> 1.8.3.1
>

On 2020/11/10 20:30, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/multifd.c |  6 ++++
>>  migration/multifd.h |  4 +++
>>  migration/rdma.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 92 insertions(+)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 1f82307..0d494df 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -1210,6 +1210,12 @@ MultiFDSetup *multifd_setup_ops_init(void)
>>  {
>>      MultiFDSetup *ops;
>>  
>> +#ifdef CONFIG_RDMA
>> +    if (migrate_use_rdma()) {
>> +        ops = multifd_rdma_setup();
>> +        return ops;
>> +    }
>> +#endif
>>      ops = &multifd_socket_ops;
>>      return ops;
>>  }
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 446315b..62a0b2a 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -173,6 +173,10 @@ typedef struct {
>>      void (*recv_channel_setup)(QIOChannel *ioc, MultiFDRecvParams *p);
>>  } MultiFDSetup;
>>  
>> +#ifdef CONFIG_RDMA
>> +MultiFDSetup *multifd_rdma_setup(void);
>> +#endif
>> +MultiFDSetup *multifd_setup_ops_init(void);
>>  void multifd_register_ops(int method, MultiFDMethods *ops);
>>  
>>  #endif
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 0340841..ad4e4ba 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -19,6 +19,7 @@
>>  #include "qemu/cutils.h"
>>  #include "rdma.h"
>>  #include "migration.h"
>> +#include "multifd.h"
>>  #include "qemu-file.h"
>>  #include "ram.h"
>>  #include "qemu-file-channel.h"
>> @@ -4138,3 +4139,84 @@ err:
>>      g_free(rdma);
>>      g_free(rdma_return_path);
>>  }
>> +
>> +static void *multifd_rdma_send_thread(void *opaque)
>> +{
>> +    MultiFDSendParams *p = opaque;
>> +
>> +    while (true) {
>> +        qemu_mutex_lock(&p->mutex);
>> +        if (p->quit) {
>> +            qemu_mutex_unlock(&p->mutex);
>> +            break;
>> +        }
>> +        qemu_mutex_unlock(&p->mutex);
>> +        qemu_sem_wait(&p->sem);
>> +    }
>> +
>> +    qemu_mutex_lock(&p->mutex);
>> +    p->running = false;
>> +    qemu_mutex_unlock(&p->mutex);
>> +
>> +    return NULL;
>> +}
> 
> You might like to consider using WITH_QEMU_LOCK_GUARD, I think that
> would become:
> 
>   while (true) {
>       WITH_QEMU_LOCK_GUARD(&p->mutex) {
>           if (p->quit) {
>               break;
>           }
>       }
>       qemu_sem_wait(&p->sem);
>   }
>   WITH_QEMU_LOCK_GUARD(&p->mutex) {
>       p->running = false;
>   }
> 
OK. and this remind me now we keep qemu_mutex_lock(&p->mutex); in our multifd code, it that should also optimized?
>> +
>> +static void multifd_rdma_send_channel_setup(MultiFDSendParams *p)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    if (p->quit) {
>> +        error_setg(&local_err, "multifd: send id %d already quit", p->id);
>> +        return ;
>> +    }
>> +    p->running = true;
>> +
>> +    qemu_thread_create(&p->thread, p->name, multifd_rdma_send_thread, p,
>> +                       QEMU_THREAD_JOINABLE);
>> +}
>> +
>> +static void *multifd_rdma_recv_thread(void *opaque)
>> +{
>> +    MultiFDRecvParams *p = opaque;
>> +
>> +    while (true) {
>> +        qemu_mutex_lock(&p->mutex);
>> +        if (p->quit) {
>> +            qemu_mutex_unlock(&p->mutex);
>> +            break;
>> +        }
>> +        qemu_mutex_unlock(&p->mutex);
>> +        qemu_sem_wait(&p->sem_sync);
>> +    }
>> +
>> +    qemu_mutex_lock(&p->mutex);
>> +    p->running = false;
>> +    qemu_mutex_unlock(&p->mutex);
>> +
>> +    return NULL;
>> +}
>> +
>> +static void multifd_rdma_recv_channel_setup(QIOChannel *ioc,
>> +                                            MultiFDRecvParams *p)
>> +{
>> +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
>> +
>> +    p->file = rioc->file;
>> +    return;
>> +}
>> +
>> +static MultiFDSetup multifd_rdma_ops = {
>> +    .send_thread_setup = multifd_rdma_send_thread,
>> +    .recv_thread_setup = multifd_rdma_recv_thread,
>> +    .send_channel_setup = multifd_rdma_send_channel_setup,
>> +    .recv_channel_setup = multifd_rdma_recv_channel_setup
>> +};
>> +
>> +MultiFDSetup *multifd_rdma_setup(void)
>> +{
>> +    MultiFDSetup *rdma_ops;
>> +
>> +    rdma_ops = &multifd_rdma_ops;
>> +
>> +    return rdma_ops;
> 
> Why bother making this a function - just export multifd_rdma_ops ?
> 
> Dave
> 
OK, will consider in that way.

>> +}
>> -- 
>> 1.8.3.1
>>