| Message ID | 20240311174026.2177152-12-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | replay: fixes and new test cases | expand |
This won't work, as needed. Announce timer can't be enabled, because it is set in post_load function. Therefore announce callbacks break the replay, when virtio-net is used with snapshots. On 11.03.2024 20:40, Nicholas Piggin wrote: > Using virtual time for announce ensures that guest visible effects > are deterministic and don't break replay. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > net/announce.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/announce.c b/net/announce.c > index 9e99044422..70b5d5e822 100644 > --- a/net/announce.c > +++ b/net/announce.c > @@ -187,7 +187,7 @@ static void qemu_announce_self_once(void *opaque) > > void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params) > { > - qemu_announce_timer_reset(timer, params, QEMU_CLOCK_REALTIME, > + qemu_announce_timer_reset(timer, params, QEMU_CLOCK_VIRTUAL, > qemu_announce_self_once, timer); > if (params->rounds) { > qemu_announce_self_once(timer);
On Tue Mar 12, 2024 at 7:09 PM AEST, Pavel Dovgalyuk wrote: > This won't work, as needed. Announce timer can't be enabled, because > it is set in post_load function. Therefore announce callbacks break > the replay, when virtio-net is used with snapshots. I see. Is that somehow marked as being incompatible with rr? Thanks, Nick > > On 11.03.2024 20:40, Nicholas Piggin wrote: > > Using virtual time for announce ensures that guest visible effects > > are deterministic and don't break replay. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > net/announce.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/announce.c b/net/announce.c > > index 9e99044422..70b5d5e822 100644 > > --- a/net/announce.c > > +++ b/net/announce.c > > @@ -187,7 +187,7 @@ static void qemu_announce_self_once(void *opaque) > > > > void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params) > > { > > - qemu_announce_timer_reset(timer, params, QEMU_CLOCK_REALTIME, > > + qemu_announce_timer_reset(timer, params, QEMU_CLOCK_VIRTUAL, > > qemu_announce_self_once, timer); > > if (params->rounds) { > > qemu_announce_self_once(timer);
On 12.03.2024 14:05, Nicholas Piggin wrote: > On Tue Mar 12, 2024 at 7:09 PM AEST, Pavel Dovgalyuk wrote: >> This won't work, as needed. Announce timer can't be enabled, because >> it is set in post_load function. Therefore announce callbacks break >> the replay, when virtio-net is used with snapshots. > > I see. Is that somehow marked as being incompatible with rr? Here's the prior discussion on it: https://lore.kernel.org/qemu-devel/8735ovx0zd.fsf@linaro.org/t/ > > Thanks, > Nick > >> >> On 11.03.2024 20:40, Nicholas Piggin wrote: >>> Using virtual time for announce ensures that guest visible effects >>> are deterministic and don't break replay. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> net/announce.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/announce.c b/net/announce.c >>> index 9e99044422..70b5d5e822 100644 >>> --- a/net/announce.c >>> +++ b/net/announce.c >>> @@ -187,7 +187,7 @@ static void qemu_announce_self_once(void *opaque) >>> >>> void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params) >>> { >>> - qemu_announce_timer_reset(timer, params, QEMU_CLOCK_REALTIME, >>> + qemu_announce_timer_reset(timer, params, QEMU_CLOCK_VIRTUAL, >>> qemu_announce_self_once, timer); >>> if (params->rounds) { >>> qemu_announce_self_once(timer); >
On Tue Mar 12, 2024 at 9:12 PM AEST, Pavel Dovgalyuk wrote: > On 12.03.2024 14:05, Nicholas Piggin wrote: > > On Tue Mar 12, 2024 at 7:09 PM AEST, Pavel Dovgalyuk wrote: > >> This won't work, as needed. Announce timer can't be enabled, because > >> it is set in post_load function. Therefore announce callbacks break > >> the replay, when virtio-net is used with snapshots. > > > > I see. Is that somehow marked as being incompatible with rr? > > Here's the prior discussion on it: > https://lore.kernel.org/qemu-devel/8735ovx0zd.fsf@linaro.org/t/ Thanks for that context. You already fixed some of these, lol that would have saved me a few hours. Maybe clearing the announce flag is the right way to solve this, I might take that instead. Trouble is we have virtio-net selftests in tests already and this causes failures. So we need to get it fixed. I do think the bh API is a bit clunky and could be revised into a name that makes more sense for the non-replay developer, but that's really a separate issue and should not prevent this bug fixes being merged. We really need non-flaky rr testing upstream because it's quite easy to break. So I will keep trying to get these merged. Thanks, Nick > > > > > > Thanks, > > Nick > > > >> > >> On 11.03.2024 20:40, Nicholas Piggin wrote: > >>> Using virtual time for announce ensures that guest visible effects > >>> are deterministic and don't break replay. > >>> > >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >>> --- > >>> net/announce.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/net/announce.c b/net/announce.c > >>> index 9e99044422..70b5d5e822 100644 > >>> --- a/net/announce.c > >>> +++ b/net/announce.c > >>> @@ -187,7 +187,7 @@ static void qemu_announce_self_once(void *opaque) > >>> > >>> void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params) > >>> { > >>> - qemu_announce_timer_reset(timer, params, QEMU_CLOCK_REALTIME, > >>> + qemu_announce_timer_reset(timer, params, QEMU_CLOCK_VIRTUAL, > >>> qemu_announce_self_once, timer); > >>> if (params->rounds) { > >>> qemu_announce_self_once(timer); > >
On Tue Mar 12, 2024 at 9:12 PM AEST, Pavel Dovgalyuk wrote: > On 12.03.2024 14:05, Nicholas Piggin wrote: > > On Tue Mar 12, 2024 at 7:09 PM AEST, Pavel Dovgalyuk wrote: > >> This won't work, as needed. Announce timer can't be enabled, because > >> it is set in post_load function. Therefore announce callbacks break > >> the replay, when virtio-net is used with snapshots. > > > > I see. Is that somehow marked as being incompatible with rr? > > Here's the prior discussion on it: > https://lore.kernel.org/qemu-devel/8735ovx0zd.fsf@linaro.org/t/ Actually I don't know if it's so simple. If VIRTIO_NET_F_GUEST_ANNOUNCE is clear then AFAIKS it sends a RARP packet instead. Also the timer can be triggered for other reasons than migration. Not quite sure how that all fits together. I guess record/replay would just have to disable it entirely. We could support it if we had a ANNOUNCE event I guess. Thanks, Nick
diff --git a/net/announce.c b/net/announce.c index 9e99044422..70b5d5e822 100644 --- a/net/announce.c +++ b/net/announce.c @@ -187,7 +187,7 @@ static void qemu_announce_self_once(void *opaque) void qemu_announce_self(AnnounceTimer *timer, AnnounceParameters *params) { - qemu_announce_timer_reset(timer, params, QEMU_CLOCK_REALTIME, + qemu_announce_timer_reset(timer, params, QEMU_CLOCK_VIRTUAL, qemu_announce_self_once, timer); if (params->rounds) { qemu_announce_self_once(timer);
Using virtual time for announce ensures that guest visible effects are deterministic and don't break replay. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- net/announce.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)