[PATCH 0/4] migration/multifd: trivial cleanup for multifd

[PATCH 0/4] migration/multifd: trivial cleanup for multifd

[Wei Yang]

Here are four trivial cleanups related to multifd.

Fix a typo, use a proper variable and setup never changed variables only once.

Wei Yang (4):
  migration/multifd: fix a typo in comment of
    multifd_recv_unfill_packet()
  migration/multifd: use pages->allocated instead of the static max
  migration/multifd: initialize packet->magic/version once at setup
    stage
  migration/multifd: pages->used would be cleared when attach to
    multifd_send_state

 migration/ram.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/4] migration/multifd: fix a typo in comment of multifd_recv_unfill_packet()

[Wei Yang]

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 22423f08cd..cf30171f44 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -838,7 +838,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 
     packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
     /*
-     * If we recevied a packet that is 100 times bigger than expected
+     * If we received a packet that is 100 times bigger than expected
      * just stop migration.  It is a magic number.
      */
     if (packet->pages_alloc > pages_max * 100) {
-- 
2.17.1

[PATCH 2/4] migration/multifd: use pages->allocated instead of the static max

[Wei Yang]

multifd_send_fill_packet() prepares meta data for following pages to
transfer. It would be more proper to fill pages->allocated instead of
static max value, especially we want to support flexible packet size.

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

diff --git a/migration/ram.c b/migration/ram.c
index cf30171f44..6a3bef0434 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -791,13 +791,12 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
 static void multifd_send_fill_packet(MultiFDSendParams *p)
 {
     MultiFDPacket_t *packet = p->packet;
-    uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size();
     int i;
 
     packet->magic = cpu_to_be32(MULTIFD_MAGIC);
     packet->version = cpu_to_be32(MULTIFD_VERSION);
     packet->flags = cpu_to_be32(p->flags);
-    packet->pages_alloc = cpu_to_be32(page_max);
+    packet->pages_alloc = cpu_to_be32(p->pages->allocated);
     packet->pages_used = cpu_to_be32(p->pages->used);
     packet->next_packet_size = cpu_to_be32(p->next_packet_size);
     packet->packet_num = cpu_to_be64(p->packet_num);
-- 
2.17.1

[PATCH 3/4] migration/multifd: initialize packet->magic/version once at setup stage

[Wei Yang]

MultiFDPacket_t's magic and version field never changes during
migration, so move these two fields in setup stage.

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

diff --git a/migration/ram.c b/migration/ram.c
index 6a3bef0434..71d845b851 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -793,8 +793,6 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
     MultiFDPacket_t *packet = p->packet;
     int i;
 
-    packet->magic = cpu_to_be32(MULTIFD_MAGIC);
-    packet->version = cpu_to_be32(MULTIFD_VERSION);
     packet->flags = cpu_to_be32(p->flags);
     packet->pages_alloc = cpu_to_be32(p->pages->allocated);
     packet->pages_used = cpu_to_be32(p->pages->used);
@@ -1240,6 +1238,8 @@ int multifd_save_setup(void)
         p->packet_len = sizeof(MultiFDPacket_t)
                       + sizeof(ram_addr_t) * page_count;
         p->packet = g_malloc0(p->packet_len);
+        p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
+        p->packet->version = cpu_to_be32(MULTIFD_VERSION);
         p->name = g_strdup_printf("multifdsend_%d", i);
         socket_send_channel_create(multifd_new_send_channel_async, p);
     }
-- 
2.17.1

[PATCH 4/4] migration/multifd: pages->used would be cleared when attach to multifd_send_state

[Wei Yang]

When we found an available channel in multifd_send_pages(), its
pages->used is cleared and then attached to multifd_send_state.

It is not necessary to do this twice.

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

diff --git a/migration/ram.c b/migration/ram.c
index 71d845b851..051de77d5a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1129,7 +1129,6 @@ static void *multifd_send_thread(void *opaque)
             p->flags = 0;
             p->num_packets++;
             p->num_pages += used;
-            p->pages->used = 0;
             qemu_mutex_unlock(&p->mutex);
 
             trace_multifd_send(p->id, packet_num, used, flags,
-- 
2.17.1

Re: [PATCH 1/4] migration/multifd: fix a typo in comment of multifd_recv_unfill_packet()

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> 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 22423f08cd..cf30171f44 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -838,7 +838,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>  
>      packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
>      /*
> -     * If we recevied a packet that is 100 times bigger than expected
> +     * If we received a packet that is 100 times bigger than expected
>       * just stop migration.  It is a magic number.
>       */
>      if (packet->pages_alloc > pages_max * 100) {

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

Re: [PATCH 3/4] migration/multifd: initialize packet->magic/version once at setup stage

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> MultiFDPacket_t's magic and version field never changes during
> migration, so move these two fields in setup stage.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

It don't really matter, and is faster your way O:-)

Re: [PATCH 4/4] migration/multifd: pages->used would be cleared when attach to multifd_send_state

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> When we found an available channel in multifd_send_pages(), its
> pages->used is cleared and then attached to multifd_send_state.
>
> It is not necessary to do this twice.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

Re: [PATCH 2/4] migration/multifd: use pages->allocated instead of the static max

[Juan Quintela]

Wei Yang <richardw.yang@linux.intel.com> wrote:
> multifd_send_fill_packet() prepares meta data for following pages to
> transfer. It would be more proper to fill pages->allocated instead of
> static max value, especially we want to support flexible packet size.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

It don't really matters.  We send full packets except the last one or
somesuch extern reason.  Only makes a difference if for that channel
only is sent a partial packet.

Re: [PATCH 3/4] migration/multifd: initialize packet->magic/version once at setup stage

[Wei Yang]

On Fri, Oct 11, 2019 at 12:20:48PM +0200, Juan Quintela wrote:
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>> MultiFDPacket_t's magic and version field never changes during
>> migration, so move these two fields in setup stage.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>It don't really matter, and is faster your way O:-)

You are right.

And I am wondering one more thing. Why we need to carry magic/version for each
packet? Would it be better to just carry and check magic/version for the
initial packet only?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/4] migration/multifd: trivial cleanup for multifd

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Here are four trivial cleanups related to multifd.
> 
> Fix a typo, use a proper variable and setup never changed variables only once.

Queued

> 
> Wei Yang (4):
>   migration/multifd: fix a typo in comment of
>     multifd_recv_unfill_packet()
>   migration/multifd: use pages->allocated instead of the static max
>   migration/multifd: initialize packet->magic/version once at setup
>     stage
>   migration/multifd: pages->used would be cleared when attach to
>     multifd_send_state
> 
>  migration/ram.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK