Re: [Qemu-devel] [PATCH v12 17/21] migration: Create ram_multifd_page

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Wed, Apr 25, 2018 at 01:27:19PM +0200, Juan Quintela wrote:
> > The function still don't use multifd, but we have simplified
> > ram_save_page, xbzrle and RDMA stuff is gone.  We have added a new
> > counter.
> > 
> > Signed-off-by: Juan Quintela <address@hidden>
> > 
> > --
> > Add last_page parameter
> > Add commets for done and address
> > Remove multifd field, it is the same than normal pages
> > Merge next patch, now we send multiple pages at a time
> > Remove counter for multifd pages, it is identical to normal pages
> > Use iovec's instead of creating the equivalent.
> > Clear memory used by pages (dave)
> > Use g_new0(danp)
> > define MULTIFD_CONTINUE
> > now pages member is a pointer
> > Fix off-by-one in number of pages in one packet
> > Remove RAM_SAVE_FLAG_MULTIFD_PAGE
> > s/multifd_pages_t/MultiFDPages_t/
> > ---
> >  migration/ram.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 398cb0af3b..862ec53d32 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -54,6 +54,7 @@
> >  #include "migration/block.h"
> >  #include "sysemu/sysemu.h"
> >  #include "qemu/uuid.h"
> > +#include "qemu/iov.h"
> >  
> >  /***********************************************************/
> >  /* ram save/restore */
> > @@ -692,8 +693,65 @@ struct {
> >      QemuSemaphore sem_sync;
> >      /* global number of generated multifd packets */
> >      uint32_t seq;
> > +    /* send channels ready */
> > +    QemuSemaphore channels_ready;
> >  } *multifd_send_state;
> >  
> > +static void multifd_send_pages(void)
> > +{
> > +    int i;
> > +    static int next_channel;
> > +    MultiFDSendParams *p = NULL; /* make happy gcc */
> > +    MultiFDPages_t *pages = multifd_send_state->pages;
> > +
> > +    qemu_sem_wait(&multifd_send_state->channels_ready);
> 
> This sem is posted when a thread has finished its work.  However this
> is called in the main migration thread.  If with this line, are the
> threads really sending things in parallel?  Since it looks to me that
> this function (and the main thread) won't send the 2nd page array if
> the 1st hasn't finished, and won't send the 3rd if the 2nd hasn't,
> vice versa...
> 
> Maybe I misunderstood something.  Please feel free to correct me.

I share a similar misunderstanding;  except I can't understand how the
first item ever gets sent if we're waiting for channels_ready.
I think I could have understood it if there was a sem_post at the top of
multifd_send_thread.

Dave

> > +    for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
> > +        p = &multifd_send_state->params[i];
> > +
> > +        qemu_mutex_lock(&p->mutex);
> > +        if (!p->pending_job) {
> > +            p->pending_job++;
> > +            next_channel = (i + 1) % migrate_multifd_channels();
> > +            break;
> > +        }
> > +        qemu_mutex_unlock(&p->mutex);
> > +    }
> > +    p->pages->used = 0;
> > +    multifd_send_state->seq++;
> > +    p->seq = multifd_send_state->seq;
> > +    p->pages->block = NULL;
> > +    multifd_send_state->pages = p->pages;
> > +    p->pages = pages;
> 
> Here we directly replaced MultiFDSendParams.pages with
> multifd_send_state->pages.  Then are we always using a single
> MultiFDPages_t struct?  And if so, will all the initial
> MultiFDSendParams.pages memory leaked without freed?
> 
> > +    qemu_mutex_unlock(&p->mutex);
> > +    qemu_sem_post(&p->sem);
> > +}
> 
> Thanks,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK