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

[Zheng Chuan <zhengchuan@huawei.com>]

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
>>

-- 
Regards.
Chuan