| Message ID | 1344171513-4659-6-git-send-email-gleb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/05/2012 03:58 PM, Gleb Natapov wrote: > Usually all APICs are HW enabled so the check can be optimized out. > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > --- > arch/x86/kvm/lapic.c | 29 ++++++++++++++++++++++++++++- > arch/x86/kvm/lapic.h | 1 + > arch/x86/kvm/x86.c | 1 + > 3 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index c3f14fe..1aa5528 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -34,6 +34,7 @@ > #include <asm/current.h> > #include <asm/apicdef.h> > #include <linux/atomic.h> > +#include <linux/jump_label.h> > #include "kvm_cache_regs.h" > #include "irq.h" > #include "trace.h" > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap) > return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); > } > > +struct static_key_deferred apic_hw_disabled __read_mostly; On top of file please. Add all_ to the name to make it clear we're talking about all apics. > + > static inline int apic_hw_enabled(struct kvm_lapic *apic) > { > - return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; > + if (static_key_false(&apic_hw_disabled.key)) > + return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; Hm, for the test to be readable, it needs to be if (static_key_false(&all_apics_hw_enabled))
On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote: > On 08/05/2012 03:58 PM, Gleb Natapov wrote: > > Usually all APICs are HW enabled so the check can be optimized out. > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > --- > > arch/x86/kvm/lapic.c | 29 ++++++++++++++++++++++++++++- > > arch/x86/kvm/lapic.h | 1 + > > arch/x86/kvm/x86.c | 1 + > > 3 files changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index c3f14fe..1aa5528 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -34,6 +34,7 @@ > > #include <asm/current.h> > > #include <asm/apicdef.h> > > #include <linux/atomic.h> > > +#include <linux/jump_label.h> > > #include "kvm_cache_regs.h" > > #include "irq.h" > > #include "trace.h" > > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap) > > return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); > > } > > > > +struct static_key_deferred apic_hw_disabled __read_mostly; > > On top of file please. Add all_ to the name to make it clear we're > talking about all apics. > This is count of disabled apics really. So I think all_apic_hw_disabled is misleading. > > + > > static inline int apic_hw_enabled(struct kvm_lapic *apic) > > { > > - return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; > > + if (static_key_false(&apic_hw_disabled.key)) > > + return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; > > Hm, for the test to be readable, it needs to be > > if (static_key_false(&all_apics_hw_enabled)) > Exactly. all_ makes it so because apic_hw_disabled is a counter that counts disabled apics. So may be call it global_hw_disabled_apic_counter? -- Gleb. -- 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
On 08/05/2012 05:42 PM, Gleb Natapov wrote: > On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote: >> On 08/05/2012 03:58 PM, Gleb Natapov wrote: >> > Usually all APICs are HW enabled so the check can be optimized out. >> > >> > Signed-off-by: Gleb Natapov <gleb@redhat.com> >> > --- >> > arch/x86/kvm/lapic.c | 29 ++++++++++++++++++++++++++++- >> > arch/x86/kvm/lapic.h | 1 + >> > arch/x86/kvm/x86.c | 1 + >> > 3 files changed, 30 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> > index c3f14fe..1aa5528 100644 >> > --- a/arch/x86/kvm/lapic.c >> > +++ b/arch/x86/kvm/lapic.c >> > @@ -34,6 +34,7 @@ >> > #include <asm/current.h> >> > #include <asm/apicdef.h> >> > #include <linux/atomic.h> >> > +#include <linux/jump_label.h> >> > #include "kvm_cache_regs.h" >> > #include "irq.h" >> > #include "trace.h" >> > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap) >> > return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); >> > } >> > >> > +struct static_key_deferred apic_hw_disabled __read_mostly; >> >> On top of file please. Add all_ to the name to make it clear we're >> talking about all apics. >> > This is count of disabled apics really. So I think all_apic_hw_disabled > is misleading. > >> > + >> > static inline int apic_hw_enabled(struct kvm_lapic *apic) >> > { >> > - return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; >> > + if (static_key_false(&apic_hw_disabled.key)) >> > + return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; >> >> Hm, for the test to be readable, it needs to be >> >> if (static_key_false(&all_apics_hw_enabled)) >> > Exactly. all_ makes it so because apic_hw_disabled is a counter that > counts disabled apics. So may be call it global_hw_disabled_apic_counter? > The problem is how static_key_false() is defined. It returns true if the count > 0, opposite from what I'd expect. So anything with counter semantics will be confusing. I guess we need to pick a neutral name (apic_disabled_key or apic_disabled_slowpath or such) to force the reader to look at the definitions.
On Sun, Aug 05, 2012 at 05:48:42PM +0300, Avi Kivity wrote: > On 08/05/2012 05:42 PM, Gleb Natapov wrote: > > On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote: > >> On 08/05/2012 03:58 PM, Gleb Natapov wrote: > >> > Usually all APICs are HW enabled so the check can be optimized out. > >> > > >> > Signed-off-by: Gleb Natapov <gleb@redhat.com> > >> > --- > >> > arch/x86/kvm/lapic.c | 29 ++++++++++++++++++++++++++++- > >> > arch/x86/kvm/lapic.h | 1 + > >> > arch/x86/kvm/x86.c | 1 + > >> > 3 files changed, 30 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > >> > index c3f14fe..1aa5528 100644 > >> > --- a/arch/x86/kvm/lapic.c > >> > +++ b/arch/x86/kvm/lapic.c > >> > @@ -34,6 +34,7 @@ > >> > #include <asm/current.h> > >> > #include <asm/apicdef.h> > >> > #include <linux/atomic.h> > >> > +#include <linux/jump_label.h> > >> > #include "kvm_cache_regs.h" > >> > #include "irq.h" > >> > #include "trace.h" > >> > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap) > >> > return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); > >> > } > >> > > >> > +struct static_key_deferred apic_hw_disabled __read_mostly; > >> > >> On top of file please. Add all_ to the name to make it clear we're > >> talking about all apics. > >> > > This is count of disabled apics really. So I think all_apic_hw_disabled > > is misleading. > > > >> > + > >> > static inline int apic_hw_enabled(struct kvm_lapic *apic) > >> > { > >> > - return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; > >> > + if (static_key_false(&apic_hw_disabled.key)) > >> > + return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; > >> > >> Hm, for the test to be readable, it needs to be > >> > >> if (static_key_false(&all_apics_hw_enabled)) > >> > > Exactly. all_ makes it so because apic_hw_disabled is a counter that > > counts disabled apics. So may be call it global_hw_disabled_apic_counter? > > > > The problem is how static_key_false() is defined. It returns true if > the count > 0, opposite from what I'd expect. So anything with counter > semantics will be confusing. I guess we need to pick a neutral name > (apic_disabled_key or apic_disabled_slowpath or such) to force the > reader to look at the definitions. > There was a long thread about readability of static_key_true/false and this is the result of it :( I read it this way: drop static_key_(true|false) and read the if() to understand its meaning. Now look at static_key_(true|false) to see what fast path will be in asm code. -- Gleb. -- 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
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index c3f14fe..1aa5528 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -34,6 +34,7 @@ #include <asm/current.h> #include <asm/apicdef.h> #include <linux/atomic.h> +#include <linux/jump_label.h> #include "kvm_cache_regs.h" #include "irq.h" #include "trace.h" @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap) return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec)); } +struct static_key_deferred apic_hw_disabled __read_mostly; + static inline int apic_hw_enabled(struct kvm_lapic *apic) { - return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; + if (static_key_false(&apic_hw_disabled.key)) + return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; + return MSR_IA32_APICBASE_ENABLE; } static inline int apic_sw_enabled(struct kvm_lapic *apic) @@ -1055,6 +1060,9 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu) hrtimer_cancel(&vcpu->arch.apic->lapic_timer.timer); + if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)) + static_key_slow_dec_deferred(&apic_hw_disabled); + if (vcpu->arch.apic->regs) free_page((unsigned long)vcpu->arch.apic->regs); @@ -1125,6 +1133,14 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) return; } + /* update jump label if enable bit changes */ + if ((vcpu->arch.apic_base ^ value) & MSR_IA32_APICBASE_ENABLE) { + if (value & MSR_IA32_APICBASE_ENABLE) + static_key_slow_dec_deferred(&apic_hw_disabled); + else + static_key_slow_inc(&apic_hw_disabled.key); + } + if (!kvm_vcpu_is_bsp(apic->vcpu)) value &= ~MSR_IA32_APICBASE_BSP; @@ -1311,6 +1327,11 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu) HRTIMER_MODE_ABS); apic->lapic_timer.timer.function = apic_timer_fn; + /* + * APIC is created enabled. This will prevent kvm_lapic_set_base from + * thinking that APIC satet has changed. + */ + vcpu->arch.apic_base = MSR_IA32_APICBASE_ENABLE; kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE); @@ -1598,3 +1619,9 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data) return kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.pv_eoi.data, addr); } + +void kvm_lapic_init(void) +{ + /* do not patch jump label more than once per second */ + jump_label_rate_limit(&apic_hw_disabled, HZ); +} diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index 166766f..73fa299 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -78,4 +78,5 @@ static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu) } int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data); +void kvm_lapic_init(void); #endif diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f339b2c..12812db 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4908,6 +4908,7 @@ int kvm_arch_init(void *opaque) if (cpu_has_xsave) host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK); + kvm_lapic_init(); return 0; out:
Usually all APICs are HW enabled so the check can be optimized out. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- arch/x86/kvm/lapic.c | 29 ++++++++++++++++++++++++++++- arch/x86/kvm/lapic.h | 1 + arch/x86/kvm/x86.c | 1 + 3 files changed, 30 insertions(+), 1 deletion(-)