diff mbox

KVM: tsc deadline timer works only when hrtimer high resolution configured

Message ID DE8DF0795D48FD4CA783C40EC8292335322311@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liu, Jinsong Sept. 7, 2012, 12:07 p.m. UTC 
Avi Kivity wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 148ed66..0e64997 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2210,7 +2210,11 @@ int kvm_dev_ioctl_check_extension(long ext) 
>>  		r = kvm_has_tsc_control; break;
>>  	case KVM_CAP_TSC_DEADLINE_TIMER:
>> +#ifdef CONFIG_HIGH_RES_TIMERS
>>  		r = boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER); +#else
>> +		r = 0;
>> +#endif
>>  		break;
> 
> I prefer a patch making kvm for x86 depend on hrtimers.  kvm already
> provides a high resolution timer to the guest in the local apic,
> backing it with the jiffies event source will likely cause some
> guests to malfunction.

Yep, I did a draft test for kvm lapic timer, it also worked fail when CONFIG_HIGH_RES_TIMERS disabled.

Attached is the udpated patch.

Thanks,
Jinsong

Comments

Avi Kivity Sept. 9, 2012, 2:28 p.m. UTC | #1 
On 09/07/2012 03:07 PM, Liu, Jinsong wrote:
> Avi Kivity wrote:
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 148ed66..0e64997 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -2210,7 +2210,11 @@ int kvm_dev_ioctl_check_extension(long ext) 
>>>  		r = kvm_has_tsc_control; break;
>>>  	case KVM_CAP_TSC_DEADLINE_TIMER:
>>> +#ifdef CONFIG_HIGH_RES_TIMERS
>>>  		r = boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER); +#else
>>> +		r = 0;
>>> +#endif
>>>  		break;
>> 
>> I prefer a patch making kvm for x86 depend on hrtimers.  kvm already
>> provides a high resolution timer to the guest in the local apic,
>> backing it with the jiffies event source will likely cause some
>> guests to malfunction.
> 
> Yep, I did a draft test for kvm lapic timer, it also worked fail when CONFIG_HIGH_RES_TIMERS disabled.
> 
> Attached is the udpated patch.
> 
> Thanks,
> Jinsong
> 
> ====================
> From 64d0458ec50a7d6917adf1e9735ba6e6ae6024ad Mon Sep 17 00:00:00 2001
> From: Liu, Jinsong <jinsong.liu@intel.com>
> Date: Sat, 8 Sep 2012 03:32:31 +0800
> Subject: [PATCH] KVM: select HIGH_RES_TIMERS when KVM enabled
> 
> This is for 2 reasons:
> 1. it's pointless for kvm lapic timer and tsc deadline timer
> when kernel hrtimer not configured as high resolution, since
> that would be not accurate based on wheel;
> 2. kvm lapic timer and tsc deadline timer based on hrtimer,
> setting a leftmost node to rb tree and then do hrtimer reprogram.
> If hrtimer not configured as high resolution, hrtimer_enqueue_reprogram
> do nothing and then make kvm lapic timer and tsc deadline timer fail.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> ---
>  arch/x86/kvm/Kconfig |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index a28f338..5f861ca 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -24,6 +24,8 @@ config KVM
>  	depends on PCI
>  	# for TASKSTATS/TASK_DELAY_ACCT:
>  	depends on NET
> +	# for HIGH_RES_TIMERS
> +	depends on !ARCH_USES_GETTIMEOFFSET
>  	select PREEMPT_NOTIFIERS
>  	select MMU_NOTIFIER
>  	select ANON_INODES
> @@ -37,6 +39,8 @@ config KVM
>  	select TASK_DELAY_ACCT
>  	select PERF_EVENTS
>  	select HAVE_KVM_MSI
> +	select GENERIC_CLOCKEVENTS
> +	select HIGH_RES_TIMERS

