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