Re: [PATCH v3 21/23] multifd: Support for zero pages transmission

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Juan Quintela (quintela@redhat.com) wrote:
>> >> This patch adds counters and similar.  Logic will be added on the
>> >> following patch.
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  migration/multifd.h    | 13 ++++++++++++-
>> >>  migration/multifd.c    | 22 +++++++++++++++++++---
>> >>  migration/trace-events |  2 +-
>> >>  3 files changed, 32 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/migration/multifd.h b/migration/multifd.h
>> >> index 39e55d7f05..973315b545 100644
>> >> --- a/migration/multifd.h
>> >> +++ b/migration/multifd.h
>> >> @@ -49,7 +49,10 @@ typedef struct {
>> >>      /* size of the next packet that contains pages */
>> >>      uint32_t next_packet_size;
>> >>      uint64_t packet_num;
>> >> -    uint64_t unused[4];    /* Reserved for future use */
>> >> +    /* zero pages */
>> >> +    uint32_t zero_pages;
>> >
>> > Had you considered just adding a flag, MULTIFD_FLAG_ZERO to the packet?
>> 
>> I *have* to also add the flag.
>
> I meant can't you add a flag to say that this whole packet is zero pages
> and then you only need one counter.

No, in general packets are going to transmit *both*, zero pages and
normal pages.  It depends on the content that one receives.

Later, Juan.

> Dave
>
>> I was waiting for 7.0 to get out, because I still have to do the
>> compatibility bits.  Otherwise you can't migrate to an old multifd version.
>> 
>> >
>> >> +    uint32_t unused32[1];    /* Reserved for future use */
>> >> +    uint64_t unused64[3];    /* Reserved for future use */
>> >>      char ramblock[256];
>> >>      uint64_t offset[];
>> >>  } __attribute__((packed)) MultiFDPacket_t;
>> >> @@ -117,6 +120,10 @@ typedef struct {
>> >>      ram_addr_t *normal;
>> >>      /* num of non zero pages */
>> >>      uint32_t normal_num;
>> >> +    /* Pages that are  zero */
>> >> +    ram_addr_t *zero;
>> >> +    /* num of zero pages */
>> >> +    uint32_t zero_num;
>> >>      /* used for compression methods */
>> >>      void *data;
>> >>  }  MultiFDSendParams;
>> >> @@ -162,6 +169,10 @@ typedef struct {
>> >>      ram_addr_t *normal;
>> >>      /* num of non zero pages */
>> >>      uint32_t normal_num;
>> >> +    /* Pages that are  zero */
>> >> +    ram_addr_t *zero;
>> >> +    /* num of zero pages */
>> >> +    uint32_t zero_num;
>> >>      /* used for de-compression methods */
>> >>      void *data;
>> >>  } MultiFDRecvParams;
>> >> diff --git a/migration/multifd.c b/migration/multifd.c
>> >> index d1ab823f98..2e4dffd6c6 100644
>> >> --- a/migration/multifd.c
>> >> +++ b/migration/multifd.c
>> >> @@ -265,6 +265,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>> >>      packet->normal_pages = cpu_to_be32(p->normal_num);
>> >>      packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>> >>      packet->packet_num = cpu_to_be64(p->packet_num);
>> >> +    packet->zero_pages = cpu_to_be32(p->zero_num);
>> >>  
>> >>      if (p->pages->block) {
>> >>          strncpy(packet->ramblock, p->pages->block->idstr, 256);
>> >> @@ -327,7 +328,15 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> >>      p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>> >>      p->packet_num = be64_to_cpu(packet->packet_num);
>> >>  
>> >> -    if (p->normal_num == 0) {
>> >> +    p->zero_num = be32_to_cpu(packet->zero_pages);
>> >> +    if (p->zero_num > packet->pages_alloc - p->normal_num) {
>> >> +        error_setg(errp, "multifd: received packet "
>> >> +                   "with %d zero pages and expected maximum pages are %d",
>> >> +                   p->normal_num, packet->pages_alloc - p->zero_num) ;
>> >
>> > should that be p->zero_num, packet->pages_alloc - p->normal_num ?
>> > (and be %u)
>> 
>> Copy and paste error.  You are right on both cases.
>> 
>> Thanks.
>>