On Sat, Dec 21, 2024 at 01:02:01PM +1000, Nicholas Piggin wrote:
On Sat Dec 21, 2024 at 2:31 AM AEST, Peter Xu wrote:
On Fri, Dec 20, 2024 at 08:42:03PM +1000, Nicholas Piggin wrote:
Migration reads CLOCK_HOST when not holding the replay_mutex, which
asserts when recording a trace. These are not guest visible so should
be CLOCK_REALTIME like other statistics in MigrationState, which do
not require the replay_mutex.
Irrelevant of the change, should we document such lock implications in
timer.h?
I guess the intention was to try to avoid caller caring too much
about replay internals, so I'm not sure if that will help or
hinder understanding :(
CLOCK_HOST should be the wall clock in QEMU, IIUC. If any QEMU caller
tries to read host wall clock requires some mutex to be held.. then I don't
see how we can avoid mentioning it. It's indeed weird if we need to take a
feature specific mutex just to read the wallclock.. But maybe I misread the
context somewhere..
I think the big rule is something like "if it affects guest state,
then you must use HOST or VIRTUAL*, if it does not affect guest state
HOST clock logically shouldn't be relevant to guest-state?
then you must use REALTIME". record-replay code then takes care of
replay mutex locking.
Does get a little fuzzy around edges in code that is somewhat
aware of record-replay though, like migration/snapshots.
Said that, I agree with the change itself - any measurement may not want to
involve NTP at all... which HOST / gtod will, but REALTIME won't. However
this patch doesn't seem to be for that purpose.. So I'd like to double
check.
Thanks,