Re: [PATCH 04/14] migration/multifd: Postpone reset of MultiFDPages_t

[Fabiano Rosas <farosas@suse.de>]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> Now we reset MultiFDPages_t object in the multifd sender thread in the
> middle of the sending job.  That's not necessary, because the "*pages"
> struct will not be reused anyway until pending_job is cleared.
>
> Move that to the end after the job is completed, provide a helper to reset
> a "*pages" object.  Use that same helper when free the object too.
>
> This prepares us to keep using p->pages in the follow up patches, where we
> may drop p->normal[].
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Just an observation about the code below.

> ---
>  migration/multifd.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 2c98023d67..5633ac245a 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -172,6 +172,17 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
>      multifd_ops[method] = ops;
>  }
>  
> +/* Reset a MultiFDPages_t* object for the next use */
> +static void multifd_pages_reset(MultiFDPages_t *pages)
> +{
> +    /*
> +     * We don't need to touch offset[] array, because it will be
> +     * overwritten later when reused.
> +     */
> +    pages->num = 0;
> +    pages->block = NULL;

Having to do this at all is a huge overloading of this field. This not
only resets it, but it also communicates to multifd_queue_page() that
the previous payload has been sent. Otherwise, multifd_queue_page()
wouldn't know whether the previous call to multifd_queue_page() has
called multifd_send_pages() or if it has exited early. So this basically
means "the block that was previously here has been sent".

That's why we need the changed=true logic. A
multifd_send_state->pages->block still has a few pages left to send, but
because it's less than pages->allocated, it skips
multifd_send_pages(). The next call to multifd_queue_page() already has
the next ramblock. So we set changed=true, call multifd_send_pages() to
send the remaining pages of that block and recurse into
multifd_queue_page() once more to send the new block.

> +}
> +
>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>  {
>      MultiFDInit_t msg = {};
> @@ -248,9 +259,8 @@ static MultiFDPages_t *multifd_pages_init(uint32_t n)
>  
>  static void multifd_pages_clear(MultiFDPages_t *pages)
>  {
> -    pages->num = 0;
> +    multifd_pages_reset(pages);
>      pages->allocated = 0;
> -    pages->block = NULL;
>      g_free(pages->offset);
>      pages->offset = NULL;
>      g_free(pages);
> @@ -704,8 +714,6 @@ static void *multifd_send_thread(void *opaque)
>              p->flags = 0;
>              p->num_packets++;
>              p->total_normal_pages += p->normal_num;
> -            p->pages->num = 0;
> -            p->pages->block = NULL;
>              qemu_mutex_unlock(&p->mutex);
>  
>              trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> @@ -732,6 +740,8 @@ static void *multifd_send_thread(void *opaque)
>  
>              stat64_add(&mig_stats.multifd_bytes,
>                         p->next_packet_size + p->packet_len);
> +
> +            multifd_pages_reset(p->pages);
>              p->next_packet_size = 0;
>              qemu_mutex_lock(&p->mutex);
>              p->pending_job--;