Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
detect hypervisor's type in early boot. There is no point in having
always-zero HYP: line on other hypervisors or bare metal.
Suggested-by: Radim Krčmář <[email protected]>
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kernel/irq.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 88b36648..0c82064 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -18,6 +18,7 @@
#include <asm/mce.h>
#include <asm/hw_irq.h>
#include <asm/desc.h>
+#include <asm/hypervisor.h>
#define CREATE_TRACE_POINTS
#include <asm/trace/irq_vectors.h>
@@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
seq_puts(p, " Machine check polls\n");
#endif
#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
- seq_printf(p, "%*s: ", prec, "HYP");
- for_each_online_cpu(j)
- seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
- seq_puts(p, " Hypervisor callback interrupts\n");
+ if (x86_hyper == &x86_hyper_ms_hyperv ||
+ x86_hyper == &x86_hyper_xen) {
+ seq_printf(p, "%*s: ", prec, "HYP");
+ for_each_online_cpu(j)
+ seq_printf(p, "%10u ",
+ irq_stats(j)->irq_hv_callback_count);
+ seq_puts(p, " Hypervisor callback interrupts\n");
+ }
#endif
seq_printf(p, "%*s: %10u\n", prec, "ERR", atomic_read(&irq_err_count));
#if defined(CONFIG_X86_IO_APIC)
--
2.4.3
* Vitaly Kuznetsov <[email protected]> wrote:
> Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
> detect hypervisor's type in early boot. There is no point in having
> always-zero HYP: line on other hypervisors or bare metal.
>
> Suggested-by: Radim Krčmář <[email protected]>
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> ---
> arch/x86/kernel/irq.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 88b36648..0c82064 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -18,6 +18,7 @@
> #include <asm/mce.h>
> #include <asm/hw_irq.h>
> #include <asm/desc.h>
> +#include <asm/hypervisor.h>
>
> #define CREATE_TRACE_POINTS
> #include <asm/trace/irq_vectors.h>
> @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
> seq_puts(p, " Machine check polls\n");
> #endif
> #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
> - seq_printf(p, "%*s: ", prec, "HYP");
> - for_each_online_cpu(j)
> - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
> - seq_puts(p, " Hypervisor callback interrupts\n");
> + if (x86_hyper == &x86_hyper_ms_hyperv ||
> + x86_hyper == &x86_hyper_xen) {
> + seq_printf(p, "%*s: ", prec, "HYP");
> + for_each_online_cpu(j)
> + seq_printf(p, "%10u ",
> + irq_stats(j)->irq_hv_callback_count);
> + seq_puts(p, " Hypervisor callback interrupts\n");
> + }
So I think we should simplify this to:
if (x86_hyper) {
...
}
this will print the HYP line on hypervisors that don't use
HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
won't accidentally skip this line on hypervisors that start using the callback
IRQ.
Thanks,
Ingo
On Sat, Jul 04, 2015 at 10:26:53AM +0200, Ingo Molnar wrote:
>
> * Vitaly Kuznetsov <[email protected]> wrote:
>
> > Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
> > detect hypervisor's type in early boot. There is no point in having
> > always-zero HYP: line on other hypervisors or bare metal.
> >
> > Suggested-by: Radim Krčmář <[email protected]>
> > Signed-off-by: Vitaly Kuznetsov <[email protected]>
> > ---
> > arch/x86/kernel/irq.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> > index 88b36648..0c82064 100644
> > --- a/arch/x86/kernel/irq.c
> > +++ b/arch/x86/kernel/irq.c
> > @@ -18,6 +18,7 @@
> > #include <asm/mce.h>
> > #include <asm/hw_irq.h>
> > #include <asm/desc.h>
> > +#include <asm/hypervisor.h>
> >
> > #define CREATE_TRACE_POINTS
> > #include <asm/trace/irq_vectors.h>
> > @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
> > seq_puts(p, " Machine check polls\n");
> > #endif
> > #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
> > - seq_printf(p, "%*s: ", prec, "HYP");
> > - for_each_online_cpu(j)
> > - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
> > - seq_puts(p, " Hypervisor callback interrupts\n");
> > + if (x86_hyper == &x86_hyper_ms_hyperv ||
> > + x86_hyper == &x86_hyper_xen) {
> > + seq_printf(p, "%*s: ", prec, "HYP");
> > + for_each_online_cpu(j)
> > + seq_printf(p, "%10u ",
> > + irq_stats(j)->irq_hv_callback_count);
> > + seq_puts(p, " Hypervisor callback interrupts\n");
> > + }
>
> So I think we should simplify this to:
>
> if (x86_hyper) {
> ...
> }
>
> this will print the HYP line on hypervisors that don't use
> HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
> won't accidentally skip this line on hypervisors that start using the callback
> IRQ.
This is a good point. When virt stuff creeps out, it's easy to lose track of
it. But, I would prefer that vmware and kvm guests don't have to have a
meaningless counter in /proc/interrupts. Also, for xen, I see that it only
matters for x86. ARM xen builds would also have a meaningless counter. Maybe
we should add a flags member to hypervisor_x86, and start using that in
these types of situations.
Thanks,
drew
On Mon, Jul 06, 2015 at 03:57:33PM +0200, Andrew Jones wrote:
> On Sat, Jul 04, 2015 at 10:26:53AM +0200, Ingo Molnar wrote:
> >
> > * Vitaly Kuznetsov <[email protected]> wrote:
> >
> > > Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
> > > detect hypervisor's type in early boot. There is no point in having
> > > always-zero HYP: line on other hypervisors or bare metal.
> > >
> > > Suggested-by: Radim Krčmář <[email protected]>
> > > Signed-off-by: Vitaly Kuznetsov <[email protected]>
> > > ---
> > > arch/x86/kernel/irq.c | 13 +++++++++----
> > > 1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> > > index 88b36648..0c82064 100644
> > > --- a/arch/x86/kernel/irq.c
> > > +++ b/arch/x86/kernel/irq.c
> > > @@ -18,6 +18,7 @@
> > > #include <asm/mce.h>
> > > #include <asm/hw_irq.h>
> > > #include <asm/desc.h>
> > > +#include <asm/hypervisor.h>
> > >
> > > #define CREATE_TRACE_POINTS
> > > #include <asm/trace/irq_vectors.h>
> > > @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
> > > seq_puts(p, " Machine check polls\n");
> > > #endif
> > > #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
> > > - seq_printf(p, "%*s: ", prec, "HYP");
> > > - for_each_online_cpu(j)
> > > - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
> > > - seq_puts(p, " Hypervisor callback interrupts\n");
> > > + if (x86_hyper == &x86_hyper_ms_hyperv ||
> > > + x86_hyper == &x86_hyper_xen) {
> > > + seq_printf(p, "%*s: ", prec, "HYP");
> > > + for_each_online_cpu(j)
> > > + seq_printf(p, "%10u ",
> > > + irq_stats(j)->irq_hv_callback_count);
> > > + seq_puts(p, " Hypervisor callback interrupts\n");
> > > + }
> >
> > So I think we should simplify this to:
> >
> > if (x86_hyper) {
> > ...
> > }
> >
> > this will print the HYP line on hypervisors that don't use
> > HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
> > won't accidentally skip this line on hypervisors that start using the callback
> > IRQ.
>
> This is a good point. When virt stuff creeps out, it's easy to lose track of
> it. But, I would prefer that vmware and kvm guests don't have to have a
> meaningless counter in /proc/interrupts. Also, for xen, I see that it only
> matters for x86. ARM xen builds would also have a meaningless counter. Maybe
> we should add a flags member to hypervisor_x86, and start using that in
> these types of situations.
>
Don't know why I just referred to ARM in the context of *x86*_hyper..., but
the flag idea might still be nice.
On 07/06/2015 09:57 AM, Andrew Jones wrote:
> On Sat, Jul 04, 2015 at 10:26:53AM +0200, Ingo Molnar wrote:
>> * Vitaly Kuznetsov <[email protected]> wrote:
>>
>>> Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
>>> detect hypervisor's type in early boot. There is no point in having
>>> always-zero HYP: line on other hypervisors or bare metal.
>>>
>>> Suggested-by: Radim Krčmář <[email protected]>
>>> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>>> ---
>>> arch/x86/kernel/irq.c | 13 +++++++++----
>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>>> index 88b36648..0c82064 100644
>>> --- a/arch/x86/kernel/irq.c
>>> +++ b/arch/x86/kernel/irq.c
>>> @@ -18,6 +18,7 @@
>>> #include <asm/mce.h>
>>> #include <asm/hw_irq.h>
>>> #include <asm/desc.h>
>>> +#include <asm/hypervisor.h>
>>>
>>> #define CREATE_TRACE_POINTS
>>> #include <asm/trace/irq_vectors.h>
>>> @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
>>> seq_puts(p, " Machine check polls\n");
>>> #endif
>>> #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
>>> - seq_printf(p, "%*s: ", prec, "HYP");
>>> - for_each_online_cpu(j)
>>> - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
>>> - seq_puts(p, " Hypervisor callback interrupts\n");
>>> + if (x86_hyper == &x86_hyper_ms_hyperv ||
>>> + x86_hyper == &x86_hyper_xen) {
>>> + seq_printf(p, "%*s: ", prec, "HYP");
>>> + for_each_online_cpu(j)
>>> + seq_printf(p, "%10u ",
>>> + irq_stats(j)->irq_hv_callback_count);
>>> + seq_puts(p, " Hypervisor callback interrupts\n");
>>> + }
>> So I think we should simplify this to:
>>
>> if (x86_hyper) {
>> ...
>> }
>>
>> this will print the HYP line on hypervisors that don't use
>> HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
>> won't accidentally skip this line on hypervisors that start using the callback
>> IRQ.
> This is a good point. When virt stuff creeps out, it's easy to lose track of
> it. But, I would prefer that vmware and kvm guests don't have to have a
> meaningless counter in /proc/interrupts. Also, for xen, I see that it only
> matters for x86. ARM xen builds would also have a meaningless counter. Maybe
> we should add a flags member to hypervisor_x86, and start using that in
> these types of situations.
This is all x86 arch code so I don't see how it will make any difference
to Xen on ARM.
-boris
Ingo Molnar <[email protected]> writes:
> * Vitaly Kuznetsov <[email protected]> wrote:
>
>> Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
>> detect hypervisor's type in early boot. There is no point in having
>> always-zero HYP: line on other hypervisors or bare metal.
>>
>> Suggested-by: Radim Krčmář <[email protected]>
>> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>> ---
>> arch/x86/kernel/irq.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index 88b36648..0c82064 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -18,6 +18,7 @@
>> #include <asm/mce.h>
>> #include <asm/hw_irq.h>
>> #include <asm/desc.h>
>> +#include <asm/hypervisor.h>
>>
>> #define CREATE_TRACE_POINTS
>> #include <asm/trace/irq_vectors.h>
>> @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
>> seq_puts(p, " Machine check polls\n");
>> #endif
>> #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
>> - seq_printf(p, "%*s: ", prec, "HYP");
>> - for_each_online_cpu(j)
>> - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
>> - seq_puts(p, " Hypervisor callback interrupts\n");
>> + if (x86_hyper == &x86_hyper_ms_hyperv ||
>> + x86_hyper == &x86_hyper_xen) {
>> + seq_printf(p, "%*s: ", prec, "HYP");
>> + for_each_online_cpu(j)
>> + seq_printf(p, "%10u ",
>> + irq_stats(j)->irq_hv_callback_count);
>> + seq_puts(p, " Hypervisor callback interrupts\n");
>> + }
>
> So I think we should simplify this to:
>
> if (x86_hyper) {
> ...
> }
>
> this will print the HYP line on hypervisors that don't use
> HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
> won't accidentally skip this line on hypervisors that start using the callback
> IRQ.
I was under an impression KVM and Vmware don't use these hypervisor
callback irqs 'by desing' and even if one day they start using it
someone will have to put inc_irq_stat() somewhere.
If the goal is to avoid having x86_hyper_ms_hyperv/x86_hyper_xen in
irq.c we can follow Andrew's suggestion and add a flag
(use_callback_irqs?) to struct hypervisor_x86.
Thanks,
--
Vitaly
* Vitaly Kuznetsov <[email protected]> wrote:
> Ingo Molnar <[email protected]> writes:
>
> > * Vitaly Kuznetsov <[email protected]> wrote:
> >
> >> Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
> >> detect hypervisor's type in early boot. There is no point in having
> >> always-zero HYP: line on other hypervisors or bare metal.
> >>
> >> Suggested-by: Radim Krčmář <[email protected]>
> >> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> >> ---
> >> arch/x86/kernel/irq.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> >> index 88b36648..0c82064 100644
> >> --- a/arch/x86/kernel/irq.c
> >> +++ b/arch/x86/kernel/irq.c
> >> @@ -18,6 +18,7 @@
> >> #include <asm/mce.h>
> >> #include <asm/hw_irq.h>
> >> #include <asm/desc.h>
> >> +#include <asm/hypervisor.h>
> >>
> >> #define CREATE_TRACE_POINTS
> >> #include <asm/trace/irq_vectors.h>
> >> @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
> >> seq_puts(p, " Machine check polls\n");
> >> #endif
> >> #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
> >> - seq_printf(p, "%*s: ", prec, "HYP");
> >> - for_each_online_cpu(j)
> >> - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
> >> - seq_puts(p, " Hypervisor callback interrupts\n");
> >> + if (x86_hyper == &x86_hyper_ms_hyperv ||
> >> + x86_hyper == &x86_hyper_xen) {
> >> + seq_printf(p, "%*s: ", prec, "HYP");
> >> + for_each_online_cpu(j)
> >> + seq_printf(p, "%10u ",
> >> + irq_stats(j)->irq_hv_callback_count);
> >> + seq_puts(p, " Hypervisor callback interrupts\n");
> >> + }
> >
> > So I think we should simplify this to:
> >
> > if (x86_hyper) {
> > ...
> > }
> >
> > this will print the HYP line on hypervisors that don't use
> > HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
> > won't accidentally skip this line on hypervisors that start using the callback
> > IRQ.
>
> I was under an impression KVM and Vmware don't use these hypervisor
> callback irqs 'by desing' and even if one day they start using it
> someone will have to put inc_irq_stat() somewhere.
>
> If the goal is to avoid having x86_hyper_ms_hyperv/x86_hyper_xen in
> irq.c we can follow Andrew's suggestion and add a flag
> (use_callback_irqs?) to struct hypervisor_x86.
Yeah, a flag would be nicer, and that flag should also be used somewhere else -
for example in the IRQ vector init code or so?
I.e. make it only possible to use the callback vector if the flag is set properly
- or something like that. That way there will never be a mismatch between flag and
reality.
Thanks,
Ingo
Ingo Molnar <[email protected]> writes:
> * Vitaly Kuznetsov <[email protected]> wrote:
>
>> Ingo Molnar <[email protected]> writes:
>>
>> > * Vitaly Kuznetsov <[email protected]> wrote:
>> >
>> >> Hypervisor callback interrupts are only accounted on Xen/Hyper-V and we
>> >> detect hypervisor's type in early boot. There is no point in having
>> >> always-zero HYP: line on other hypervisors or bare metal.
>> >>
>> >> Suggested-by: Radim Krčmář <[email protected]>
>> >> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>> >> ---
>> >> arch/x86/kernel/irq.c | 13 +++++++++----
>> >> 1 file changed, 9 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> >> index 88b36648..0c82064 100644
>> >> --- a/arch/x86/kernel/irq.c
>> >> +++ b/arch/x86/kernel/irq.c
>> >> @@ -18,6 +18,7 @@
>> >> #include <asm/mce.h>
>> >> #include <asm/hw_irq.h>
>> >> #include <asm/desc.h>
>> >> +#include <asm/hypervisor.h>
>> >>
>> >> #define CREATE_TRACE_POINTS
>> >> #include <asm/trace/irq_vectors.h>
>> >> @@ -139,10 +140,14 @@ int arch_show_interrupts(struct seq_file *p, int prec)
>> >> seq_puts(p, " Machine check polls\n");
>> >> #endif
>> >> #if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
>> >> - seq_printf(p, "%*s: ", prec, "HYP");
>> >> - for_each_online_cpu(j)
>> >> - seq_printf(p, "%10u ", irq_stats(j)->irq_hv_callback_count);
>> >> - seq_puts(p, " Hypervisor callback interrupts\n");
>> >> + if (x86_hyper == &x86_hyper_ms_hyperv ||
>> >> + x86_hyper == &x86_hyper_xen) {
>> >> + seq_printf(p, "%*s: ", prec, "HYP");
>> >> + for_each_online_cpu(j)
>> >> + seq_printf(p, "%10u ",
>> >> + irq_stats(j)->irq_hv_callback_count);
>> >> + seq_puts(p, " Hypervisor callback interrupts\n");
>> >> + }
>> >
>> > So I think we should simplify this to:
>> >
>> > if (x86_hyper) {
>> > ...
>> > }
>> >
>> > this will print the HYP line on hypervisors that don't use
>> > HYPERVISOR_CALLBACK_VECTOR, but it will make it a lot more self-maintaining, we
>> > won't accidentally skip this line on hypervisors that start using the callback
>> > IRQ.
>>
>> I was under an impression KVM and Vmware don't use these hypervisor
>> callback irqs 'by desing' and even if one day they start using it
>> someone will have to put inc_irq_stat() somewhere.
>>
>> If the goal is to avoid having x86_hyper_ms_hyperv/x86_hyper_xen in
>> irq.c we can follow Andrew's suggestion and add a flag
>> (use_callback_irqs?) to struct hypervisor_x86.
>
> Yeah, a flag would be nicer, and that flag should also be used somewhere else -
> for example in the IRQ vector init code or so?
>
> I.e. make it only possible to use the callback vector if the flag is set properly
> - or something like that. That way there will never be a mismatch between flag and
> reality.
Actually we already have the flag :-) Both Xen and Hyper-V do
alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, ...) so we can simply do
test_bit(HYPERVISOR_CALLBACK_VECTOR, used_vectors) here. It feels a
little bit hackish but it checks what we need to check.
--
Vitaly