hrtimers is an intrusive feature, I don't think we should force-enable
it.  Please change it to a depends on.
Liu, Jinsong Sept. 9, 2012, 2:54 p.m. UTC | #2 
Avi Kivity wrote:
> On 09/07/2012 03:07 PM, Liu, Jinsong wrote:
>> Avi Kivity wrote:
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 148ed66..0e64997 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -2210,7 +2210,11 @@ int kvm_dev_ioctl_check_extension(long ext)
>>>>  		r = kvm_has_tsc_control; break;
>>>>  	case KVM_CAP_TSC_DEADLINE_TIMER:
>>>> +#ifdef CONFIG_HIGH_RES_TIMERS
>>>>  		r = boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER); +#else +		r =
>>>> 0; +#endif
>>>>  		break;
>>> 
>>> I prefer a patch making kvm for x86 depend on hrtimers.  kvm already
>>> provides a high resolution timer to the guest in the local apic,
>>> backing it with the jiffies event source will likely cause some
>>> guests to malfunction.
>> 
>> Yep, I did a draft test for kvm lapic timer, it also worked fail
>> when CONFIG_HIGH_RES_TIMERS disabled. 
>> 
>> Attached is the udpated patch.
>> 
>> Thanks,
>> Jinsong
>> 
>> ====================
>> From 64d0458ec50a7d6917adf1e9735ba6e6ae6024ad Mon Sep 17 00:00:00
>> 2001 
>> From: Liu, Jinsong <jinsong.liu@intel.com>
>> Date: Sat, 8 Sep 2012 03:32:31 +0800
>> Subject: [PATCH] KVM: select HIGH_RES_TIMERS when KVM enabled
>> 
>> This is for 2 reasons:
>> 1. it's pointless for kvm lapic timer and tsc deadline timer
>> when kernel hrtimer not configured as high resolution, since
>> that would be not accurate based on wheel;
>> 2. kvm lapic timer and tsc deadline timer based on hrtimer,
>> setting a leftmost node to rb tree and then do hrtimer reprogram.
>> If hrtimer not configured as high resolution,
>> hrtimer_enqueue_reprogram 
>> do nothing and then make kvm lapic timer and tsc deadline timer fail.
>> 
>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> ---
>>  arch/x86/kvm/Kconfig |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
>> index a28f338..5f861ca 100644
>> --- a/arch/x86/kvm/Kconfig
>> +++ b/arch/x86/kvm/Kconfig
>> @@ -24,6 +24,8 @@ config KVM
>>  	depends on PCI
>>  	# for TASKSTATS/TASK_DELAY_ACCT:
>>  	depends on NET
>> +	# for HIGH_RES_TIMERS
>> +	depends on !ARCH_USES_GETTIMEOFFSET
>>  	select PREEMPT_NOTIFIERS
>>  	select MMU_NOTIFIER
>>  	select ANON_INODES
>> @@ -37,6 +39,8 @@ config KVM
>>  	select TASK_DELAY_ACCT
>>  	select PERF_EVENTS
>>  	select HAVE_KVM_MSI
>> +	select GENERIC_CLOCKEVENTS
>> +	select HIGH_RES_TIMERS
> 
> hrtimers is an intrusive feature, I don't think we should force-enable
> it.  Please change it to a depends on.

Hmm, if it changed as
config KVM
	depends on HIGH_RES_TIMERS
The item 'Kernel-based Virtual Machine (KVM) support (NEW)' even didn't appear to user when make menuconfig (when HIGH_RES_TIMERS disable)

Is it good? I just have a little concern here:)

Thanks,
Jinsong--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity Sept. 9, 2012, 2:59 p.m. UTC | #3 
On 09/09/2012 05:54 PM, Liu, Jinsong wrote:
>> 
>> hrtimers is an intrusive feature, I don't think we should force-enable
>> it.  Please change it to a depends on.
> 
> Hmm, if it changed as
> config KVM
> 	depends on HIGH_RES_TIMERS
> The item 'Kernel-based Virtual Machine (KVM) support (NEW)' even didn't appear to user when make menuconfig (when HIGH_RES_TIMERS disable)
> 
> Is it good? I just have a little concern here:)

It's not good, but that's what we have.

It's okay to force-enable low-impact features (like preempt notifies).

hrimers, on the other hand, change kernel behaviour quite deeply.

Maybe over time someone will fix the config tools to unhide features
that can be enabled by turning on a dependency.
diff mbox

Patch

====================
From 64d0458ec50a7d6917adf1e9735ba6e6ae6024ad Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Sat, 8 Sep 2012 03:32:31 +0800
Subject: [PATCH] KVM: select HIGH_RES_TIMERS when KVM enabled

This is for 2 reasons:
1. it's pointless for kvm lapic timer and tsc deadline timer
when kernel hrtimer not configured as high resolution, since
that would be not accurate based on wheel;
2. kvm lapic timer and tsc deadline timer based on hrtimer,
setting a leftmost node to rb tree and then do hrtimer reprogram.
If hrtimer not configured as high resolution, hrtimer_enqueue_reprogram
do nothing and then make kvm lapic timer and tsc deadline timer fail.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
---
 arch/x86/kvm/Kconfig |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index a28f338..5f861ca 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -24,6 +24,8 @@  config KVM
 	depends on PCI
 	# for TASKSTATS/TASK_DELAY_ACCT:
 	depends on NET
+	# for HIGH_RES_TIMERS
+	depends on !ARCH_USES_GETTIMEOFFSET
 	select PREEMPT_NOTIFIERS
 	select MMU_NOTIFIER
 	select ANON_INODES
@@ -37,6 +39,8 @@  config KVM
 	select TASK_DELAY_ACCT
 	select PERF_EVENTS
 	select HAVE_KVM_MSI
+	select GENERIC_CLOCKEVENTS
+	select HIGH_RES_TIMERS
 	---help---
 	  Support hosting fully virtualized guest machines using hardware
 	  virtualization extensions.  You will need a fairly recent