Re: [PATCH v4 08/23] multifd: Move iov from pages to params

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> This will allow us to reduce the number of system calls on the next patch.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/multifd.h |  8 ++++++--
> >>  migration/multifd.c | 34 ++++++++++++++++++++++++----------
> >>  2 files changed, 30 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index e57adc783b..c3f18af364 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -62,8 +62,6 @@ typedef struct {
> >>      uint64_t packet_num;
> >>      /* offset of each page */
> >>      ram_addr_t *offset;
> >> -    /* pointer to each page */
> >> -    struct iovec *iov;
> >>      RAMBlock *block;
> >>  } MultiFDPages_t;
> >>  
> >> @@ -110,6 +108,10 @@ typedef struct {
> >>      uint64_t num_pages;
> >>      /* syncs main thread and channels */
> >>      QemuSemaphore sem_sync;
> >> +    /* buffers to send */
> >> +    struct iovec *iov;
> >> +    /* number of iovs used */
> >> +    uint32_t iovs_num;
> >>      /* used for compression methods */
> >>      void *data;
> >>  }  MultiFDSendParams;
> >> @@ -149,6 +151,8 @@ typedef struct {
> >>      uint64_t num_pages;
> >>      /* syncs main thread and channels */
> >>      QemuSemaphore sem_sync;
> >> +    /* buffers to recv */
> >> +    struct iovec *iov;
> >
> > Why is there the asymmetry between send and recv, where the send
> > has the iovs_num and the recv doesn't?
> 
> When we are sending data, we have the normal page and the iov, so it is
> normal_pages + 1.  On reception side, we have to read first the header,
> because that is where normal_pages is stored.
> 
> I can drop iovs_num on the send side and add a comment, but I think that
> the new variable is more descriptive.
> 
> Or I can add iovs_num to the recv_side and just do a iovs_num =
> normal_pages, but it seems a bit pointless, no?

OK, it would be great to add a comment; because it jumps out as a little
odd.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> Later, Juan.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK