Re: [PATCH v2 25/29] migration/multifd: Support outgoing fixed-ram stream format

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Oct 23, 2023 at 05:36:04PM -0300, Fabiano Rosas wrote:
> The new fixed-ram stream format uses a file transport and puts ram
> pages in the migration file at their respective offsets and can be
> done in parallel by using the pwritev system call which takes iovecs
> and an offset.
> 
> Add support to enabling the new format along with multifd to make use
> of the threading and page handling already in place.
> 
> This requires multifd to stop sending headers and leaving the stream
> format to the fixed-ram code. When it comes time to write the data, we
> need to call a version of qio_channel_write that can take an offset.
> 
> Usage on HMP is:
> 
> (qemu) stop
> (qemu) migrate_set_capability multifd on
> (qemu) migrate_set_capability fixed-ram on
> (qemu) migrate_set_parameter max-bandwidth 0
> (qemu) migrate_set_parameter multifd-channels 8
> (qemu) migrate file:migfile
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/qemu/bitops.h | 13 ++++++++++
>  migration/multifd.c   | 55 +++++++++++++++++++++++++++++++++++++++++--
>  migration/options.c   |  6 -----
>  migration/ram.c       |  2 +-
>  4 files changed, 67 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index cb3526d1f4..2c0a2fe751 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -67,6 +67,19 @@ static inline void clear_bit(long nr, unsigned long *addr)
>      *p &= ~mask;
>  }
>  
> +/**
> + * clear_bit_atomic - Clears a bit in memory atomically
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + */
> +static inline void clear_bit_atomic(long nr, unsigned long *addr)
> +{
> +    unsigned long mask = BIT_MASK(nr);
> +    unsigned long *p = addr + BIT_WORD(nr);
> +
> +    return qatomic_and(p, ~mask);
> +}
> +
>  /**
>   * change_bit - Toggle a bit in memory
>   * @nr: Bit to change
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 20e8635740..3f95a41ee9 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -260,6 +260,19 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
>      g_free(pages);
>  }
>  
> +static void multifd_set_file_bitmap(MultiFDSendParams *p)
> +{
> +    MultiFDPages_t *pages = p->pages;
> +
> +    if (!pages->block) {
> +        return;
> +    }
> +
> +    for (int i = 0; i < p->normal_num; i++) {
> +        ramblock_set_shadow_bmap_atomic(pages->block, pages->offset[i]);
> +    }
> +}
> +
>  static void multifd_send_fill_packet(MultiFDSendParams *p)
>  {
>      MultiFDPacket_t *packet = p->packet;
> @@ -606,6 +619,29 @@ int multifd_send_sync_main(QEMUFile *f)
>          }
>      }
>  
> +    if (!migrate_multifd_packets()) {
> +        /*
> +         * There's no sync packet to send. Just make sure the sending
> +         * above has finished.
> +         */
> +        for (i = 0; i < migrate_multifd_channels(); i++) {
> +            qemu_sem_wait(&multifd_send_state->channels_ready);
> +        }
> +
> +        /* sanity check and release the channels */
> +        for (i = 0; i < migrate_multifd_channels(); i++) {
> +            MultiFDSendParams *p = &multifd_send_state->params[i];
> +
> +            qemu_mutex_lock(&p->mutex);
> +            assert(!p->pending_job || p->quit);
> +            qemu_mutex_unlock(&p->mutex);
> +
> +            qemu_sem_post(&p->sem);
> +        }
> +
> +        return 0;
> +    }
> +
>      /*
>       * 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
> @@ -689,6 +725,8 @@ static void *multifd_send_thread(void *opaque)
>  
>          if (p->pending_job) {
>              uint32_t flags;
> +            uint64_t write_base;
> +
>              p->normal_num = 0;
>  
>              if (!use_packets || use_zero_copy_send) {
> @@ -713,6 +751,16 @@ static void *multifd_send_thread(void *opaque)
>              if (use_packets) {
>                  multifd_send_fill_packet(p);
>                  p->num_packets++;
> +                write_base = 0;
> +            } else {
> +                multifd_set_file_bitmap(p);
> +
> +                /*
> +                 * If we subtract the host page now, we don't need to
> +                 * pass it into qio_channel_write_full_all() below.
> +                 */
> +                write_base = p->pages->block->pages_offset -
> +                    (uint64_t)p->pages->block->host;
>              }
>  
>              flags = p->flags;
> @@ -738,8 +786,9 @@ static void *multifd_send_thread(void *opaque)
>                  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);
> +            ret = qio_channel_write_full_all(p->c, p->iov, p->iovs_num,
> +                                             write_base, NULL, 0,
> +                                             p->write_flags, &local_err);
>              if (ret != 0) {
>                  break;
>              }
> @@ -969,6 +1018,8 @@ int multifd_save_setup(Error **errp)
>  
>          if (migrate_zero_copy_send()) {
>              p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> +        } else if (!use_packets) {
> +            p->write_flags |= QIO_CHANNEL_WRITE_FLAG_WITH_OFFSET;
>          } else {
>              p->write_flags = 0;
>          }

Ah, so this is why you had the wierd overloaded design for
qio_channel_write_full_all in patch 22 that I queried. I'd
still prefer the simpler design at the QIO level, and just
calling the appropriate function above. 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|