Re: [Qemu-devel] [PATCH v10 14/14, resend] rdma: add setup time accounting to QMP statistics

[Michael R. Hines]

On 06/24/2013 10:02 AM, Paolo Bonzini wrote:
> Il 24/06/2013 15:55, Michael R. Hines ha scritto:
> > > > Reviewed-by: Paolo Bonzini <address@hidden>
> > > Please stop inventing Reviewed-by tags, or I will stop reviewing your
> > > patches.
> > >
> > > Paolo
> > Inventing? I don't understand.
> >
> > I accumulated all of those tags from everybody - copy and pasted
> > directly from everyone when they gave them to me.\
> >
> > What exactly is wrong here?
> I didn't review this patch in particular, or at least it was changed
> very heavily since I last reviewed it.
>
> I certainly haven't reviewed this:

OK, so I need finer granularity with the tags. I didn't understand that 
until now.

I apologize for that.

> > > > +static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> > > >    {
> > > > -    if (__sync_val_compare_and_swap(&s->state, MIG_STATE_ACTIVE,
> > > > +    if (__sync_val_compare_and_swap(&s->state, old_state,
> > > >                                        new_state) == new_state) {
> and I wouldn't have approved it in fact.  In this shape, the patch
> should be split in at least 3 or 4 pieces.
>
> In v9 I asked to not apply patch 14 of v9 (I don't remember if you had
> already the R-b there); in v10 you rewrote patch 14 and I pointed out
> that the Reviewed-by tag was coming out of thin air; now you resent the
> series and left the wrong tag.  That's why I started to be a bit grumpy.
>
> By now you must have learnt that I want changes outside migration-rdma.c
> to be as a) minimal b) separate c) well-described as possible.
>
> What is _exactly_ the reason why you're changing the state machine?
> What is the change exactly, in fact?

Patch 14 is very important: Currently QEMU does not timestamp the setup 
phase.

On the other hand: RDMA uses memory pinning, which happens in the setup 
phase.

Unfortunately, since QEMU does not *explicitly* treat the setup phase 
like a fully-fledged
phase, this phase is unknown to the user.

As a result, pinning can take a very long time sometimes and it makes 
the migration command
(not the VM, just the command) appear to "hang" for several seconds when 
in reality,
the setup phase is just taking a long time for the pinning.

As you recommended previously, since the pinning now happens in the 
setup phase,
we need a patch which make the user aware of *why* the setup phase is 
taking such a long time.

Does that make sense?

This means that we cannot transition from MIG_STATE_SETUP => 
MIG_STATE_ACTIVE until
the pinniing has already completed (i.e. *after* qemu_savevm_state_begin()).

But the only way to do that is to call MIG_STATE_ACTIVE *inside* the 
migration thread.

- Michael


> Paolo