pet...@redhat.com writes:
> From: Peter Xu <pet...@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 <pet...@redhat.com>
Reviewed-by: Fabiano Rosas <faro...@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--;
