* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-06 14:17 [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c David Hildenbrand
@ 2018-02-06 14:29 ` Cornelia Huck
2018-02-07 18:28 ` Christian Borntraeger
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Cornelia Huck @ 2018-02-06 14:29 UTC (permalink / raw)
To: David Hildenbrand; +Cc: linux-s390, kvm, Christian Borntraeger, Janosch Frank
On Tue, 6 Feb 2018 15:17:43 +0100
David Hildenbrand <david@redhat.com> wrote:
> Just like for the interception handlers, let's also use a switch-case
> in our interrupt delivery code.
Amusingly, we did have a switch/case in the past :)
Nice that we can get rid of the I/O interrupt special casing.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/interrupt.c | 84 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 34 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-06 14:17 [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c David Hildenbrand
2018-02-06 14:29 ` Cornelia Huck
@ 2018-02-07 18:28 ` Christian Borntraeger
2018-02-08 8:14 ` Cornelia Huck
2018-02-08 8:23 ` Janosch Frank
2018-02-08 10:29 ` Christian Borntraeger
3 siblings, 1 reply; 9+ messages in thread
From: Christian Borntraeger @ 2018-02-07 18:28 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm; +Cc: Cornelia Huck, Janosch Frank
I see a minimal regression for uperf 1byte ping pong between two guests (~3%)
Probably because the old code first handled IO interrupts and then did the remaing
stuff. Not sure if its worth to keep the old io_ioirq hack.
Other than that this looks good.
On 02/06/2018 03:17 PM, David Hildenbrand wrote:
> Just like for the interception handlers, let's also use a switch-case
> in our interrupt delivery code.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/interrupt.c | 84 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index aabf46f5f883..3ea9cfa31b16 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -187,12 +187,6 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
> return kvm_s390_get_cpu_timer(vcpu) >> 63;
> }
>
> -static inline int is_ioirq(unsigned long irq_type)
> -{
> - return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
> - (irq_type <= IRQ_PEND_IO_ISC_0));
> -}
> -
> static uint64_t isc_to_isc_bits(int isc)
> {
> return (0x80 >> isc) << 24;
> @@ -1011,24 +1005,6 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
> return rc;
> }
>
> -typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> -
> -static const deliver_irq_t deliver_irq_funcs[] = {
> - [IRQ_PEND_MCHK_EX] = __deliver_machine_check,
> - [IRQ_PEND_MCHK_REP] = __deliver_machine_check,
> - [IRQ_PEND_PROG] = __deliver_prog,
> - [IRQ_PEND_EXT_EMERGENCY] = __deliver_emergency_signal,
> - [IRQ_PEND_EXT_EXTERNAL] = __deliver_external_call,
> - [IRQ_PEND_EXT_CLOCK_COMP] = __deliver_ckc,
> - [IRQ_PEND_EXT_CPU_TIMER] = __deliver_cpu_timer,
> - [IRQ_PEND_RESTART] = __deliver_restart,
> - [IRQ_PEND_SET_PREFIX] = __deliver_set_prefix,
> - [IRQ_PEND_PFAULT_INIT] = __deliver_pfault_init,
> - [IRQ_PEND_EXT_SERVICE] = __deliver_service,
> - [IRQ_PEND_PFAULT_DONE] = __deliver_pfault_done,
> - [IRQ_PEND_VIRTIO] = __deliver_virtio,
> -};
> -
> /* Check whether an external call is pending (deliverable or not) */
> int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
> {
> @@ -1192,7 +1168,6 @@ void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu)
> int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> - deliver_irq_t func;
> int rc = 0;
> unsigned long irq_type;
> unsigned long irqs;
> @@ -1212,16 +1187,57 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> while ((irqs = deliverable_irqs(vcpu)) && !rc) {
> /* bits are in the reverse order of interrupt priority */
> irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
> - if (is_ioirq(irq_type)) {
> + switch (irq_type) {
> + case IRQ_PEND_IO_ISC_0:
> + case IRQ_PEND_IO_ISC_1:
> + case IRQ_PEND_IO_ISC_2:
> + case IRQ_PEND_IO_ISC_3:
> + case IRQ_PEND_IO_ISC_4:
> + case IRQ_PEND_IO_ISC_5:
> + case IRQ_PEND_IO_ISC_6:
> + case IRQ_PEND_IO_ISC_7:
> rc = __deliver_io(vcpu, irq_type);
> - } else {
> - func = deliver_irq_funcs[irq_type];
> - if (!func) {
> - WARN_ON_ONCE(func == NULL);
> - clear_bit(irq_type, &li->pending_irqs);
> - continue;
> - }
> - rc = func(vcpu);
> + break;
> + case IRQ_PEND_MCHK_EX:
> + case IRQ_PEND_MCHK_REP:
> + rc = __deliver_machine_check(vcpu);
> + break;
> + case IRQ_PEND_PROG:
> + rc = __deliver_prog(vcpu);
> + break;
> + case IRQ_PEND_EXT_EMERGENCY:
> + rc = __deliver_emergency_signal(vcpu);
> + break;
> + case IRQ_PEND_EXT_EXTERNAL:
> + rc = __deliver_external_call(vcpu);
> + break;
> + case IRQ_PEND_EXT_CLOCK_COMP:
> + rc = __deliver_ckc(vcpu);
> + break;
> + case IRQ_PEND_EXT_CPU_TIMER:
> + rc = __deliver_cpu_timer(vcpu);
> + break;
> + case IRQ_PEND_RESTART:
> + rc = __deliver_restart(vcpu);
> + break;
> + case IRQ_PEND_SET_PREFIX:
> + rc = __deliver_set_prefix(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_INIT:
> + rc = __deliver_pfault_init(vcpu);
> + break;
> + case IRQ_PEND_EXT_SERVICE:
> + rc = __deliver_service(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_DONE:
> + rc = __deliver_pfault_done(vcpu);
> + break;
> + case IRQ_PEND_VIRTIO:
> + rc = __deliver_virtio(vcpu);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
> + clear_bit(irq_type, &li->pending_irqs);
> }
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-07 18:28 ` Christian Borntraeger
@ 2018-02-08 8:14 ` Cornelia Huck
2018-02-08 9:53 ` Christian Borntraeger
0 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2018-02-08 8:14 UTC (permalink / raw)
To: Christian Borntraeger; +Cc: David Hildenbrand, linux-s390, kvm, Janosch Frank
On Wed, 7 Feb 2018 19:28:04 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> I see a minimal regression for uperf 1byte ping pong between two guests (~3%)
> Probably because the old code first handled IO interrupts and then did the remaing
> stuff. Not sure if its worth to keep the old io_ioirq hack.
Hm, that confuses me a bit. We search the pending bit map, which should
give us the irq with the highest priority, and the switch/case still
starts out with I/O interrupts.
>
> Other than that this looks good.
>
>
> On 02/06/2018 03:17 PM, David Hildenbrand wrote:
> > Just like for the interception handlers, let's also use a switch-case
> > in our interrupt delivery code.
> >
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > arch/s390/kvm/interrupt.c | 84 ++++++++++++++++++++++++++++-------------------
> > 1 file changed, 50 insertions(+), 34 deletions(-)
> >
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index aabf46f5f883..3ea9cfa31b16 100644
> > --- a/arch/s390/kvm/interrupt.c
> > +++ b/arch/s390/kvm/interrupt.c
> > @@ -187,12 +187,6 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
> > return kvm_s390_get_cpu_timer(vcpu) >> 63;
> > }
> >
> > -static inline int is_ioirq(unsigned long irq_type)
> > -{
> > - return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
> > - (irq_type <= IRQ_PEND_IO_ISC_0));
> > -}
> > -
> > static uint64_t isc_to_isc_bits(int isc)
> > {
> > return (0x80 >> isc) << 24;
> > @@ -1011,24 +1005,6 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
> > return rc;
> > }
> >
> > -typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> > -
> > -static const deliver_irq_t deliver_irq_funcs[] = {
> > - [IRQ_PEND_MCHK_EX] = __deliver_machine_check,
> > - [IRQ_PEND_MCHK_REP] = __deliver_machine_check,
> > - [IRQ_PEND_PROG] = __deliver_prog,
> > - [IRQ_PEND_EXT_EMERGENCY] = __deliver_emergency_signal,
> > - [IRQ_PEND_EXT_EXTERNAL] = __deliver_external_call,
> > - [IRQ_PEND_EXT_CLOCK_COMP] = __deliver_ckc,
> > - [IRQ_PEND_EXT_CPU_TIMER] = __deliver_cpu_timer,
> > - [IRQ_PEND_RESTART] = __deliver_restart,
> > - [IRQ_PEND_SET_PREFIX] = __deliver_set_prefix,
> > - [IRQ_PEND_PFAULT_INIT] = __deliver_pfault_init,
> > - [IRQ_PEND_EXT_SERVICE] = __deliver_service,
> > - [IRQ_PEND_PFAULT_DONE] = __deliver_pfault_done,
> > - [IRQ_PEND_VIRTIO] = __deliver_virtio,
> > -};
> > -
> > /* Check whether an external call is pending (deliverable or not) */
> > int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
> > {
> > @@ -1192,7 +1168,6 @@ void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu)
> > int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> > {
> > struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> > - deliver_irq_t func;
> > int rc = 0;
> > unsigned long irq_type;
> > unsigned long irqs;
> > @@ -1212,16 +1187,57 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> > while ((irqs = deliverable_irqs(vcpu)) && !rc) {
> > /* bits are in the reverse order of interrupt priority */
> > irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
> > - if (is_ioirq(irq_type)) {
> > + switch (irq_type) {
> > + case IRQ_PEND_IO_ISC_0:
> > + case IRQ_PEND_IO_ISC_1:
> > + case IRQ_PEND_IO_ISC_2:
> > + case IRQ_PEND_IO_ISC_3:
> > + case IRQ_PEND_IO_ISC_4:
> > + case IRQ_PEND_IO_ISC_5:
> > + case IRQ_PEND_IO_ISC_6:
> > + case IRQ_PEND_IO_ISC_7:
> > rc = __deliver_io(vcpu, irq_type);
> > - } else {
> > - func = deliver_irq_funcs[irq_type];
> > - if (!func) {
> > - WARN_ON_ONCE(func == NULL);
> > - clear_bit(irq_type, &li->pending_irqs);
> > - continue;
> > - }
> > - rc = func(vcpu);
> > + break;
> > + case IRQ_PEND_MCHK_EX:
> > + case IRQ_PEND_MCHK_REP:
> > + rc = __deliver_machine_check(vcpu);
> > + break;
> > + case IRQ_PEND_PROG:
> > + rc = __deliver_prog(vcpu);
> > + break;
> > + case IRQ_PEND_EXT_EMERGENCY:
> > + rc = __deliver_emergency_signal(vcpu);
> > + break;
> > + case IRQ_PEND_EXT_EXTERNAL:
> > + rc = __deliver_external_call(vcpu);
> > + break;
> > + case IRQ_PEND_EXT_CLOCK_COMP:
> > + rc = __deliver_ckc(vcpu);
> > + break;
> > + case IRQ_PEND_EXT_CPU_TIMER:
> > + rc = __deliver_cpu_timer(vcpu);
> > + break;
> > + case IRQ_PEND_RESTART:
> > + rc = __deliver_restart(vcpu);
> > + break;
> > + case IRQ_PEND_SET_PREFIX:
> > + rc = __deliver_set_prefix(vcpu);
> > + break;
> > + case IRQ_PEND_PFAULT_INIT:
> > + rc = __deliver_pfault_init(vcpu);
> > + break;
> > + case IRQ_PEND_EXT_SERVICE:
> > + rc = __deliver_service(vcpu);
> > + break;
> > + case IRQ_PEND_PFAULT_DONE:
> > + rc = __deliver_pfault_done(vcpu);
> > + break;
> > + case IRQ_PEND_VIRTIO:
> > + rc = __deliver_virtio(vcpu);
> > + break;
> > + default:
> > + WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
> > + clear_bit(irq_type, &li->pending_irqs);
> > }
> > }
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-08 8:14 ` Cornelia Huck
@ 2018-02-08 9:53 ` Christian Borntraeger
2018-02-08 10:00 ` Cornelia Huck
0 siblings, 1 reply; 9+ messages in thread
From: Christian Borntraeger @ 2018-02-08 9:53 UTC (permalink / raw)
To: Cornelia Huck; +Cc: David Hildenbrand, linux-s390, kvm, Janosch Frank
On 02/08/2018 09:14 AM, Cornelia Huck wrote:
> On Wed, 7 Feb 2018 19:28:04 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> I see a minimal regression for uperf 1byte ping pong between two guests (~3%)
>> Probably because the old code first handled IO interrupts and then did the remaing
>> stuff. Not sure if its worth to keep the old io_ioirq hack.
>
> Hm, that confuses me a bit. We search the pending bit map, which should
> give us the irq with the highest priority, and the switch/case still
> starts out with I/O interrupts.
gcc does not obey the order of the case statements. It uses several heuristics depending
on the size and others. So gcc might fall back to jump tables for large switches, or
it uses bisecting or it might even split the search into a jump table and several
relative branches if there are strange distributions. Quite often the default
case is evaulated first.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-08 9:53 ` Christian Borntraeger
@ 2018-02-08 10:00 ` Cornelia Huck
2018-02-08 10:04 ` Christian Borntraeger
0 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2018-02-08 10:00 UTC (permalink / raw)
To: Christian Borntraeger; +Cc: David Hildenbrand, linux-s390, kvm, Janosch Frank
On Thu, 8 Feb 2018 10:53:00 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> On 02/08/2018 09:14 AM, Cornelia Huck wrote:
> > On Wed, 7 Feb 2018 19:28:04 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >
> >> I see a minimal regression for uperf 1byte ping pong between two guests (~3%)
> >> Probably because the old code first handled IO interrupts and then did the remaing
> >> stuff. Not sure if its worth to keep the old io_ioirq hack.
> >
> > Hm, that confuses me a bit. We search the pending bit map, which should
> > give us the irq with the highest priority, and the switch/case still
> > starts out with I/O interrupts.
>
> gcc does not obey the order of the case statements. It uses several heuristics depending
> on the size and others. So gcc might fall back to jump tables for large switches, or
> it uses bisecting or it might even split the search into a jump table and several
> relative branches if there are strange distributions. Quite often the default
> case is evaulated first.
But should we really try to optimize something that may change with a
different compiler anyway? The important thing is the priority in the
bitmap.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-08 10:00 ` Cornelia Huck
@ 2018-02-08 10:04 ` Christian Borntraeger
0 siblings, 0 replies; 9+ messages in thread
From: Christian Borntraeger @ 2018-02-08 10:04 UTC (permalink / raw)
To: Cornelia Huck; +Cc: David Hildenbrand, linux-s390, kvm, Janosch Frank
On 02/08/2018 11:00 AM, Cornelia Huck wrote:
> On Thu, 8 Feb 2018 10:53:00 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 02/08/2018 09:14 AM, Cornelia Huck wrote:
>>> On Wed, 7 Feb 2018 19:28:04 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>
>>>> I see a minimal regression for uperf 1byte ping pong between two guests (~3%)
>>>> Probably because the old code first handled IO interrupts and then did the remaing
>>>> stuff. Not sure if its worth to keep the old io_ioirq hack.
>>>
>>> Hm, that confuses me a bit. We search the pending bit map, which should
>>> give us the irq with the highest priority, and the switch/case still
>>> starts out with I/O interrupts.
>>
>> gcc does not obey the order of the case statements. It uses several heuristics depending
>> on the size and others. So gcc might fall back to jump tables for large switches, or
>> it uses bisecting or it might even split the search into a jump table and several
>> relative branches if there are strange distributions. Quite often the default
>> case is evaulated first.
>
> But should we really try to optimize something that may change with a
> different compiler anyway? The important thing is the priority in the
> bitmap.
An if before the switch would always prefer that condition. But I agree,we should
probably not go down this path of micro optimization.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-06 14:17 [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c David Hildenbrand
2018-02-06 14:29 ` Cornelia Huck
2018-02-07 18:28 ` Christian Borntraeger
@ 2018-02-08 8:23 ` Janosch Frank
2018-02-08 10:29 ` Christian Borntraeger
3 siblings, 0 replies; 9+ messages in thread
From: Janosch Frank @ 2018-02-08 8:23 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm; +Cc: Christian Borntraeger, Cornelia Huck
[-- Attachment #1.1: Type: text/plain, Size: 4444 bytes --]
On 06.02.2018 15:17, David Hildenbrand wrote:
> Just like for the interception handlers, let's also use a switch-case
> in our interrupt delivery code.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
> arch/s390/kvm/interrupt.c | 84 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index aabf46f5f883..3ea9cfa31b16 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -187,12 +187,6 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
> return kvm_s390_get_cpu_timer(vcpu) >> 63;
> }
>
> -static inline int is_ioirq(unsigned long irq_type)
> -{
> - return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
> - (irq_type <= IRQ_PEND_IO_ISC_0));
> -}
> -
> static uint64_t isc_to_isc_bits(int isc)
> {
> return (0x80 >> isc) << 24;
> @@ -1011,24 +1005,6 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
> return rc;
> }
>
> -typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> -
> -static const deliver_irq_t deliver_irq_funcs[] = {
> - [IRQ_PEND_MCHK_EX] = __deliver_machine_check,
> - [IRQ_PEND_MCHK_REP] = __deliver_machine_check,
> - [IRQ_PEND_PROG] = __deliver_prog,
> - [IRQ_PEND_EXT_EMERGENCY] = __deliver_emergency_signal,
> - [IRQ_PEND_EXT_EXTERNAL] = __deliver_external_call,
> - [IRQ_PEND_EXT_CLOCK_COMP] = __deliver_ckc,
> - [IRQ_PEND_EXT_CPU_TIMER] = __deliver_cpu_timer,
> - [IRQ_PEND_RESTART] = __deliver_restart,
> - [IRQ_PEND_SET_PREFIX] = __deliver_set_prefix,
> - [IRQ_PEND_PFAULT_INIT] = __deliver_pfault_init,
> - [IRQ_PEND_EXT_SERVICE] = __deliver_service,
> - [IRQ_PEND_PFAULT_DONE] = __deliver_pfault_done,
> - [IRQ_PEND_VIRTIO] = __deliver_virtio,
> -};
> -
> /* Check whether an external call is pending (deliverable or not) */
> int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
> {
> @@ -1192,7 +1168,6 @@ void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu)
> int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> - deliver_irq_t func;
> int rc = 0;
> unsigned long irq_type;
> unsigned long irqs;
> @@ -1212,16 +1187,57 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> while ((irqs = deliverable_irqs(vcpu)) && !rc) {
> /* bits are in the reverse order of interrupt priority */
> irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
> - if (is_ioirq(irq_type)) {
> + switch (irq_type) {
> + case IRQ_PEND_IO_ISC_0:
> + case IRQ_PEND_IO_ISC_1:
> + case IRQ_PEND_IO_ISC_2:
> + case IRQ_PEND_IO_ISC_3:
> + case IRQ_PEND_IO_ISC_4:
> + case IRQ_PEND_IO_ISC_5:
> + case IRQ_PEND_IO_ISC_6:
> + case IRQ_PEND_IO_ISC_7:
> rc = __deliver_io(vcpu, irq_type);
> - } else {
> - func = deliver_irq_funcs[irq_type];
> - if (!func) {
> - WARN_ON_ONCE(func == NULL);
> - clear_bit(irq_type, &li->pending_irqs);
> - continue;
> - }
> - rc = func(vcpu);
> + break;
> + case IRQ_PEND_MCHK_EX:
> + case IRQ_PEND_MCHK_REP:
> + rc = __deliver_machine_check(vcpu);
> + break;
> + case IRQ_PEND_PROG:
> + rc = __deliver_prog(vcpu);
> + break;
> + case IRQ_PEND_EXT_EMERGENCY:
> + rc = __deliver_emergency_signal(vcpu);
> + break;
> + case IRQ_PEND_EXT_EXTERNAL:
> + rc = __deliver_external_call(vcpu);
> + break;
> + case IRQ_PEND_EXT_CLOCK_COMP:
> + rc = __deliver_ckc(vcpu);
> + break;
> + case IRQ_PEND_EXT_CPU_TIMER:
> + rc = __deliver_cpu_timer(vcpu);
> + break;
> + case IRQ_PEND_RESTART:
> + rc = __deliver_restart(vcpu);
> + break;
> + case IRQ_PEND_SET_PREFIX:
> + rc = __deliver_set_prefix(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_INIT:
> + rc = __deliver_pfault_init(vcpu);
> + break;
> + case IRQ_PEND_EXT_SERVICE:
> + rc = __deliver_service(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_DONE:
> + rc = __deliver_pfault_done(vcpu);
> + break;
> + case IRQ_PEND_VIRTIO:
> + rc = __deliver_virtio(vcpu);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
> + clear_bit(irq_type, &li->pending_irqs);
> }
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c
2018-02-06 14:17 [PATCH v1] KVM: s390: use switch vs jump table in interrupt.c David Hildenbrand
` (2 preceding siblings ...)
2018-02-08 8:23 ` Janosch Frank
@ 2018-02-08 10:29 ` Christian Borntraeger
3 siblings, 0 replies; 9+ messages in thread
From: Christian Borntraeger @ 2018-02-08 10:29 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm; +Cc: Cornelia Huck, Janosch Frank
On 02/06/2018 03:17 PM, David Hildenbrand wrote:
> Just like for the interception handlers, let's also use a switch-case
> in our interrupt delivery code.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Thanks applied.
> ---
> arch/s390/kvm/interrupt.c | 84 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index aabf46f5f883..3ea9cfa31b16 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -187,12 +187,6 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
> return kvm_s390_get_cpu_timer(vcpu) >> 63;
> }
>
> -static inline int is_ioirq(unsigned long irq_type)
> -{
> - return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
> - (irq_type <= IRQ_PEND_IO_ISC_0));
> -}
> -
> static uint64_t isc_to_isc_bits(int isc)
> {
> return (0x80 >> isc) << 24;
> @@ -1011,24 +1005,6 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
> return rc;
> }
>
> -typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> -
> -static const deliver_irq_t deliver_irq_funcs[] = {
> - [IRQ_PEND_MCHK_EX] = __deliver_machine_check,
> - [IRQ_PEND_MCHK_REP] = __deliver_machine_check,
> - [IRQ_PEND_PROG] = __deliver_prog,
> - [IRQ_PEND_EXT_EMERGENCY] = __deliver_emergency_signal,
> - [IRQ_PEND_EXT_EXTERNAL] = __deliver_external_call,
> - [IRQ_PEND_EXT_CLOCK_COMP] = __deliver_ckc,
> - [IRQ_PEND_EXT_CPU_TIMER] = __deliver_cpu_timer,
> - [IRQ_PEND_RESTART] = __deliver_restart,
> - [IRQ_PEND_SET_PREFIX] = __deliver_set_prefix,
> - [IRQ_PEND_PFAULT_INIT] = __deliver_pfault_init,
> - [IRQ_PEND_EXT_SERVICE] = __deliver_service,
> - [IRQ_PEND_PFAULT_DONE] = __deliver_pfault_done,
> - [IRQ_PEND_VIRTIO] = __deliver_virtio,
> -};
> -
> /* Check whether an external call is pending (deliverable or not) */
> int kvm_s390_ext_call_pending(struct kvm_vcpu *vcpu)
> {
> @@ -1192,7 +1168,6 @@ void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu)
> int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> - deliver_irq_t func;
> int rc = 0;
> unsigned long irq_type;
> unsigned long irqs;
> @@ -1212,16 +1187,57 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> while ((irqs = deliverable_irqs(vcpu)) && !rc) {
> /* bits are in the reverse order of interrupt priority */
> irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
> - if (is_ioirq(irq_type)) {
> + switch (irq_type) {
> + case IRQ_PEND_IO_ISC_0:
> + case IRQ_PEND_IO_ISC_1:
> + case IRQ_PEND_IO_ISC_2:
> + case IRQ_PEND_IO_ISC_3:
> + case IRQ_PEND_IO_ISC_4:
> + case IRQ_PEND_IO_ISC_5:
> + case IRQ_PEND_IO_ISC_6:
> + case IRQ_PEND_IO_ISC_7:
> rc = __deliver_io(vcpu, irq_type);
> - } else {
> - func = deliver_irq_funcs[irq_type];
> - if (!func) {
> - WARN_ON_ONCE(func == NULL);
> - clear_bit(irq_type, &li->pending_irqs);
> - continue;
> - }
> - rc = func(vcpu);
> + break;
> + case IRQ_PEND_MCHK_EX:
> + case IRQ_PEND_MCHK_REP:
> + rc = __deliver_machine_check(vcpu);
> + break;
> + case IRQ_PEND_PROG:
> + rc = __deliver_prog(vcpu);
> + break;
> + case IRQ_PEND_EXT_EMERGENCY:
> + rc = __deliver_emergency_signal(vcpu);
> + break;
> + case IRQ_PEND_EXT_EXTERNAL:
> + rc = __deliver_external_call(vcpu);
> + break;
> + case IRQ_PEND_EXT_CLOCK_COMP:
> + rc = __deliver_ckc(vcpu);
> + break;
> + case IRQ_PEND_EXT_CPU_TIMER:
> + rc = __deliver_cpu_timer(vcpu);
> + break;
> + case IRQ_PEND_RESTART:
> + rc = __deliver_restart(vcpu);
> + break;
> + case IRQ_PEND_SET_PREFIX:
> + rc = __deliver_set_prefix(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_INIT:
> + rc = __deliver_pfault_init(vcpu);
> + break;
> + case IRQ_PEND_EXT_SERVICE:
> + rc = __deliver_service(vcpu);
> + break;
> + case IRQ_PEND_PFAULT_DONE:
> + rc = __deliver_pfault_done(vcpu);
> + break;
> + case IRQ_PEND_VIRTIO:
> + rc = __deliver_virtio(vcpu);
> + break;
> + default:
> + WARN_ONCE(1, "Unknown pending irq type %ld", irq_type);
> + clear_bit(irq_type, &li->pending_irqs);
> }
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread