* Wei Yang (richardw.y...@linux.intel.com) wrote: > On Wed, Oct 09, 2019 at 01:36:34PM +0800, Peter Xu wrote: > >On Wed, Oct 09, 2019 at 01:07:56PM +0800, Wei Yang wrote: > >> On Wed, Oct 09, 2019 at 12:12:25PM +0800, Peter Xu wrote: > >> >On Wed, Oct 09, 2019 at 09:02:04AM +0800, Wei Yang wrote: > >> >> On Tue, Oct 08, 2019 at 05:40:46PM +0100, Dr. David Alan Gilbert wrote: > >> >> >* Wei Yang (richardw.y...@linux.intel.com) wrote: > >> >> >> Currently, we set PostcopyState blindly to RUNNING, even we found the > >> >> >> previous state is not LISTENING. This will lead to a corner case. > >> >> >> > >> >> >> First let's look at the code flow: > >> >> >> > >> >> >> qemu_loadvm_state_main() > >> >> >> ret = loadvm_process_command() > >> >> >> loadvm_postcopy_handle_run() > >> >> >> return -1; > >> >> >> if (ret < 0) { > >> >> >> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING) > >> >> >> ... > >> >> >> } > >> >> >> > >> >> >> From above snippet, the corner case is loadvm_postcopy_handle_run() > >> >> >> always sets state to RUNNING. And then it checks the previous state. > >> >> >> If > >> >> >> the previous state is not LISTENING, it will return -1. But at this > >> >> >> moment, PostcopyState is already been set to RUNNING. > >> >> >> > >> >> >> Then ret is checked in qemu_loadvm_state_main(), when it is -1 > >> >> >> PostcopyState is checked. Current logic would pause postcopy and > >> >> >> retry > >> >> >> if PostcopyState is RUNNING. This is not what we expect, because > >> >> >> postcopy is not active yet. > >> >> >> > >> >> >> This patch makes sure state is set to RUNNING only previous state is > >> >> >> LISTENING by introducing an old_state parameter in > >> >> >> postcopy_state_set(). > >> >> >> New state only would be set when current state equals to old_state. > >> >> >> > >> >> >> Signed-off-by: Wei Yang <richardw.y...@linux.intel.com> > >> >> > > >> >> >OK, it's a shame to use a pointer there, but it works. > >> >> > >> >> You mean second parameter of postcopy_state_set()? > >> >> > >> >> I don't have a better idea. Or we introduce a new state > >> >> POSTCOPY_INCOMING_NOCHECK. Do you feel better with this? > >> > > >> >Maybe simply fix loadvm_postcopy_handle_run() to set the state after > >> >the POSTCOPY_INCOMING_LISTENING check? > >> > > >> > >> Set state back to ps if ps is not POSTCOPY_INCOMING_LISTENING? > >> > >> Sounds like another option. > > > >Even simpler? > > > > ps = postcopy_state_get(); > > if (ps != INCOMING)
^^ LISTENING > > return -1; > > postcopy_state_set(RUNNING); > > > > Looks good to me. > > Dave, > > Do you feel good with it? Yes, I think so; it's simpler. Dave > >Thanks, > > > >-- > >Peter Xu > > -- > Wei Yang > Help you, Help me -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK