| Message ID | 20160608051404.1688.65453.stgit@PASHA-ISP (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru> > To: qemu-devel@nongnu.org > Cc: pbonzini@redhat.com, jasowang@redhat.com, agraf@suse.de, david@gibson.dropbear.id.au > Sent: Wednesday, June 8, 2016 7:14:04 AM > Subject: [PATCH 2/3] replay: allow replay stopping and restarting > > This patch fixes bug with stopping and restarting replay > through monitor. > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > block/blkreplay.c | 18 +++++++++++++----- > cpus.c | 1 + > include/sysemu/replay.h | 2 ++ > replay/replay-internal.h | 2 -- > vl.c | 1 + > 5 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/block/blkreplay.c b/block/blkreplay.c > index 42f1813..438170c 100644 > --- a/block/blkreplay.c > +++ b/block/blkreplay.c > @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque) > g_free(req); > } > > +static uint64_t blkreplay_next_id(void) > +{ > + if (replay_events_enabled()) { > + return request_id++; > + } > + return 0; > +} What happens if 0 is returned? I think that you want to call replay_disable_events... > bdrv_drain_all(); ... after this bdrv_drain_all. I was going to suggest using qemu_add_vm_change_state_handler in replay_start (which could have replaced the existing call to replay_enable_events), but that's not possible if you have to do your calls after bdrv_drain_all. Thanks, Paolo
> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru> > > This patch fixes bug with stopping and restarting replay > > through monitor. > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > block/blkreplay.c | 18 +++++++++++++----- > > cpus.c | 1 + > > include/sysemu/replay.h | 2 ++ > > replay/replay-internal.h | 2 -- > > vl.c | 1 + > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/block/blkreplay.c b/block/blkreplay.c > > index 42f1813..438170c 100644 > > --- a/block/blkreplay.c > > +++ b/block/blkreplay.c > > @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque) > > g_free(req); > > } > > > > +static uint64_t blkreplay_next_id(void) > > +{ > > + if (replay_events_enabled()) { > > + return request_id++; > > + } > > + return 0; > > +} > > What happens if 0 is returned? It could be any value. When replay events are disables, it means that either replay is disabled or execution is stopped. In first case we won't pass this requests through the replay queue and therefore id is useless. In stopped mode we have to keep request_id unchanged to make record/replay deterministic. > I think that you want to call > replay_disable_events... > > > bdrv_drain_all(); > > ... after this bdrv_drain_all. Why? We disable replay events to avoid adding new block requests to the queue. How this is related to drain all? > > I was going to suggest using qemu_add_vm_change_state_handler > in replay_start (which could have replaced the existing call > to replay_enable_events), but that's not possible if you have > to do your calls after bdrv_drain_all. Pavel Dovgalyuk
Ping? Pavel Dovgalyuk > -----Original Message----- > From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru] > Sent: Monday, June 20, 2016 9:27 AM > To: 'Paolo Bonzini'; 'Pavel Dovgalyuk' > Cc: qemu-devel@nongnu.org; jasowang@redhat.com; agraf@suse.de; david@gibson.dropbear.id.au > Subject: RE: [PATCH 2/3] replay: allow replay stopping and restarting > > > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > > From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru> > > > This patch fixes bug with stopping and restarting replay > > > through monitor. > > > > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > > --- > > > block/blkreplay.c | 18 +++++++++++++----- > > > cpus.c | 1 + > > > include/sysemu/replay.h | 2 ++ > > > replay/replay-internal.h | 2 -- > > > vl.c | 1 + > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/block/blkreplay.c b/block/blkreplay.c > > > index 42f1813..438170c 100644 > > > --- a/block/blkreplay.c > > > +++ b/block/blkreplay.c > > > @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque) > > > g_free(req); > > > } > > > > > > +static uint64_t blkreplay_next_id(void) > > > +{ > > > + if (replay_events_enabled()) { > > > + return request_id++; > > > + } > > > + return 0; > > > +} > > > > What happens if 0 is returned? > > It could be any value. When replay events are disables, > it means that either replay is disabled or execution is stopped. > In first case we won't pass this requests through the replay queue > and therefore id is useless. > In stopped mode we have to keep request_id unchanged to make > record/replay deterministic. > > > I think that you want to call > > replay_disable_events... > > > > > bdrv_drain_all(); > > > > ... after this bdrv_drain_all. > > Why? We disable replay events to avoid adding new block requests > to the queue. How this is related to drain all? > > > > > I was going to suggest using qemu_add_vm_change_state_handler > > in replay_start (which could have replaced the existing call > > to replay_enable_events), but that's not possible if you have > > to do your calls after bdrv_drain_all. > > Pavel Dovgalyuk
On 20/06/2016 08:26, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>> From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru> >>> This patch fixes bug with stopping and restarting replay >>> through monitor. >>> >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> >>> --- >>> block/blkreplay.c | 18 +++++++++++++----- >>> cpus.c | 1 + >>> include/sysemu/replay.h | 2 ++ >>> replay/replay-internal.h | 2 -- >>> vl.c | 1 + >>> 5 files changed, 17 insertions(+), 7 deletions(-) >>> >>> diff --git a/block/blkreplay.c b/block/blkreplay.c >>> index 42f1813..438170c 100644 >>> --- a/block/blkreplay.c >>> +++ b/block/blkreplay.c >>> @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque) >>> g_free(req); >>> } >>> >>> +static uint64_t blkreplay_next_id(void) >>> +{ >>> + if (replay_events_enabled()) { >>> + return request_id++; >>> + } >>> + return 0; >>> +} >> >> What happens if 0 is returned? > > It could be any value. When replay events are disables, > it means that either replay is disabled or execution is stopped. > In first case we won't pass this requests through the replay queue > and therefore id is useless. > In stopped mode we have to keep request_id unchanged to make > record/replay deterministic. > >> I think that you want to call >> replay_disable_events... >> >>> bdrv_drain_all(); >> >> ... after this bdrv_drain_all. > > Why? We disable replay events to avoid adding new block requests > to the queue. How this is related to drain all? drain all completes the guest's pending requests. If you disable events before drain all, doesn't that cause a mismatch between record and replay? >> >> I was going to suggest using qemu_add_vm_change_state_handler >> in replay_start (which could have replaced the existing call >> to replay_enable_events), but that's not possible if you have >> to do your calls after bdrv_drain_all. > > Pavel Dovgalyuk > > >
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > On 20/06/2016 08:26, Pavel Dovgalyuk wrote: > >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] > >>> From: "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru> > >>> This patch fixes bug with stopping and restarting replay > >>> through monitor. > >>> > >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > >>> --- > > > >> I think that you want to call > >> replay_disable_events... > >> > >>> bdrv_drain_all(); > >> > >> ... after this bdrv_drain_all. > > > > Why? We disable replay events to avoid adding new block requests > > to the queue. How this is related to drain all? > > drain all completes the guest's pending requests. If you disable events > before drain all, doesn't that cause a mismatch between record and replay? Looks reasonable, thanks. I'll update the patch. What about replay patch for networking? Pavel Dovgalyuk
diff --git a/block/blkreplay.c b/block/blkreplay.c index 42f1813..438170c 100644 --- a/block/blkreplay.c +++ b/block/blkreplay.c @@ -70,6 +70,14 @@ static void blkreplay_bh_cb(void *opaque) g_free(req); } +static uint64_t blkreplay_next_id(void) +{ + if (replay_events_enabled()) { + return request_id++; + } + return 0; +} + static void block_request_create(uint64_t reqid, BlockDriverState *bs, Coroutine *co) { @@ -84,7 +92,7 @@ static void block_request_create(uint64_t reqid, BlockDriverState *bs, static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov) { - uint64_t reqid = request_id++; + uint64_t reqid = blkreplay_next_id(); int ret = bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov); block_request_create(reqid, bs, qemu_coroutine_self()); qemu_coroutine_yield(); @@ -95,7 +103,7 @@ static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs, static int coroutine_fn blkreplay_co_writev(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov) { - uint64_t reqid = request_id++; + uint64_t reqid = blkreplay_next_id(); int ret = bdrv_co_writev(bs->file->bs, sector_num, nb_sectors, qiov); block_request_create(reqid, bs, qemu_coroutine_self()); qemu_coroutine_yield(); @@ -106,7 +114,7 @@ static int coroutine_fn blkreplay_co_writev(BlockDriverState *bs, static int coroutine_fn blkreplay_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, int nb_sectors, BdrvRequestFlags flags) { - uint64_t reqid = request_id++; + uint64_t reqid = blkreplay_next_id(); int ret = bdrv_co_write_zeroes(bs->file->bs, sector_num, nb_sectors, flags); block_request_create(reqid, bs, qemu_coroutine_self()); qemu_coroutine_yield(); @@ -117,7 +125,7 @@ static int coroutine_fn blkreplay_co_write_zeroes(BlockDriverState *bs, static int coroutine_fn blkreplay_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors) { - uint64_t reqid = request_id++; + uint64_t reqid = blkreplay_next_id(); int ret = bdrv_co_discard(bs->file->bs, sector_num, nb_sectors); block_request_create(reqid, bs, qemu_coroutine_self()); qemu_coroutine_yield(); @@ -127,7 +135,7 @@ static int coroutine_fn blkreplay_co_discard(BlockDriverState *bs, static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs) { - uint64_t reqid = request_id++; + uint64_t reqid = blkreplay_next_id(); int ret = bdrv_co_flush(bs->file->bs); block_request_create(reqid, bs, qemu_coroutine_self()); qemu_coroutine_yield(); diff --git a/cpus.c b/cpus.c index 326742f..34f951f 100644 --- a/cpus.c +++ b/cpus.c @@ -742,6 +742,7 @@ static int do_vm_stop(RunState state) runstate_set(state); vm_state_notify(0, state); qapi_event_send_stop(&error_abort); + replay_disable_events(); } bdrv_drain_all(); diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h index 0a88393..52430d3 100644 --- a/include/sysemu/replay.h +++ b/include/sysemu/replay.h @@ -105,6 +105,8 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint); /*! Disables storing events in the queue */ void replay_disable_events(void); +/*! Enables storing events in the queue */ +void replay_enable_events(void); /*! Returns true when saving events is enabled */ bool replay_events_enabled(void); /*! Adds bottom half event to the queue */ diff --git a/replay/replay-internal.h b/replay/replay-internal.h index efbf14c..310c4b7 100644 --- a/replay/replay-internal.h +++ b/replay/replay-internal.h @@ -119,8 +119,6 @@ void replay_read_next_clock(unsigned int kind); void replay_init_events(void); /*! Clears internal data structures for events handling */ void replay_finish_events(void); -/*! Enables storing events in the queue */ -void replay_enable_events(void); /*! Flushes events queue */ void replay_flush_events(void); /*! Clears events list before loading new VM state */ diff --git a/vl.c b/vl.c index 2f74fe8..b361ca8 100644 --- a/vl.c +++ b/vl.c @@ -765,6 +765,7 @@ void vm_start(void) if (runstate_is_running()) { qapi_event_send_stop(&error_abort); } else { + replay_enable_events(); cpu_enable_ticks(); runstate_set(RUN_STATE_RUNNING); vm_state_notify(1, RUN_STATE_RUNNING);
This patch fixes bug with stopping and restarting replay through monitor. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- block/blkreplay.c | 18 +++++++++++++----- cpus.c | 1 + include/sysemu/replay.h | 2 ++ replay/replay-internal.h | 2 -- vl.c | 1 + 5 files changed, 17 insertions(+), 7 deletions(-)