Re: [Qemu-devel] [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <address@hidden>
> À: "Sebastian Tanase" <address@hidden>, address@hidden
> Cc: address@hidden, address@hidden, address@hidden, "peter maydell" 
> <address@hidden>,
> address@hidden, address@hidden, address@hidden, address@hidden, 
> address@hidden,
> address@hidden, address@hidden, address@hidden, address@hidden, 
> address@hidden, "camille begue"
> <address@hidden>
> Envoyé: Mercredi 16 Juillet 2014 15:18:33
> Objet: Re: [RFC PATCH V4 6/6] monitor: Add drift info to 'info jit'
> 
> Il 16/07/2014 14:18, Sebastian Tanase ha scritto:
> > -    static int64_t clocks_offset;
> > -    if (!icount_align_option) {
> > -        return;
> > +    static int64_t realtime_clock_value;
> 
> Does this really need to be static?
> 
> > +    if (icount_align_option || !realtime_clock_value) {
> > +        realtime_clock_value =
> > qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >      }
> > -    sc->realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >      /* Compute offset between the 2 clocks. */
> >      if (!clocks_offset) {
> > -        clocks_offset = sc->realtime_clock -
> > +        clocks_offset = realtime_clock_value -
> >                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >      }
> 
> Isn't clocks_offset roughly the same as
> -timers_state.cpu_clock_offset?
>   If so, you could be some simplification in the code.  Feel free to
> move the TimersState struct definition to include/sysemu/cpus.h and
> make
> timers_state public.


-timers_state.cpu_clock_offset contains the offset between the real and virtual 
clocks.
However, when using the value of the virtual clock 
(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)),
qemu_icount_bias already includes this offset because, on ARM, qemu_clock_warp 
(which 
then calls icount_warp_rt) is called for the first time in tcg_exec_all, making
qemu_icount_bias take the value of qemu_clock_get_ns(QEMU_CLOCK_REALTIME)

    static void icount_warp_rt(void *opaque) {
        if (atomic_read(&vm_clock_warp_start) == -1) {
            return;
        }

        seqlock_write_lock(&timers_state.vm_clock_seqlock);
        if (runstate_is_running()) {
            int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
            int64_t warp_delta;
            ...
            warp_delta = clock - vm_clock_warp_start; //vm_clock_warp_start is 
0 the very first time
            qemu_icount_bias += warp_delta;
        }
        vm_clock_warp_start = -1;
        seqlock_write_unlock(&timers_state.vm_clock_seqlock);

        if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
            qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
        }
    }


Also, the first time qemu_clock_warp -> icount_warp_rt are called (on ARM), 
qemu_clock_expired(QEMU_CLOCK_VIRTUAL) 
is false because there are no active timers on the vm clock timer list; I'll 
explain why I bring this up
below.

A solution to not compute the initial offset in qemu_icount_bias would be to 
initialize 
vm_clock_warp_start to -1. The result  will be that qemu_icount_bias will start 
counting when
the vcpu goes from active to inactive. At that time, vm_clock_warp_start will 
already store the realtime clock 
value and a timer on the real clock will be set to expire at clock + deadline, 
making qemu_icount_bias increment
by deadline.
A consequence of initializing vm_clock_warp_start to -1 is also 
the fact that we'll skip the first check for a virtual expired timer. As I 
mentioned above, in ARM case, 
it's not dangerous because there are no timers active the first time we perform 
this check. However, this 
is just a potential scenario and I cannot guarantee that on other target 
architectures there won't be 
an expired timer pending the first time we check.

I also tested on x86:
qemu_clock_warp -> icount_warp_rt are first called on "pit_reset" after a 
virtual timer is set to expire.
However, in this case, the qemu_clock_expired(QEMU_CLOCK_VIRTUAL) fails because 
the current virtual clock is 0
so the timer doesn't have to expire yet. In this case also the above solution 
would work without breaking anything.


So, do you think it is worth taking this solution into account or it will cause 
more harm than good?

Sebastian

> 
> > +    cpu_fprintf(f, "Host - Guest clock  %ld(ms)\n",
> > +                (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) -
> > clocks_offset -
> > +                 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL))/SCALE_MS);
> 
> I think this is (cpu_get_clock() - cpu_get_icount()) / SCALE_MS.
> 
> Paolo
>