[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_t
Dr. David Alan Gilbert
Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event
Fri, 25 Aug 2017 16:51:29 +0100
* Peter Maydell (address@hidden) wrote:
> On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
> <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > migration_incoming_state_destroy doesn't really destroy, it cleans up.
> > After a loadvm it's called, but the loadvm command can be run twice,
> > and so destroying an init-once mutex breaks on the second loadvm.
> > Reported-by: Stafford Horne <address@hidden>
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> > migration/migration.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/migration/migration.c b/migration/migration.c
> > index c3fe0ed9ca..a625551ce5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void)
> > mis->from_src_file = NULL;
> > }
> > - qemu_event_destroy(&mis->main_thread_load_event);
> > + qemu_event_reset(&mis->main_thread_load_event);
> > }
> Is it worth doing more here?
In the longer-term, yes; it seemed right just to get
this bug fixed first though.
> For instance:
> * rename the function to something that better reflects
> what it's doing
> * make sure it actually goes back to the state it's in
> after you first call migration_incoming_get_current()
> (eg setting mis_current.state, zeroing various fields)
> * maybe instead of a "get current state" that does a
> reset if it's not been called before and a "destroy"
> that does cleanup stuff (like telling the source we've
> stopped) and also resetting back to clean state, we
> could structure this with a function that does
> "give me a clean completely reset state" which you
> must call first, then use get_current() purely to
> get the current state with no 'reset on first call'
> semantics, and finally a "complete" function that just
> does the "tell source we've stopped" and close
> resources we no longer need ?
Yes; an init/current/clean does make sense; one of my comments
on one of Peter's patchsets was to point out the
migration_incoming_get_current isn't thread safe if you're
not sure you've already called it, so it could do with fixing.
There has also been a few things that have wanted to gather stats
that are available after the end of an incoming migration;
so we don't really want to destroy that state, we just want
to get rid of anything temporary.
You could argue that this thread_load_event is best init'd
at the start of the incoming migration and then destroyed at the
> PS, in migration_incoming_get_current() we do
> mis_current.state = MIGRATION_STATUS_NONE;
> memset(&mis_current, 0, sizeof(MigrationIncomingState));
> and the first line there is pointless because the memset
> blasts zeroes over it anyway.
> -- PMM
Dr. David Alan Gilbert / address@hidden / Manchester, UK