Re: [PATCH 01/17] replay: Fix migration use of clock for statistics

[Peter Xu <peterx@redhat.com>]

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,

-- 
Peter Xu