Re: [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers

[Roman Kagan]

On Fri, Nov 27, 2015 at 11:49:40AM +0100, Paolo Bonzini wrote:

[ sorry missed your message on Friday, replying now ]

> On 27/11/2015 09:12, Roman Kagan wrote:
> >> > +n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
> >> > +stimer->exp_time += n * stimer->count;
> > This is actually just a reminder calculation so I'd rather do it
> > directly with div64_u64_rem().
> 
> It took me a while to understand why it was a remained. :)

It gets easier if you think of it this way: we've slipped a few whole
periods and the remainder of the slack into the current period, so
the time left till the next tick is ("count" is the timer period here)

  delta = count - slack % count
where
  slack = time_now - exp_time

This gives you immediately your

>   exp_time = time_now + (count - (time_now - exp_time) % count)

Roman.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers

[Andrey Smetanin]

On 11/27/2015 01:49 PM, Paolo Bonzini wrote:



On 27/11/2015 09:12, Roman Kagan wrote:

+   n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
+   stimer->exp_time += n * stimer->count;

This is actually just a reminder calculation so I'd rather do it
directly with div64_u64_rem().


It took me a while to understand why it was a remained. :)  Expanding
Andrey's formula you get

exp_time = exp_time + count + (time_now - exp_time) / count * count

the remainder, (time_now - exp_time) % count would be

time_now - exp_time - (time_now - exp_time) / count * count

so -((time_now - exp_time) % count) is

exp_time - time_now + (time_now - exp_time) / count * count

so Andrey's expression is

exp_time = time_now + (count - (time_now - exp_time) % count)

Yeah, that looks nice.

I'll redo this way.


Paolo


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers

[Roman Kagan]

On Wed, Nov 25, 2015 at 06:20:21PM +0300, Andrey Smetanin wrote:
> Per Hyper-V specification (and as required by Hyper-V-aware guests),
> SynIC provides 4 per-vCPU timers.  Each timer is programmed via a pair
> of MSRs, and signals expiration by delivering a special format message
> to the configured SynIC message slot and triggering the corresponding
> synthetic interrupt.
> 
> Note: as implemented by this patch, all periodic timers are "lazy"
> (i.e. if the vCPU wasn't scheduled for more than the timer period the
> timer events are lost), regardless of the corresponding configuration
> MSR.  If deemed necessary, the "catch up" mode (the timer period is
> shortened until the timer catches up) will be implemented later.
> 
> Signed-off-by: Andrey Smetanin 
> CC: Gleb Natapov 
> CC: Paolo Bonzini 
> CC: "K. Y. Srinivasan" 
> CC: Haiyang Zhang 
> CC: Vitaly Kuznetsov 
> CC: Roman Kagan 
> CC: Denis V. Lunev 
> CC: qemu-de...@nongnu.org
> ---
>  arch/x86/include/asm/kvm_host.h|  13 ++
>  arch/x86/include/uapi/asm/hyperv.h |   6 +
>  arch/x86/kvm/hyperv.c  | 325 
> -
>  arch/x86/kvm/hyperv.h  |  24 +++
>  arch/x86/kvm/x86.c |   9 +
>  include/linux/kvm_host.h   |   1 +
>  6 files changed, 375 insertions(+), 3 deletions(-)

A couple of nitpicks:

> +static void stimer_restart(struct kvm_vcpu_hv_stimer *stimer)
> +{
> + u64 time_now;
> + ktime_t ktime_now;
> + u64 n;
> +
> + time_now = get_time_ref_counter(stimer_to_vcpu(stimer)->kvm);
> + ktime_now = ktime_get();
> +
> + /*
> +  * Calculate positive integer n for which condtion -
> +  * (stimer->exp_time + n * stimer->count) > time_now
> +  * is true. We will use (stimer->exp_time + n * stimer->count)
> +  * as new stimer->exp_time.
> +  */
> +
> + n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
> + stimer->exp_time += n * stimer->count;

This is actually just a reminder calculation so I'd rather do it
directly with div64_u64_rem().

> +void kvm_hv_process_stimers(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
> + struct kvm_vcpu_hv_stimer *stimer;
> + u64 time_now;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++)
> + if (test_and_clear_bit(i, hv_vcpu->stimer_pending_bitmap)) {
> + stimer = _vcpu->stimer[i];
> + stimer_stop(stimer);

I think there's no need in this explicit stop: I see no way to arrive
here with a running timer, and even if there were, it would be safe as
your timer callback only manipulates the bitmaps atomically.

Neither comment is critical so

Reviewed-by: Roman Kagan 

Roman.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 7/7] kvm/x86: Hyper-V SynIC timers

[Paolo Bonzini]

On 27/11/2015 09:12, Roman Kagan wrote:
>> > +  n = div64_u64(time_now - stimer->exp_time, stimer->count) + 1;
>> > +  stimer->exp_time += n * stimer->count;
> This is actually just a reminder calculation so I'd rather do it
> directly with div64_u64_rem().

It took me a while to understand why it was a remained. :)  Expanding
Andrey's formula you get

exp_time = exp_time + count + (time_now - exp_time) / count * count

the remainder, (time_now - exp_time) % count would be

time_now - exp_time - (time_now - exp_time) / count * count

so -((time_now - exp_time) % count) is

exp_time - time_now + (time_now - exp_time) / count * count

so Andrey's expression is

exp_time = time_now + (count - (time_now - exp_time) % count)

Yeah, that looks nice.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html