2020-03-24 15:20:36

by Tianyu Lan

[permalink] [raw]
Subject: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

sched clock callback should return time with nano second as unit
but current hv callback returns time with 100ns. Fix it.

Cc: [email protected]
Signed-off-by: Yubo Xie <[email protected]>
Signed-off-by: Tianyu Lan <[email protected]>
Fixes: adb87ff4f96c ("clocksource/drivers/hyperv: Allocate Hyper-V TSC page statically")
---
drivers/clocksource/hyperv_timer.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 9d808d595ca8..662ed978fa24 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -343,7 +343,8 @@ static u64 notrace read_hv_clock_tsc_cs(struct clocksource *arg)

static u64 read_hv_sched_clock_tsc(void)
{
- return read_hv_clock_tsc() - hv_sched_clock_offset;
+ return (read_hv_clock_tsc() - hv_sched_clock_offset)
+ * (NSEC_PER_SEC / HV_CLOCK_HZ);
}

static void suspend_hv_clock_tsc(struct clocksource *arg)
@@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)

static u64 read_hv_sched_clock_msr(void)
{
- return read_hv_clock_msr() - hv_sched_clock_offset;
+ return (read_hv_clock_msr() - hv_sched_clock_offset)
+ * (NSEC_PER_SEC / HV_CLOCK_HZ);
}

static struct clocksource hyperv_cs_msr = {
--
2.14.5


2020-03-24 15:51:39

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

Yubo Xie <[email protected]> writes:

> sched clock callback should return time with nano second as unit
> but current hv callback returns time with 100ns. Fix it.
>
> Cc: [email protected]
> Signed-off-by: Yubo Xie <[email protected]>
> Signed-off-by: Tianyu Lan <[email protected]>
> Fixes: adb87ff4f96c ("clocksource/drivers/hyperv: Allocate Hyper-V TSC page statically")

I don't think this is the right commit to reference,

commit bd00cd52d5be655a2f217e2ed74b91a71cb2b14f
Author: Tianyu Lan <[email protected]>
Date: Wed Aug 14 20:32:16 2019 +0800

clocksource/drivers/hyperv: Add Hyper-V specific sched clock function

looks like the one.

> ---
> drivers/clocksource/hyperv_timer.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index 9d808d595ca8..662ed978fa24 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -343,7 +343,8 @@ static u64 notrace read_hv_clock_tsc_cs(struct clocksource *arg)
>
> static u64 read_hv_sched_clock_tsc(void)
> {
> - return read_hv_clock_tsc() - hv_sched_clock_offset;
> + return (read_hv_clock_tsc() - hv_sched_clock_offset)
> + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> }
>
> static void suspend_hv_clock_tsc(struct clocksource *arg)
> @@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)
>
> static u64 read_hv_sched_clock_msr(void)
> {
> - return read_hv_clock_msr() - hv_sched_clock_offset;
> + return (read_hv_clock_msr() - hv_sched_clock_offset)
> + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> }

kvmclock seems to have the same (pre-patch) code ...

