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

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> On receiving MIG_RPCOMM_REQ_PAGES look up the address and
> queue the page.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>


>      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....

> +        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 me wave a macro for this in qemu?

> 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?

> +    }
> +    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?

> +
> +    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?

> +    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?

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

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?

Thanks, Juan.