Re: [PATCH v4 3/4] migration: zero-copy flush only at the end of bitmap scanning

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> When sending memory pages with MSG_ZEROCOPY, it's necessary to flush
> to make sure all dirty pages are sent before a future version of them
> happens to be sent.
>
> Currently, the flush happens every time at the end of ram_save_iterate(),
> which usually happens around 20x per second, due to a timeout.
>
> Change so it flushes only after a whole scanning of the dirty bimap,
> so it never sends a newer version of a page before an older one, while
> avoiding unnecessary overhead.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

I agree that previous one is too many flushes, but this one changes to too
much memory to be uncommitted, and that is important because otherwise we
have unlimited amount of dirty memory.

> +/*
> + * Set zero_copy_flush = true for every multifd channel
> + *
> + * When using zero-copy, it's necessary to flush the pages before any of
> + * the pages can be sent again, so we'll make sure the new version of the
> + * pages will always arrive _later_ than the old pages.
> + *
> + * Should be called only after we finished one whole scanning of
> + * all the dirty bitmaps.
> + */
> +int multifd_zero_copy_flush(void)
> +{
> +    int i;
> +    Error *local_err = NULL;
> +
> +    if (!migrate_use_multifd()) {
> +        return 0;
> +    }
> +
> +    for (i = 0; i < migrate_multifd_channels(); i++) {
> +        MultiFDSendParams *p = &multifd_send_state->params[i];
> +        int ret;
> +
> +        ret = qio_channel_flush(p->c, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}


Here you flush every channel, Only at the end of a range you want to do this.


>  int multifd_send_sync_main(QEMUFile *f)
>  {
>      int i;
> -    bool flush_zero_copy;
>  
>      if (!migrate_use_multifd()) {
>          return 0;
> @@ -581,19 +613,6 @@ int multifd_send_sync_main(QEMUFile *f)
>          }
>      }
>  
> -    /*
> -     * When using zero-copy, it's necessary to flush the pages before any of
> -     * the pages can be sent again, so we'll make sure the new version of the
> -     * pages will always arrive _later_ than the old pages.
> -     *
> -     * Currently we achieve this by flushing the zero-page requested writes
> -     * per ram iteration, but in the future we could potentially optimize it
> -     * to be less frequent, e.g. only after we finished one whole scanning of
> -     * all the dirty bitmaps.
> -     */
> -
> -    flush_zero_copy = migrate_use_zero_copy_send();
> -
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
> @@ -615,17 +634,6 @@ int multifd_send_sync_main(QEMUFile *f)
>          ram_counters.transferred += p->packet_len;
>          qemu_mutex_unlock(&p->mutex);
>          qemu_sem_post(&p->sem);
> -
> -        if (flush_zero_copy && p->c) {
> -            int ret;
> -            Error *err = NULL;
> -
> -            ret = qio_channel_flush(p->c, &err);
> -            if (ret < 0) {
> -                error_report_err(err);
> -                return -1;
> -            }
> -        }

This synchronization already happens once every iteration through all
ram.

</me checks how>

And low and behold, it doesn't.

The problem here is that we are calling multifd_send_sync_main() in the
wrong place, i.e. we are being too conservative.

We need to call multifd_send_sync_main() just before doing
migration_bitmap_sync().  The reason that we need to call that function
is exactly the same that we need to flush for zero_copy.

So, what we need to change is remove the call to
multifd_send_sync_main(), not how it handles zero_copy.

>      }
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
> diff --git a/migration/ram.c b/migration/ram.c
> index 5f5e37f64d..514584e44f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2288,6 +2288,13 @@ static int ram_find_and_save_block(RAMState *rs)
>      rs->last_seen_block = pss.block;
>      rs->last_page = pss.page;
>  
> +    if (pss.complete_round && migrate_use_zero_copy_send()) {
> +        int ret = multifd_zero_copy_flush();
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +

This place is not right either, because we can have a sync in the middle
for other reasons.

We call migration_bitmap_sync() in save_live_pending(), and that is not
when we finish the complete_round.

Once discussed this, what I asked in the past is that you are having too
much dirty memory on zero_copy.  When you have a Multiterabyte guest, in
a single round you have a "potentially" dirty memory on each channel of:

   total_amount_memory / number of channels.

In a Multiterabyte guest, this is going to be more that probably in the
dozens of gigabytes.  As far as I know there is no card/driver that will
benefit for so many pages in zero_copy, and kernel will move to
synchronous copy at some point.  (In older threads, daniel showed how to
test for this case).

What I proposed is that you check in the migration_send_thread() how
much memory you have written since last synchronization.  Once that it
is big enough (I don't know the limits for card, in the docs that you
showed suggested the size is a few megabytes), you just sync at that
point and continue.

You still need to synchronize all threads at bitmap sync, but as said,
that is handled by multifd_send_sync_main(), and we should fix that
instead of changing where zero_copy flushes.

/* Removed not relevant bits of the function */

static void *multifd_send_thread(void *opaque)
{
    size_t zero_copy_bytes_sent = 0;

    ...

    while (true) {

            ....

            trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
                               p->flags, p->next_packet_size);

            if (use_zero_copy_send) {
                /* Send header first, without zerocopy */
                ret = qio_channel_write_all(p->c, (void *)p->packet,
                                            p->packet_len, &local_err);
                if (ret != 0) {
                    break;
                }

****** Note aside

Did you answered my question here of what happens when you do:

write_with_zero_copy(1MB); write_without_zero_copy(4KB); 
write_with_zero_copy(1MB); write_without_zero_copy(4KB);

My guess is that write_without_zero_copy(4KB) will just flush the
socket.  I can't think how it can work without doing that.
If so, we are not getting what we want.

            } else {
                /* Send header using the same writev call */
                p->iov[0].iov_len = p->packet_len;
                p->iov[0].iov_base = p->packet;
            }

            ret = qio_channel_writev_full_all(p->c, p->iov, p->iovs_num, NULL,
                                              0, p->write_flags, &local_err);
            if (ret != 0) {
                break;
            }

            qemu_mutex_lock(&p->mutex);
            p->num_packets++;
            p->total_normal_pages += p->normal_num;
            p->total_zero_pages += p->zero_num;
            p->pages->num = 0;
            p->pages->block = NULL;
            p->sent_bytes += p->packet_len;;
            p->sent_bytes += p->next_packet_size;

**** Addition
            zero_copy_bytes_sent += p->packet_len + p->next_packet_size;

            p->pending_job--;
            qemu_mutex_unlock(&p->mutex);
***** Addition
            if (zero_copy_bytes_sent > Threshold) // 2MB/4MB?  I don't know
                    ret = qio_channel_flush(p->c, &local_err);
                    // Handle error somehow
                    //  If you want to be a pro, just change the
                    // Threshold depending on what the kernel answers.
                    // If it has to revert to synchronous sent, just
                    // decrease the threshold, otherwise increase it.

            if (p->flags & MULTIFD_FLAG_SYNC) {
                qemu_sem_post(&p->sem_sync);
            }
            qemu_sem_post(&multifd_send_state->channels_ready);
        } else if (p->quit) {
            qemu_mutex_unlock(&p->mutex);
            break;
        } else {
            qemu_mutex_unlock(&p->mutex);
            /* sometimes there are spurious wakeups */
        }
    }
    .............
}

What do you think?

Later, Juan.