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

[Zheng Chuan]

On 2021/2/4 18:32, 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 |  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.
> 
OK. I will do that in V5.

>>          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?
> 
Yes, the multifd_load_cleanup cleans up all the channels with the iteration of 
migrate_multifd_channels.
Now, we just put multifd_rdma_cleanup into that iteration of 
multifd_load_cleanup to clear it one by one.
do you mean the name of multifd_rdma_cleanup is misleading and should changed 
it in order to distinguish with  multifd_load_cleanup?

> 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!
> 
I agree with you.
I will do that in V5 with typedefs.h.
>> +{
>> +    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
>>

-- 
Regards.
Chuan