Re: [PATCH v5 13/21] migration: Postcopy recover with preempt enabled

[Peter Xu <peterx@redhat.com>]

Hi, Manish,

On Mon, May 16, 2022 at 07:01:35PM +0530, manish.mishra wrote:
> 
> On 26/04/22 5:08 am, Peter Xu wrote:
> LGTM,
> Peter, I wanted to give review-tag for this and ealier patch too. I am new
> to qemu
> review process so not sure how give review-tag, did not find any reference
> on
> google too. So if you please let me know how to do it.

It's here:

https://git.qemu.org/?p=qemu.git;a=blob;f=docs/devel/submitting-a-patch.rst;hb=HEAD#l492

Since afaict QEMU is mostly following what Linux does, you can also
reference to this one with more context:

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

But since you're still having question regarding this patch, no rush on
providing your R-bs; let's finish the discussion first.

[...]

> > +static void postcopy_pause_ram_fast_load(MigrationIncomingState *mis)
> > +{
> > +    trace_postcopy_pause_fast_load();
> > +    qemu_mutex_unlock(&mis->postcopy_prio_thread_mutex);
> 
> I may have misunderstood synchronisation here but very very rare chance,
> 
> as both threads are working independently is it possible qemu_sem_post on
> 
> postcopy_pause_sem_fast_load is called by main thread even before we go
> 
> to qemu_sem_wait in next line, causing some kind of deadlock. That's should
> 
> not be possible as i understand it requires manually calling
> qmp_migration_recover
> 
> so chances are almost impossible. Just wanted to confirm it.

Sorry I don't quite get the question, could you elaborate?  E.g., when the
described deadlock happened, what is both main thread and preempt load
thread doing?  What are they waiting at?

Note: we have already released mutex before waiting on sem.

> 
> > +    qemu_sem_wait(&mis->postcopy_pause_sem_fast_load);
> 
> Just wanted to confirm why postcopy_pause_incoming is not called here
> itself.

postcopy_pause_incoming() is only used in the main ram load thread, while
this function (postcopy_pause_ram_fast_load) is only called by the preempt
load thread.

> 
> Is it based on assumption that if there is error in any of the channel it
> will
> 
> eventually be paused on source side, closing both channels, resulting
> 
> postcopy_pause_incoming will be called from main thread on destination?

Yes.

> 
> Usually it should be good to call as early as possible. It is left to main
> 
> thread default path so that we do not have any synchronisation overhead?

What's the sync overhead you mentioned? What we want to do here is simply
to put all the dest QEMU migration threads into a halted state rather than
quitting, so that they can be continued when necessary.

> 
> Also Peter, i was trying to understand postcopy recovery model so is use
> case
> 
> of qmp_migrate_pause just for debugging purpose?

Yes.  It's also a way to cleanly stop using the network (comparing to force
unplug the nic ports?) for whatever reason with a shutdown() syscall upon
the socket.  I just don't know whether there's any real use case of that in
reality.

Thanks,

-- 
Peter Xu