Re: [Qemu-devel] [PATCH v7 31/42] Page request: Process incoming page request

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > On receiving MIG_RPCOMM_REQ_PAGES look up the address and
> > queue the page.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> >      migrate_fd_cleanup_src_rp(s);
> >  
> > +    /* This queue generally should be empty - but in the case of a failed
> > +     * migration might have some droppings in.
> > +     */
> > +    struct MigrationSrcPageRequest *mspr, *next_mspr;
> > +    QSIMPLEQ_FOREACH_SAFE(mspr, &s->src_page_requests, next_req, next_mspr) {
> > +        QSIMPLEQ_REMOVE_HEAD(&s->src_page_requests, next_req);
> 
> How nice of QSIMPLEQ.  To remove elements you don't use mspr....

I guess I'm really just using the FOREACH as a while-not-empty.

> > +        g_free(mspr);
> > +    }
> > +
> >      if (s->file) {
> >          trace_migrate_fd_cleanup();
> >          qemu_mutex_unlock_iothread();
> > @@ -713,6 +729,8 @@ MigrationState *migrate_init(const MigrationParams *params)
> >      s->state = MIGRATION_STATUS_SETUP;
> >      trace_migrate_set_state(MIGRATION_STATUS_SETUP);
> >  
> > +    QSIMPLEQ_INIT(&s->src_page_requests);
> > +
> >      s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >      return s;
> >  }
> > @@ -976,7 +994,25 @@ static void source_return_path_bad(MigrationState *s)
> >  static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
> >                                         ram_addr_t start, ram_addr_t len)
> >  {
> > +    long our_host_ps = getpagesize();
> > +
> >      trace_migrate_handle_rp_req_pages(rbname, start, len);
> > +
> > +    /*
> > +     * Since we currently insist on matching page sizes, just sanity check
> > +     * we're being asked for whole host pages.
> > +     */
> > +    if (start & (our_host_ps-1) ||
> > +       (len & (our_host_ps-1))) {
> 
> 
> I don't know if creating a macro is a good idea?
> #define HOST_ALIGN_CHECK(addr)  (addr & (getpagesize()-1))
> 
> ???
> 
> Don't we have a macro for this in qemu?

Not that I can find; include/exec/cpu-all.h has a HOST_PAGE_ALIGN macro,
but it realigns an address to the boundary rather than being a test.
cpu-all.h also exposes a bunch of globals (e.g. qemu_host_page_size and
qemu_host_page_mask) that would simplify it, but being in cpu-all.h
it means I can't use it here because it won't let me include it in
generic code.  I guess that needs moving out of cpu-all.h to somewhere
else.

> > index f7d957e..da3e9ea 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -924,6 +924,69 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
> >  }
> >  
> >  /**
> > + * Queue the pages for transmission, e.g. a request from postcopy destination
> > + *   ms: MigrationStatus in which the queue is held
> > + *   rbname: The RAMBlock the request is for - may be NULL (to mean reuse last)
> > + *   start: Offset from the start of the RAMBlock
> > + *   len: Length (in bytes) to send
> > + *   Return: 0 on success
> > + */
> > +int ram_save_queue_pages(MigrationState *ms, const char *rbname,
> > +                         ram_addr_t start, ram_addr_t len)
> > +{
> > +    RAMBlock *ramblock;
> > +
> > +    rcu_read_lock();
> > +    if (!rbname) {
> > +        /* Reuse last RAMBlock */
> > +        ramblock = ms->last_req_rb;
> > +
> > +        if (!ramblock) {
> > +            /*
> > +             * Shouldn't happen, we can't reuse the last RAMBlock if
> > +             * it's the 1st request.
> > +             */
> > +            error_report("ram_save_queue_pages no previous block");
> > +            goto err;
> > +        }
> > +    } else {
> > +        ramblock = ram_find_block(rbname);
> > +
> > +        if (!ramblock) {
> > +            /* We shouldn't be asked for a non-existent RAMBlock */
> > +            error_report("ram_save_queue_pages no block '%s'", rbname);
> > +            goto err;
> > +        }
> 
>        Here?

Is that Here? The pointer for the next question?

> > +    }
> > +    trace_ram_save_queue_pages(ramblock->idstr, start, len);
> > +    if (start+len > ramblock->used_length) {
> > +        error_report("%s request overrun start=%zx len=%zx blocklen=%zx",
> > +                     __func__, start, len, ramblock->used_length);
> > +        goto err;
> > +    }
> > +
> > +    struct MigrationSrcPageRequest *new_entry =
> > +        g_malloc0(sizeof(struct MigrationSrcPageRequest));
> > +    new_entry->rb = ramblock;
> > +    new_entry->offset = start;
> > +    new_entry->len = len;
> > +    ms->last_req_rb = ramblock;
> 
> Can we move this line to the else?

Done.

> > +
> > +    qemu_mutex_lock(&ms->src_page_req_mutex);
> > +    memory_region_ref(ramblock->mr);
> 
> I haven't looked further in the patch series yet, but I can't see on
> this patch a memory_region_unref ....  Don't we need it?

No; we take the ref when we put it into the queue, and unref it when
we take it out of the queue (which is in the later patch).  Actually,
that does mean I need to unref when I drain the queue in that QSIMPLEQ_FOREACH;
I'll fix that.

> > +    QSIMPLEQ_INSERT_TAIL(&ms->src_page_requests, new_entry, next_req);
> > +    qemu_mutex_unlock(&ms->src_page_req_mutex);
> > +    rcu_read_unlock();
> 
> Of everything that we have inside the rcu_read_lock() .... Is there
> anything else that the memory_region_ref() that needs rcu?

The ram_find_block_by_id also needs it.

> Would not be possible to do the memory reference before asking for the
> mutex?

Yes; I've swapped that round so it's:
    memory_region_ref(ramblock->mr);
    qemu_mutex_lock(&ms->src_page_req_mutex);
    QSIMPLEQ_INSERT_TAIL(&ms->src_page_requests, new_entry, next_req);
    qemu_mutex_unlock(&ms->src_page_req_mutex);
    rcu_read_unlock();

> Once here, do we care about calling malloc with the rcu set?  or could
> we just call malloc at the beggining of the function and free it in case
> that it is not needed on err?

Why would that be better?

Dave

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