Re: vtime accounting

[Christoffer Dall <cdall@linaro.org>]

Hi Radim,

[Stephen, I am cc'ing you because we are managing to confuse ourselves
here and some of my ramblings are basically misrepresented bits of
information I got from you].

On Wed, Mar 15, 2017 at 04:57:24PM +0100, Radim Krčmář wrote:
> 2017-03-15 09:43+0100, Christoffer Dall:
> > On Tue, Mar 14, 2017 at 09:27:22PM +0100, Radim Krčmář wrote:
> >> 2017-03-14 19:39+0100, Christoffer Dall:
> >> > On Tue, Mar 14, 2017 at 05:58:59PM +0100, Radim Krčmář wrote:
> >> >> 2017-03-14 09:26+0100, Christoffer Dall:
> >> >> > On Mon, Mar 13, 2017 at 06:28:16PM +0100, Radim Krčmář wrote:
> >> >> >> 2017-03-08 02:57-0800, Christoffer Dall:
> >> >> >> > Hi Paolo,
> >> >> >> > 
> >> >> >> > I'm looking at improving KVM/ARM a bit by calling guest_exit_irqoff
> >> >> >> > before enabling interrupts when coming back from the guest.
> >> >> >> > 
> >> >> >> > Unfortunately, this appears to mess up my view of CPU usage using
> >> >> >> > something like htop on the host, because it appears all time is spent
> >> >> >> > inside the kernel.
> >> >> >> > 
> >> >> >> > From my analysis, I think this is because we never handle any interrupts
> >> >> >> > before enabling interrupts, where the x86 code does its
> >> >> >> > handle_external_intr, and the result on ARM is that we never increment
> >> >> >> > jiffies before doing the vtime accounting.
> >> >> >> 
> >> >> >> (Hm, the counting might be broken on nohz_full then.)
> >> >> >> 
> >> >> > 
> >> >> > Don't you still have a scheduler tick even with nohz_full and something
> >> >> > that will eventually update jiffies then?
> >> >> 
> >> >> Probably, I don't understand jiffies accounting too well and didn't see
> >> >> anything that would bump the jiffies in or before guest_exit_irqoff().
> >> > 
> >> > As far as I understand, from my very very short time of looking at the
> >> > timer code, jiffies are updated on every tick, which can be cause by a
> >> > number of events, including *any* interrupt handler (coming from idle
> >> > state), soft timers, timer interrupts, and possibly other things.
> >> 
> >> Yes, I was thinking that entering/exiting user mode should trigger it as
> >> well, in order to correctly account for time spent there, but couldn't
> >> find it ...
> > 
> > Isn't it based on taking the timer interrupt when running in userspace
> > as opposed to when running in the kernel?
> 
> The timer interrupt usually accounts it, but there is no timer interrupt
> when tickless, so there has to be accounting somewhere on the border ...

I think there are; only it's not a regular interval but only when a
timing is needed, such as the time slice expiring as decided by the
scheduler.

> 
> __context_tracking_enter/exit(CONTEXT_USER) calls
> vtime_user_enter/exit() to do accounting in that case, but expects that
> jiffies were updated by someone else.
> 

I'm assuming user/kernel accounting works today with tickless, so surely
this is handled somewhere.  There are a bunch of calling paths to
tick_do_update_jiffies64() in kernel/time/tick-sched.c related to nohz,
so my best guess is that it's updated at the sched_clock granularity and
that is sufficient for profiling, because you basically care about where
a particualr time slice was spent, I suppose.

