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

[Leonardo Bras Soares Passos <leobras@redhat.com>]

Hello Juan,

On Tue, Nov 2, 2021 at 9:32 AM Juan Quintela <quintela@redhat.com> wrote:
>
> 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.

Current multifd's sendmsg() will block until all data is sent, is that correct?

If that's the case, and supposing the network driver supports
multiqueue, maybe there is a small chance for this to happen.
I will add the flush at the end of each iteration, just to be sure.

>
> 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

Sure, I will try to divide the patch like this.

>
>
> >              '*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.

Could you please elaborate?

>
>
> > @@ -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);

Ok, I will try to make it work with this suggestion.

>
>
> > -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.

Ok, I will work on that.

>
> > @@ -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.
>
>

Ok, I will take a look on that.

> Later, Juan.
>

Thanks,
Leo