>
> static struct clocksource hyperv_cs_msr = {

--
Vitaly

2020-03-25 13:59:41

by Tianyu Lan

[permalink] [raw]
Subject: Re: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

Hi Vitaly:
Thanks for your review.

On 3/24/2020 11:49 PM, Vitaly Kuznetsov wrote:
> Yubo Xie <[email protected]> writes:
>
>> sched clock callback should return time with nano second as unit
>> but current hv callback returns time with 100ns. Fix it.
>>
>> Cc: [email protected]
>> Signed-off-by: Yubo Xie <[email protected]>
>> Signed-off-by: Tianyu Lan <[email protected]>
>> Fixes: adb87ff4f96c ("clocksource/drivers/hyperv: Allocate Hyper-V TSC page statically")
>
> I don't think this is the right commit to reference,
>
> commit bd00cd52d5be655a2f217e2ed74b91a71cb2b14f
> Author: Tianyu Lan <[email protected]>
> Date: Wed Aug 14 20:32:16 2019 +0800
>
> clocksource/drivers/hyperv: Add Hyper-V specific sched clock function
>
> looks like the one.

Sorry. You are right. Will update in the next version.

>
>> ---
>> drivers/clocksource/hyperv_timer.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
>> index 9d808d595ca8..662ed978fa24 100644
>> --- a/drivers/clocksource/hyperv_timer.c
>> +++ b/drivers/clocksource/hyperv_timer.c
>> @@ -343,7 +343,8 @@ static u64 notrace read_hv_clock_tsc_cs(struct clocksource *arg)
>>
>> static u64 read_hv_sched_clock_tsc(void)
>> {
>> - return read_hv_clock_tsc() - hv_sched_clock_offset;
>> + return (read_hv_clock_tsc() - hv_sched_clock_offset)
>> + * (NSEC_PER_SEC / HV_CLOCK_HZ);
>> }
>>
>> static void suspend_hv_clock_tsc(struct clocksource *arg)
>> @@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)
>>
>> static u64 read_hv_sched_clock_msr(void)
>> {
>> - return read_hv_clock_msr() - hv_sched_clock_offset;
>> + return (read_hv_clock_msr() - hv_sched_clock_offset)
>> + * (NSEC_PER_SEC / HV_CLOCK_HZ);
>> }
>
> kvmclock seems to have the same (pre-patch) code ...


kvm sched clock gets time from pvclock_clocksource_read() and
the time unit is nanosecond. So there is such issue in KVM code.

>
>>
>> static struct clocksource hyperv_cs_msr = {
>

2020-03-25 14:29:29

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

Tianyu Lan <[email protected]> writes:

>>> @@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)
>>>
>>> static u64 read_hv_sched_clock_msr(void)
>>> {
>>> - return read_hv_clock_msr() - hv_sched_clock_offset;
>>> + return (read_hv_clock_msr() - hv_sched_clock_offset)
>>> + * (NSEC_PER_SEC / HV_CLOCK_HZ);
>>> }
>>
>> kvmclock seems to have the same (pre-patch) code ...
>
>
> kvm sched clock gets time from pvclock_clocksource_read() and
> the time unit is nanosecond. So there is such issue in KVM code.
>

Ah, true, kvmclock is always 1Ghz so it's reading 'naturally' converts
to ns.

--
Vitaly

2020-03-25 18:48:26

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

> From: [email protected]
> <[email protected]> On Behalf Of Yubo Xie
> Sent: Tuesday, March 24, 2020 8:20 AM
> ...
> sched clock callback should return time with nano second as unit
> but current hv callback returns time with 100ns. Fix it.

Hi Yubo,
I'm curious how you found the bug? :-) Did you notice some kind of symtom?

Thanks,
-- Dexuan

2020-03-25 18:58:04

by Yubo Xie

[permalink] [raw]
Subject: RE: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

I use "time" to run some performance tests in our project under WSL2, but the result looks weird: the value of "real" is far from the sum of "user" + "sys".

-----Original Message-----
From: Dexuan Cui <[email protected]>
Sent: Wednesday, March 25, 2020 11:46 AM
To: Yubo Xie <[email protected]>; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; Wei Liu <[email protected]>; [email protected]; [email protected]; Michael Kelley <[email protected]>
Cc: Yubo Xie <[email protected]>; [email protected]; [email protected]; vkuznets <[email protected]>; [email protected]; Tianyu Lan <[email protected]>
Subject: RE: [PATCH] x86/Hyper-V: Fix hv sched clock function return wrong time unit

> From: [email protected]
> <[email protected]> On Behalf Of Yubo Xie
> Sent: Tuesday, March 24, 2020 8:20 AM
> ...
> sched clock callback should return time with nano second as unit but
> current hv callback returns time with 100ns. Fix it.

Hi Yubo,
I'm curious how you found the bug? :-) Did you notice some kind of symtom?

Thanks,
-- Dexuan