> >> The case I was wondering about is if the kernel spent e.g. 10 jiffies in
> >> guest mode and then exited on mmio -- no interrupt in the host, and
> >> guest_exit_irqoff() would flip accouting would over system time.
> >> Can those 10 jiffies get accounted to system (not guest) time?
> > 
> > Yes, I think the key is whether you end up taking a timer interrupt
> > before or after switchign PF_VCPU.  So you can spend X jiffies in the
> > guest, come back, change PF_VCPU (timer still hasen't expired), and then
> > the timer expires immediately afterwards, and the whole block of jiffies
> > that are incremented as a result of the timer gets accounted as kernel
> > time.
> 
> Wouldn't that be result of a bug?
> 

I don't think anyone cares about this level of granularity really, the
case I mentioned above is possible, albeit unlikely.

> (The timer period is 1 jiffy, so the VM would get kicked before getting
>  to 2 jiffies in guest mode and spending so much time in bettwen seems
>  like a bug.
>  10 jiffies could be spend in a guest while under nohz_full, but there is
>  still at least one CPU with HZ timer that updates the global jiffies, so
>  I assume that guest_exit() should see at least 9 jiffies.)
> 
> > (Note that jiffies on clocksourced architectures is an arbitrary number,
> > which somehow scales according to the clocksource frequency, depending
> > on the CONFIG_HZ, so a single timer interrupt can increment jiffies by
> > more than 1.
> > 
> >> Accounting 1 jiffy wrong is normal as we expect that the distribution of
> >> guest/kernel time in the jiffy is going to be approximated over a longer
> >> sampling period, but if we could account multiple jiffies wrong, this
> >> expectation is very hard to defend.
> >> 
> > 
> > I think it's a best effort, and we expect inaccuracies over much more
> > than a single jiffy, see above.
> > 
> >> >> >> > So my current idea is to increment jiffies according to the clocksource
> >> >> >> > before calling guest_exit_irqoff, but this would require some main
> >> >> >> > clocksource infrastructure changes.
> >> >> >> 
> >> >> >> This seems similar to calling the function from the timer interrupt.
> >> >> >> The timer interrupt would be delivered after that and only wasted time,
> >> >> >> so it might actually be slower than just delivering it before ...
> >> >> > 
> >> >> > That's assuming that the timer interrupt hits at every exit.  I don't
> >> >> > think that's the case, but I should measure it.
> >> >> 
> >> >> There cannot be less vm exits and I think there are far more vm exits,
> >> >> but if there was no interrupt, then jiffies shouldn't raise and we would
> >> >> get the same result as with plain guest_exit_irqoff().
> >> >> 
> >> > 
> >> > That's true if you're guaranteed to take the timer interrupts that
> >> > happen while running the guest before hitting guest_exit_irqoff(), so
> >> > that you eventually count *some* time for the guest.  In the arm64 case,
> >> > if we just do guest_exit_irqoff(), we *never* account any time to the
> >> > guest.
> >> 
> >> Yes.  I assumed that if jiffies should be incremented, then you also get
> >> a timer tick,
> > 
> > I think jiffies can be incremented in a number of cases, and as I
> > understand it, when the kernel folks talk about "the tick" that doesn't
> > necessarily mean that your scheduler timer fires an interrupt, but I
> > could be wrong.
> 
> Good point.  do_timer() (the only place to update jiffies, I hope) is

as I understand it, the whole jiffies thing is circular, so the update
can happen in other paths than do_timer.

> called from:
> 
>   tick_periodic()
>   xtime_update()
>   tick_do_update_jiffies64()
> 
> The first two seem to coincide with a specific timer interrupt.  The
> last one is called from:
> 
>   tick_sched_do_timer() [timer interrupt]
>   tick_nohz_update_jiffies() [any interrupt]
>   do_idle() [not interrupt, but also never hit after VM exit]
> 
> Looks like jiffies are mostly timer interrupt based, but if you have
> nohz, then there are also other interrupts and switches from idle.
> 

yes exactly, clearly, non of us are timekeeping experts...

> >> so checking whether jiffies should be incremented inside
> >> guest_exit_irqoff() was only slowing down the common case, where jiffies
> >> remained the same.
> > 
> > My original suggestion was to not use jiffies at all, but use ktime, on
> > architectures that don't do handle_external_intr() type things, which
> > would result in a clocksource read on every exit, which on ARM I think
> > is faster than our enable/disable interrupt dance we do at the moment.
> 
> I didn't catch that, sorry.
> 
> > But of course, I'd have to measure that.
> 
> The accounting would need to change to ktime even for userspace switches

good point.

> to avoid double-accounting time and I think that ktime was abolished
> because it was too slow ... definitely a lot of benchmarking.
> 
> >> (Checking if jiffies should be incremented is quite slow by itself.)
> >> 
> > 
> > Really?  I think that depends on the architecture.  Reading the counter
> > on ARM is supposed to be cheap, I think.
> 
> True, and also depends on what slow means. :)
> 
> IIRC, it takes around 50 cycles to read TSC-based ktime on x86, which
> would be rougly 2% of a current VM exit/entry loop.
> 
> >> >> > I assume there's a good reason why we call guest_enter() and
> >> >> > guest_exit() in the hot path on every KVM architecture?
> >> >> 
> >> >> I consider myself biased when it comes to jiffies, so no judgement. :)
> >> >> 
> >> >> From what I see, the mode switch is used only for statistics.
> >> >> The original series is
> >> >> 
> >> >> 5e84cfde51cf303d368fcb48f22059f37b3872de~1..d172fcd3ae1ca7ac27ec8904242fd61e0e11d332
> >> >> 
> >> >> It didn't introduce the overhead with interrupt window and it didn't
> >> >> count host kernel irq time as user time, so it was better at that time.
> >> > 
> >> > Yes, but it was based on cputime_to... functions, which I understand use
> >> > ktime, which on systems running KVM will most often read the clocksource
> >> > directly from the hardware, and that was then optimized later to just
> >> > use jiffies to avoid the clocksource read because jiffies is already in
> >> > memory and adjusted to the granularity we need, so in some sense an
> >> > improvement, only it doesn't work if you don't update jiffies when
> >> > needed.
> >> 
> >> True.  The kvm_guest_enter/exit still didn't trigger accounting, but the
> >> granularity was better if there were other sources of accounting than
> >> just timer ticks.
> > 
> > I think the granularity was better, yes, but I wonder if that improved
> > accuracy was ever visible to the user, given that the CPU time may be
> > counted in jiffies?
> 
> I think the time for statistics used to be in cycles before it switched
> to nanoseconds, so user could have seen it some difference.
> 
> >> And I noticed another funny feature: the original intention was that if
> >> the guest uses hardware acceleration, then the whole timeslice gets
> >> accounted to guest/user time, because kvm_guest_exit() was not supposed
> >> to clear PF_VCPU: https://lkml.org/lkml/2007/10/15/105
> > 
> > I don't see how this works on a current kernel though, because I can't
> > find any path beyond the kvm_guest_exit() which clears the flag at the
> > moment.
> 
> Right, the flag used to be cleared in account_system_time().
> 
> >> This somehow got mangled when merging and later there was a fix that
> >> introduced the current behavior, which might be slightly more accurate:
> >> 0552f73b9a81 ("KVM: Move kvm_guest_exit() after local_irq_enable()")
> > 
> > So Laurent was confused as well?
> 
> It looks that Laurent understood that you need the timer tick before
> clearing the flag.  No idea how it really happened.
> 
> > It seems he needed to add that before you guys added
> > handle_external_intr()?
> 
> Yes.

In any case, it would be good for us to get some answers to the stuff
above, but I'll also try to prototype the load/put accounting of time.

Thanks,
-Christoffer