Re: [PATCH v7 5/5] migration: add MULTIFD_RECV_SYNC migration command

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 04, 2025 at 01:40:02PM +0530, Prasad Pandit wrote:
> Hello Peter,
> 
> On Mon, 3 Mar 2025 at 20:20, Peter Xu <peterx@redhat.com> wrote:
> > We need the header.
> 
> * We need a section type, which is sent by qemu_savevm_command_send()
> as 'QEMU_VM_COMMAND'.

I think we need the header, the ram is a module.

> 
> >  Maybe the easiest as of now is one more hook like
> > qemu_savevm_state_complete_precopy_prepare(), and only use it in RAM as of
> > now.
> 
> * What will this helper do?

Do similarly like qemu_savevm_state_complete_precopy_iterable() but do
whatever a vmstate hander wants, so it'll be with a header.

> 
> > > * But earlier we discussed 'flush and sync' is enough for that, no?
> >
> > Yes it's ok I think, but this patch didn't do that.
> >
> > +            multifd_send_flush();
> > +            multifd_send_sync_main(MULTIFD_SYNC_LOCAL);
> > +            qemu_savevm_send_multifd_recv_sync(s->to_dst_file);
> >
> > I don't think it sent RAM_SAVE_FLAG_MULTIFD_FLUSH.  IIUC you need the
> > complete multifd_ram_flush_and_sync(), and the new message not needed.
> 
> * If we look at multifd_ram_flush_and_sync(), it does:
>      1. multifd_send()                       <= this patch does it via
> multifd_send_flush()
>      2. multifd_send_sync_main()    <= this patch also calls it above

MULTIFD_SYNC_LOCAL will not invoke MULTIFD_FLAG_SYNC, which we need.

>      3. send RAM_SAVE_FLAG_MULTIFD_FLUSH  <= this patch sends
> MIG_CMD_MULTIFD_RECV_SYNC

IMO we shouldn't make a global cmd for multifd.

> 
> * What is missing?
> 
> > Instead of I prepare the patch and whole commit message, please take your
> > time and think about it, justify it, and if you also think it works put
> > explanation into commit message and then we can go with it.
> 
> * The commit message does explain about flush and sync and how the
> migration command helps. What else do we need to add?

Please consider adding details like "we need message AAA on BBB channel to
serialize with CCC" and details.  Not asking that as required to merge, but
my understanding is that that's what is missing and that's why none of yet
versions can make sure of it in code.  Maybe that'll help you to understand
how that was serialized.

> 
> > > * And multifd_recv_sync_main() function on the destination blocks the
> > > 'main' thread until all multfd_recv_threads (mig/dst/recv_x) have
> > > exited, only then it proceeds to accept the incoming new postcopy
> > > connection.
> >
> > I don't think it makes sure threads have exited,
> 
> * 'multifd_recv_sync_main()' blocks the main thread on
> 'multifd_recv_state->sem_sync' semaphore. It is increased when
> multifd_recv threads exit due to the shutdown message. ie. the 'main'
> thread unblocks when all 'mig/dst/recv_x' threads have exited.

So is it your intention to not send MULTIFD_FLAG_SYNC above?

In all cases, I still think that's not the right way to do.

Thanks,

-- 
Peter Xu