| Message ID | 20190412201834.10831-5-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: lapic: Fix a variety of timer adv issues | expand |
> On 12 Apr 2019, at 23:18, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Disable auto-tuning the timer advancement if the user specifies an > explicit value via the module param. Aside from the obvious override > capability, this also allows the KVM admin to set the advancement > beyond the internally-capped max of 5000ns. > > Cc: Wanpeng Li <wanpengli@tencent.com> > Fixes: 3b8a5df6c4dc6 ("KVM: LAPIC: Tune lapic_timer_advance_ns automatically") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> I agree we should allow admin the ability to disable the auto-tuning. However, I think we should keep the semantic of lapic_timer_advance_ns value. Whether this value is used as initial-value for auto-tuning or is auto-tuning disabled is a different knob for admin in my opinion. Therefore, I think we should just add another module parameter which basically set the initial value for apic->lapic_timer.timer_advance_adjust_done. One could also wonder if it makes sense that whether auto-tuning will be enabled or not varies between VMs and should not be a global variable of KVM module? -Liran > --- > arch/x86/kvm/lapic.c | 2 ++ > arch/x86/kvm/lapic.h | 2 ++ > arch/x86/kvm/x86.c | 2 +- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 70a0acd98e9e..c43cd26f040b 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2286,6 +2286,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, u32 timer_advance_ns) > HRTIMER_MODE_ABS_PINNED); > apic->lapic_timer.timer.function = apic_timer_fn; > apic->lapic_timer.timer_advance_ns = timer_advance_ns; > + if (timer_advance_ns != LAPIC_TIMER_ADVANCE_DEFAULT_NS) > + apic->lapic_timer.timer_advance_adjust_done = true; > > /* > * APIC is created enabled. This will prevent kvm_lapic_set_base from > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 3e97f8a68967..c7233629c05b 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -16,6 +16,8 @@ > #define APIC_BUS_CYCLE_NS 1 > #define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS) > > +#define LAPIC_TIMER_ADVANCE_DEFAULT_NS 1000 > + > enum lapic_mode { > LAPIC_MODE_DISABLED = 0, > LAPIC_MODE_INVALID = X2APIC_ENABLE, > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b303a21a2bc2..709a8bf5ae0e 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -137,7 +137,7 @@ static u32 __read_mostly tsc_tolerance_ppm = 250; > module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR); > > /* lapic timer advance (tscdeadline mode only) in nanoseconds */ > -static u32 __read_mostly lapic_timer_advance_ns = 1000; > +static u32 __read_mostly lapic_timer_advance_ns = LAPIC_TIMER_ADVANCE_DEFAULT_NS; > module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR); > > static bool __read_mostly vector_hashing = true; > -- > 2.21.0 >
On Sun, Apr 14, 2019 at 02:35:39PM +0300, Liran Alon wrote: > > > > On 12 Apr 2019, at 23:18, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > Disable auto-tuning the timer advancement if the user specifies an > > explicit value via the module param. Aside from the obvious override > > capability, this also allows the KVM admin to set the advancement > > beyond the internally-capped max of 5000ns. > > > > Cc: Wanpeng Li <wanpengli@tencent.com> > > Fixes: 3b8a5df6c4dc6 ("KVM: LAPIC: Tune lapic_timer_advance_ns automatically") > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > I agree we should allow admin the ability to disable the auto-tuning. > However, I think we should keep the semantic of lapic_timer_advance_ns value. > Whether this value is used as initial-value for auto-tuning or is auto-tuning > disabled is a different knob for admin in my opinion. Therefore, I think we > should just add another module parameter which basically set the initial > value for apic->lapic_timer.timer_advance_adjust_done. I waffled on whether to add a param or do the implicit override. I opted for the implicit behavior primarily because I think most people would expect that setting lapic_timer_advance_ns to some specific value would fix the advancement at said value, as opposed to acting as a hint to the autotuning logic. In other words, I again don't have a strong opinion :-) > One could also wonder if it makes sense that whether auto-tuning will be > enabled or not varies between VMs and should not be a global variable of KVM > module? I have no opinion on this one as I don't have any direct visibility into how the auto-tuning will be used in real world deployments. > > -Liran > > > --- > > arch/x86/kvm/lapic.c | 2 ++ > > arch/x86/kvm/lapic.h | 2 ++ > > arch/x86/kvm/x86.c | 2 +- > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 70a0acd98e9e..c43cd26f040b 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2286,6 +2286,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, u32 timer_advance_ns) > > HRTIMER_MODE_ABS_PINNED); > > apic->lapic_timer.timer.function = apic_timer_fn; > > apic->lapic_timer.timer_advance_ns = timer_advance_ns; > > + if (timer_advance_ns != LAPIC_TIMER_ADVANCE_DEFAULT_NS) > > + apic->lapic_timer.timer_advance_adjust_done = true; > > > > /* > > * APIC is created enabled. This will prevent kvm_lapic_set_base from > > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > > index 3e97f8a68967..c7233629c05b 100644 > > --- a/arch/x86/kvm/lapic.h > > +++ b/arch/x86/kvm/lapic.h > > @@ -16,6 +16,8 @@ > > #define APIC_BUS_CYCLE_NS 1 > > #define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS) > > > > +#define LAPIC_TIMER_ADVANCE_DEFAULT_NS 1000 > > + > > enum lapic_mode { > > LAPIC_MODE_DISABLED = 0, > > LAPIC_MODE_INVALID = X2APIC_ENABLE, > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index b303a21a2bc2..709a8bf5ae0e 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -137,7 +137,7 @@ static u32 __read_mostly tsc_tolerance_ppm = 250; > > module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR); > > > > /* lapic timer advance (tscdeadline mode only) in nanoseconds */ > > -static u32 __read_mostly lapic_timer_advance_ns = 1000; > > +static u32 __read_mostly lapic_timer_advance_ns = LAPIC_TIMER_ADVANCE_DEFAULT_NS; > > module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR); > > > > static bool __read_mostly vector_hashing = true; > > -- > > 2.21.0 > > >
> On 15 Apr 2019, at 19:23, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Sun, Apr 14, 2019 at 02:35:39PM +0300, Liran Alon wrote: >> >> >>> On 12 Apr 2019, at 23:18, Sean Christopherson <sean.j.christopherson@intel.com> wrote: >>> >>> Disable auto-tuning the timer advancement if the user specifies an >>> explicit value via the module param. Aside from the obvious override >>> capability, this also allows the KVM admin to set the advancement >>> beyond the internally-capped max of 5000ns. >>> >>> Cc: Wanpeng Li <wanpengli@tencent.com> >>> Fixes: 3b8a5df6c4dc6 ("KVM: LAPIC: Tune lapic_timer_advance_ns automatically") >>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >> >> I agree we should allow admin the ability to disable the auto-tuning. >> However, I think we should keep the semantic of lapic_timer_advance_ns value. >> Whether this value is used as initial-value for auto-tuning or is auto-tuning >> disabled is a different knob for admin in my opinion. Therefore, I think we >> should just add another module parameter which basically set the initial >> value for apic->lapic_timer.timer_advance_adjust_done. > > I waffled on whether to add a param or do the implicit override. I opted > for the implicit behavior primarily because I think most people would > expect that setting lapic_timer_advance_ns to some specific value would > fix the advancement at said value, as opposed to acting as a hint to the > autotuning logic. In other words, I again don't have a strong opinion :-) Let’s wait for Paolo’s opinion on this then :) BTW, a nice side-effect of my proposal is that also the current value used by KVM module at runtime (after auto-tuning) can be viewed by printing current value of lapic_timer_advance_ns module_param. > >> One could also wonder if it makes sense that whether auto-tuning will be >> enabled or not varies between VMs and should not be a global variable of KVM >> module? > > I have no opinion on this one as I don't have any direct visibility into > how the auto-tuning will be used in real world deployments. I will wait for Paolo’s & Wanpeng’s opinion on this as-well. -Liran > >> >> -Liran >> >>> --- >>> arch/x86/kvm/lapic.c | 2 ++ >>> arch/x86/kvm/lapic.h | 2 ++ >>> arch/x86/kvm/x86.c | 2 +- >>> 3 files changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>> index 70a0acd98e9e..c43cd26f040b 100644 >>> --- a/arch/x86/kvm/lapic.c >>> +++ b/arch/x86/kvm/lapic.c >>> @@ -2286,6 +2286,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, u32 timer_advance_ns) >>> HRTIMER_MODE_ABS_PINNED); >>> apic->lapic_timer.timer.function = apic_timer_fn; >>> apic->lapic_timer.timer_advance_ns = timer_advance_ns; >>> + if (timer_advance_ns != LAPIC_TIMER_ADVANCE_DEFAULT_NS) >>> + apic->lapic_timer.timer_advance_adjust_done = true; >>> >>> /* >>> * APIC is created enabled. This will prevent kvm_lapic_set_base from >>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h >>> index 3e97f8a68967..c7233629c05b 100644 >>> --- a/arch/x86/kvm/lapic.h >>> +++ b/arch/x86/kvm/lapic.h >>> @@ -16,6 +16,8 @@ >>> #define APIC_BUS_CYCLE_NS 1 >>> #define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS) >>> >>> +#define LAPIC_TIMER_ADVANCE_DEFAULT_NS 1000 >>> + >>> enum lapic_mode { >>> LAPIC_MODE_DISABLED = 0, >>> LAPIC_MODE_INVALID = X2APIC_ENABLE, >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index b303a21a2bc2..709a8bf5ae0e 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -137,7 +137,7 @@ static u32 __read_mostly tsc_tolerance_ppm = 250; >>> module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR); >>> >>> /* lapic timer advance (tscdeadline mode only) in nanoseconds */ >>> -static u32 __read_mostly lapic_timer_advance_ns = 1000; >>> +static u32 __read_mostly lapic_timer_advance_ns = LAPIC_TIMER_ADVANCE_DEFAULT_NS; >>> module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR); >>> >>> static bool __read_mostly vector_hashing = true; >>> -- >>> 2.21.0 >>> >>
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 70a0acd98e9e..c43cd26f040b 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2286,6 +2286,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, u32 timer_advance_ns) HRTIMER_MODE_ABS_PINNED); apic->lapic_timer.timer.function = apic_timer_fn; apic->lapic_timer.timer_advance_ns = timer_advance_ns; + if (timer_advance_ns != LAPIC_TIMER_ADVANCE_DEFAULT_NS) + apic->lapic_timer.timer_advance_adjust_done = true; /* * APIC is created enabled. This will prevent kvm_lapic_set_base from diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index 3e97f8a68967..c7233629c05b 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -16,6 +16,8 @@ #define APIC_BUS_CYCLE_NS 1 #define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS) +#define LAPIC_TIMER_ADVANCE_DEFAULT_NS 1000 + enum lapic_mode { LAPIC_MODE_DISABLED = 0, LAPIC_MODE_INVALID = X2APIC_ENABLE, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b303a21a2bc2..709a8bf5ae0e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -137,7 +137,7 @@ static u32 __read_mostly tsc_tolerance_ppm = 250; module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR); /* lapic timer advance (tscdeadline mode only) in nanoseconds */ -static u32 __read_mostly lapic_timer_advance_ns = 1000; +static u32 __read_mostly lapic_timer_advance_ns = LAPIC_TIMER_ADVANCE_DEFAULT_NS; module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR); static bool __read_mostly vector_hashing = true;
Disable auto-tuning the timer advancement if the user specifies an explicit value via the module param. Aside from the obvious override capability, this also allows the KVM admin to set the advancement beyond the internally-capped max of 5000ns. Cc: Wanpeng Li <wanpengli@tencent.com> Fixes: 3b8a5df6c4dc6 ("KVM: LAPIC: Tune lapic_timer_advance_ns automatically") Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/lapic.c | 2 ++ arch/x86/kvm/lapic.h | 2 ++ arch/x86/kvm/x86.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-)