Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Rework the migration thread to setup and start postcopy.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/migration/migration.h |   3 +
> >  migration.c                   | 201 
> > ++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 185 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index b01cc17..f401775 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -125,6 +125,9 @@ struct MigrationState
> >      /* Flag set once the migration has been asked to enter postcopy */
> >      volatile bool start_postcopy;
> >  
> > +    /* Flag set once the migration thread is running (and needs joining) */
> > +    volatile bool started_migration_thread;
> 
> volatile almost never does what you think it does. :)

True.

> In this case, I think only one thread reads/writes the variable so
> "volatile" is unnecessary.

Lets just check that; so it's set by 'migrate_fd_connect' (from the main thread)
when it spawns the thread, and it's cleared by migrate_fd_cleanup that's always 
run
as a bh, so should always be in the main thread; so yes - always the same 
thread,
that's nice and simple; volatile evaporated.

> Otherwise, you would need to add actual memory barriers, atomic
> operations, or synchronization primitives.
> 
> For start_postcopy, it is okay because it is just a hint to the compiler
> and the processor will eventually see the assignment.

Yes, in this case my understanding is that it's necessary to stop the
compiler potentially moving the check outside the loop.

> For this case
> QEMU has atomic_read/atomic_set (corresponding to __ATOMIC_RELAXED in
> C/C++1x), so you could use those as well.

Ah, so those look like they just volatile cast anyway.

(I've probably got some other flags I need to think about reading/writing
atomically/safely).

Dave
(I'll take the other issues in this mail separately since there are quite a 
few).
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK