Re: [Qemu-devel] [PATCH v10 19/24] migration: Create multifd channels

[Juan Quintela <quintela@redhat.com>]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 12:00:05PM +0100, Juan Quintela wrote:
>> In both sides.  We still don't transmit anything through them.
>
> s/In/On/?
>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/ram.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 42 insertions(+), 10 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b57d9fd667..7ef0c2b7e2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -399,6 +399,7 @@ struct MultiFDSendParams {
>>      uint8_t id;
>>      char *name;
>>      QemuThread thread;
>> +    QIOChannel *c;
>>      QemuSemaphore sem;
>>      QemuMutex mutex;
>>      bool running;
>> @@ -455,6 +456,8 @@ int multifd_save_cleanup(Error **errp)
>>              qemu_thread_join(&p->thread);
>>              p->running = false;
>>          }
>> +        socket_send_channel_destroy(p->c);
>> +        p->c = NULL;
>>          qemu_mutex_destroy(&p->mutex);
>>          qemu_sem_destroy(&p->sem);
>>          g_free(p->name);
>> @@ -514,6 +517,27 @@ static void *multifd_send_thread(void *opaque)
>>      return NULL;
>>  }
>>  
>> +static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>> +{
>> +    MultiFDSendParams *p = opaque;
>> +    QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
>> +    Error *local_err = NULL;
>> +
>> +    if (qio_task_propagate_error(task, &local_err)) {
>> +        if (multifd_save_cleanup(&local_err) != 0) {
>
> Do we need to call multifd_save_cleanup() explicitly here?

Is the easiest way of stoping all multifd threads, no?

> Asked since I saw that it would also be called in migrate_fd_cleanup(),
> and it seems that we should call migrate_fd_cleanup() soon too when
> this happens?

We need to stop migraiton. thtat migrate_set_error() is only used for
reporting in info migrate, it is not acted upon.

Yes, perhaps it should, but as it is, it is not.  So, I think it is
right O:-)

Later, Juan.

> Otherwise it looks fine to me.  Thanks,
>
>> +            migrate_set_error(migrate_get_current(), local_err);
>> +        }
>> +    } else {
>> +        p->c = QIO_CHANNEL(sioc);
>> +        qio_channel_set_delay(p->c, false);
>> +        p->running = true;
>> +        qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>> +                           QEMU_THREAD_JOINABLE);
>> +
>> +        atomic_inc(&multifd_send_state->count);
>> +    }
>> +}
>
> [...]