[Qemu-devel] [PATCH 0/4] Multifd Cleanup

[Qemu-devel] [PATCH 0/4] Multifd Cleanup

[Wei Yang]

Just found several small places for unused variables.

Wei Yang (4):
  migration: multifd_save_setup always return 0
  migration/ram.c: use same type in MultiFDPages_t to define offsest
  migration/ram.c: MultiFDSendParams.sem_sync is not really used
  migration/ram.c: multifd_send_state->count is not really used

 migration/migration.c |  7 +------
 migration/ram.c       | 11 +----------
 2 files changed, 2 insertions(+), 16 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/4] migration: multifd_save_setup always return 0

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/migration.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d0a0f68f11..3aae4f2734 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3336,12 +3336,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         return;
     }
 
-    if (multifd_save_setup() != 0) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_FAILED);
-        migrate_fd_cleanup(s);
-        return;
-    }
+    multifd_save_setup();
     qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
                        QEMU_THREAD_JOINABLE);
     s->migration_thread_running = true;
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/4] migration: multifd_save_setup always return 0

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/migration.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index d0a0f68f11..3aae4f2734 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3336,12 +3336,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          return;
>      }
>  
> -    if (multifd_save_setup() != 0) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_FAILED);
> -        migrate_fd_cleanup(s);
> -        return;
> -    }
> +    multifd_save_setup();
>      qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>                         QEMU_THREAD_JOINABLE);
>      s->migration_thread_running = true;

Nack.

On the compression patches that are on list, multifd_save_setup()
returns -1 if there is a problem when we allocate a zbuff.

Re: [Qemu-devel] [PATCH 1/4] migration: multifd_save_setup always return 0

[Wei Yang]

On Tue, May 28, 2019 at 10:11:11AM +0200, Juan Quintela wrote:
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/migration.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index d0a0f68f11..3aae4f2734 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3336,12 +3336,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>          return;
>>      }
>>  
>> -    if (multifd_save_setup() != 0) {
>> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> -                          MIGRATION_STATUS_FAILED);
>> -        migrate_fd_cleanup(s);
>> -        return;
>> -    }
>> +    multifd_save_setup();
>>      qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>>                         QEMU_THREAD_JOINABLE);
>>      s->migration_thread_running = true;
>
>Nack.
>
>On the compression patches that are on list, multifd_save_setup()
>returns -1 if there is a problem when we allocate a zbuff.

You mean there are some patches in mail list?

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 1/4] migration: multifd_save_setup always return 0

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> On Tue, May 28, 2019 at 10:11:11AM +0200, Juan Quintela wrote:
>>Wei Yang <richardw.yang@linux.intel.com> wrote:
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  migration/migration.c | 7 +------
>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index d0a0f68f11..3aae4f2734 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -3336,12 +3336,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>>          return;
>>>      }
>>>  
>>> -    if (multifd_save_setup() != 0) {
>>> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>>> -                          MIGRATION_STATUS_FAILED);
>>> -        migrate_fd_cleanup(s);
>>> -        return;
>>> -    }
>>> +    multifd_save_setup();
>>>      qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>>>                         QEMU_THREAD_JOINABLE);
>>>      s->migration_thread_running = true;
>>
>>Nack.
>>
>>On the compression patches that are on list, multifd_save_setup()
>>returns -1 if there is a problem when we allocate a zbuff.
>
> You mean there are some patches in mail list?

Yeap.

Re: [Qemu-devel] [PATCH 1/4] migration: multifd_save_setup always return 0

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> On Tue, May 28, 2019 at 10:11:11AM +0200, Juan Quintela wrote:
>>Wei Yang <richardw.yang@linux.intel.com> wrote:
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  migration/migration.c | 7 +------
>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index d0a0f68f11..3aae4f2734 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -3336,12 +3336,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>>          return;
>>>      }
>>>  
>>> -    if (multifd_save_setup() != 0) {
>>> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>>> -                          MIGRATION_STATUS_FAILED);
>>> -        migrate_fd_cleanup(s);
>>> -        return;
>>> -    }
>>> +    multifd_save_setup();
>>>      qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
>>>                         QEMU_THREAD_JOINABLE);
>>>      s->migration_thread_running = true;
>>
>>Nack.
>>
>>On the compression patches that are on list, multifd_save_setup()
>>returns -1 if there is a problem when we allocate a zbuff.
>
> You mean there are some patches in mail list?

Yeap.

[Qemu-devel] [PATCH 2/4] migration/ram.c: use same type in MultiFDPages_t to define offsest

[Wei Yang]

MultiFDPacket_t.offset is allocated to store MultiFDPages_t.offset.

