Re: [PATCH v4 3/3] multifd: Implement zerocopy write in multifd migration (multifd-zerocopy)

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Implement zerocopy on nocomp_send_write(), by making use of QIOChannel
> zerocopy interface.
>
> Change multifd_send_sync_main() so it can distinguish the last sync from
> the setup and per-iteration ones, so a flush_zerocopy() can be called
> at the last sync in order to make sure all RAM is sent before finishing
> the migration.

You need to do this after each iteration.  Otherwise it can happen that:

channel 1:               channel 2:

   send page 11

next iteration
                         send page 11

                         this page arrives

now arrives this old copy.

After each iteration, one needs to be sure that no ram is inflight.

This means that I think you don't need the last_sync parameter at all,
as you have to do the flush() in every iteration.

> Also make it return -1 if flush_zerocopy() fails, in order to cancel
> the migration process, and avoid resuming the guest in the target host
> without receiving all current RAM.
>
> This will work fine on RAM migration because the RAM pages are not usually 
> freed,
> and there is no problem on changing the pages content between async_send() and
> the actual sending of the buffer, because this change will dirty the page and
> cause it to be re-sent on a next iteration anyway.
>
> Given a lot of locked memory may be needed in order to use multid migration
> with zerocopy enabled, make it optional by creating a new parameter
> multifd-zerocopy on qapi, so low-privileged users can still perform multifd
> migrations.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

I think it is better that you split this patch into two:

- Add the new parameter (it looks good to me, and can be reviewed-by)
- Implement the feature, here probably you need more changes/review


>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> +         '*multifd-zerocopy': 'bool',
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }

Something weird here.

>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> +         '*multifd-zerocopy': 'bool',
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>  

Same here.


> @@ -105,7 +105,13 @@ static int nocomp_send_prepare(MultiFDSendParams *p, 
> uint32_t used,
>   */
>  static int nocomp_send_write(MultiFDSendParams *p, uint32_t used, Error 
> **errp)
>  {
> -    return qio_channel_writev_all(p->c, p->pages->iov, used, errp);
> +    int flags = 0;
> +
> +    if (migrate_multifd_zerocopy()) {
> +        flags = QIO_CHANNEL_WRITE_FLAG_ZEROCOPY;
> +    }

You have added an if on each write, just add it during initialization.

There is already a uint32_t flags field in MultiFDRecvParams, but you
can add a

int write_flags;

one and add it during initialization.  That way you don't need any check
here, just pass it.

> +    return qio_channel_writev_all_flags(p->c, p->pages->iov, used, flags, 
> errp);


> -void multifd_send_sync_main(QEMUFile *f)
> +int multifd_send_sync_main(QEMUFile *f, bool last_sync)

As you need to check every round, you now have to check for errors on
every multifd_send_sync_main() call.  It really looked weird that you
only need to check it sometimes.

> @@ -3006,13 +3006,19 @@ static int ram_save_complete(QEMUFile *f, void 
> *opaque)
>          ram_control_after_iterate(f, RAM_CONTROL_FINISH);
>      }
>  
> -    if (ret >= 0) {
> -        multifd_send_sync_main(rs->f);
> -        qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> -        qemu_fflush(f);
> +    if (ret < 0) {
> +        return ret;
>      }
>  
> -    return ret;
> +    ret = multifd_send_sync_main(rs->f, true);
> +    if (ret < 0) {
> +        return -1;

Why are you returning -1 instead of ret?

Callers of ram_save_complete(). We set qemu_error_file() with that
error, so it is not a good idea to reset it.


Later, Juan.