Re: [PATCH 02/17] replay: Fix migration replay_mutex locking

[Nicholas Piggin]

On Fri Dec 20, 2024 at 11:08 PM AEST, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> Hi Nick,
>
> I'm ignorant about replay, but we try to know why were taking the BQL in
> the migration code, we move it around sometimes, etc. Can we be a bit
> more strict with documentation here so we don't get stuck with a lock
> that can't be changed?
>
> > Migration causes a number of events that need to go in the replay
> > trace, such as vm state transitions. The replay_mutex lock needs to
> > be held for these.
> >
>
> Is it practical to explicitly list which events are those?

As a general rule it is something like "while the target can be
producing or consuming rr events".

There is some record-replay handling in snapshot code (flush
events, get icount, etc) as well as SHUTDOWN_CAUSE_SNAPSHOT_LOAD
event generated and possibly a few other things. So for migration
it's not just a side effect of calling other APIs, but it is
explicitly "replay-aware", at least in some part.

I actually don't know full details about how snapshot/migrate
and record-replay work together. I know reverse debugging can
use snapshots to load the most recent possible state to
minimize replay, but that is "external" to the machine itself.
But I don't know why you would want to record and replay snapshot
loading as part of the trace. But facility exists. Pavel
understands all the big picture much better.

> Are there any tests that exercise this that we could use to validate
> changes around this area?

Yes I added some more avocado testing which includes snapshotting
while recording, and that's where I hit these bugs. I do plan to
submit that up as soon as this series gets in, just trying to keep
things managable. In that case we could defer this patch from this
series (the replay_linux test does not do any snapshotting as yet).

I think once you have some regression tests, you probably won't
have to worry _too_ much about record/replay details in migraiton.

>
> > The simplest approach seems to be just take it up-front when taking
> > the bql.
>
> But also the thing asserts if taken inside the BQL, so is the actual
> matter here that we _cannot_ take the lock around the proper places?

Yes, that is part of it in this case. Some other code drops the
bql then retakes both... but that is more complex and requires
knowledge of calling bql context to be sure it is safe to drop.

> I also see the replay lock around the main loop, so is it basically bql2
> from the perspective of most of QEMU?

Unfortunately it is a big scope yes. Basically it needs to
maintain atomicity between the event log entry that we record or
replay (run N instructions; raise an interrupt; get a character
from keyboard; etc.,) and the actual running of that operation
in the machine.

Thanks,
Nick