It would be better to use the same type.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 4c60869226..dcf4c54eb5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -607,7 +607,7 @@ typedef struct {
     uint64_t packet_num;
     uint64_t unused[4];    /* Reserved for future use */
     char ramblock[256];
-    uint64_t offset[];
+    ram_addr_t offset[];
 } __attribute__((packed)) MultiFDPacket_t;
 
 typedef struct {
-- 
2.19.1

Re: [Qemu-devel] [PATCH 2/4] migration/ram.c: use same type in MultiFDPages_t to define offsest

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> MultiFDPacket_t.offset is allocated to store MultiFDPages_t.offset.
>
> It would be better to use the same type.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 4c60869226..dcf4c54eb5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -607,7 +607,7 @@ typedef struct {
>      uint64_t packet_num;
>      uint64_t unused[4];    /* Reserved for future use */
>      char ramblock[256];
> -    uint64_t offset[];
> +    ram_addr_t offset[];
>  } __attribute__((packed)) MultiFDPacket_t;
>  
>  typedef struct {

This needs a comment, but it is on purpose.  We want that the value on
the wire to be the same for any architecture.  (Migration stream is
supposed to be architecture independent).  ram_addr_t is architecture
dependent.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/4] migration/ram.c: use same type in MultiFDPages_t to define offsest

[Wei Yang]

On Tue, May 28, 2019 at 10:12:39AM +0200, Juan Quintela wrote:
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>> MultiFDPacket_t.offset is allocated to store MultiFDPages_t.offset.
>>
>> It would be better to use the same type.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 4c60869226..dcf4c54eb5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -607,7 +607,7 @@ typedef struct {
>>      uint64_t packet_num;
>>      uint64_t unused[4];    /* Reserved for future use */
>>      char ramblock[256];
>> -    uint64_t offset[];
>> +    ram_addr_t offset[];
>>  } __attribute__((packed)) MultiFDPacket_t;
>>  
>>  typedef struct {
>
>This needs a comment, but it is on purpose.  We want that the value on
>the wire to be the same for any architecture.  (Migration stream is
>supposed to be architecture independent).  ram_addr_t is architecture
>dependent.
>

Sounds reasonable.

>Later, Juan.

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [PATCH 3/4] migration/ram.c: MultiFDSendParams.sem_sync is not really used

[Wei Yang]

Besides init and destroy, MultiFDSendParams.sem_sync is not really used.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index dcf4c54eb5..5d31f7bd4c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -661,8 +661,6 @@ typedef struct {
     uint64_t num_packets;
     /* pages sent through this channel */
     uint64_t num_pages;
-    /* syncs main thread and channels */
-    QemuSemaphore sem_sync;
 }  MultiFDSendParams;
 
 typedef struct {
@@ -1027,7 +1025,6 @@ void multifd_save_cleanup(void)
         p->c = NULL;
         qemu_mutex_destroy(&p->mutex);
         qemu_sem_destroy(&p->sem);
-        qemu_sem_destroy(&p->sem_sync);
         g_free(p->name);
         p->name = NULL;
         multifd_pages_clear(p->pages);
@@ -1201,7 +1198,6 @@ int multifd_save_setup(void)
 
         qemu_mutex_init(&p->mutex);
         qemu_sem_init(&p->sem, 0);
-        qemu_sem_init(&p->sem_sync, 0);
         p->quit = false;
         p->pending_job = 0;
         p->id = i;
-- 
2.19.1

Re: [Qemu-devel] [PATCH 3/4] migration/ram.c: MultiFDSendParams.sem_sync is not really used

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> Besides init and destroy, MultiFDSendParams.sem_sync is not really used.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I mad SendParamas and RecvParams identical, but they are different.  You
are right.

Re: [Qemu-devel] [PATCH 3/4] migration/ram.c: MultiFDSendParams.sem_sync is not really used

[Wei Yang]

On Tue, May 28, 2019 at 10:16:06AM +0200, Juan Quintela wrote:
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>> Besides init and destroy, MultiFDSendParams.sem_sync is not really used.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>I mad SendParamas and RecvParams identical, but they are different.  You
>are right.

Thanks.

BTW, I found some interesting thing about multifd_send_state->channels_ready.
By checking the value of this semaphore, it grows far beyond what we really
have.

For example, we have default 2 channels which means
multifd_send_state->channels_ready's value is no more than 2. But the actual
value could go to more than 30.

The behavior sounds not right.

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [PATCH 4/4] migration/ram.c: multifd_send_state->count is not really used

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5d31f7bd4c..c9a9f7489b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -892,8 +892,6 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 
 struct {
     MultiFDSendParams *params;
-    /* number of created threads */
-    int count;
     /* array of pages to sent */
     MultiFDPages_t *pages;
     /* syncs main thread and channels */
@@ -1171,8 +1169,6 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
         p->running = true;
         qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
                            QEMU_THREAD_JOINABLE);
-
-        atomic_inc(&multifd_send_state->count);
     }
 }
 
@@ -1188,7 +1184,6 @@ int multifd_save_setup(void)
     thread_count = migrate_multifd_channels();
     multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
     multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
-    atomic_set(&multifd_send_state->count, 0);
     multifd_send_state->pages = multifd_pages_init(page_count);
     qemu_sem_init(&multifd_send_state->sem_sync, 0);
     qemu_sem_init(&multifd_send_state->channels_ready, 0);
-- 
2.19.1

Re: [Qemu-devel] [PATCH 4/4] migration/ram.c: multifd_send_state->count is not really used

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

Once upon a time, we checked how many threads were created to recover
for errors, but this was wrong because one of the "missing" threads
could be one different from the last one.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 0/4] Multifd Cleanup

[Wei Yang]

Hi, David

Do you prefer I resend 3/4 or it is ok for you to pick up here directly?

On Tue, May 28, 2019 at 09:46:59AM +0800, Wei Yang wrote:
>Just found several small places for unused variables.
>
>Wei Yang (4):
>  migration: multifd_save_setup always return 0
>  migration/ram.c: use same type in MultiFDPages_t to define offsest
>  migration/ram.c: MultiFDSendParams.sem_sync is not really used
>  migration/ram.c: multifd_send_state->count is not really used
>
> migration/migration.c |  7 +------
> migration/ram.c       | 11 +----------
> 2 files changed, 2 insertions(+), 16 deletions(-)
>
>-- 
>2.19.1

-- 
Wei Yang
Help you, Help me