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

[Peter Xu]

On Tue, Dec 24, 2024 at 10:24:51AM +0300, Pavel Dovgalyuk wrote:
> On 23.12.2024 20:26, Peter Xu wrote:
> > 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?
> 
> CLOCK_HOST is used for rtc by default. As the rtc affects the guest state,
> therefore CLOCK_HOST affects guest state too.

It's not obvious to me that HOST should only be used for rtc, and it's part
of guest state.  If that's a must, I'd still suggest we add that into doc.
But then it means we lose one way to fetch host wallclock in the time API;
I still see some other users use it, I'm guessing in the way to fetch host
wall clock.

> 
> Migration is not related to guest state change, therefore it should either
> use realtime clock, or set some flag to make host clock reads not tracked by
> record/replay.

In migration's case, realtime clock suites more.  But maybe we still need
another clock indeed just to fetch host wall clock without any lock
implications.  So maybe the better way is making the tracked one to be
CLOCK_GUEST_RTC, put rich documentatation to avoid abuse, then keep HOST
the simple definition.

Thanks,

-- 
Peter Xu