|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC 02/16] hack: drop GIC v3 support
On Wed, 28 Nov 2018 23:31:57 +0200
Andrii Anisov <andrii.anisov@xxxxxxxxx> wrote:
Hi,
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> This reduces some code and conditions in an IRQ processing path,
> and opens way to further code reduction.
While I understand that this is some sort of a hack, I am commenting
just on this patch to demonstrate some issues I see all over the series.
So please apply those ideas to the other patches as well if applicable.
> Also insert compilation errors into gicv3 code, because it would
> not compile or work with following patches.
>
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
> xen/arch/arm/configs/arm64_defconfig | 1 +
> xen/arch/arm/gic-v3-lpi.c | 2 ++
> xen/arch/arm/gic-v3.c | 2 ++
> xen/arch/arm/gic-vgic.c | 13 ++++++++++++-
> xen/arch/arm/gic.c | 6 ++++++
> xen/arch/arm/vgic-v3-its.c | 1 +
> xen/arch/arm/vgic.c | 20 ++++++++++++++++++++
> xen/arch/arm/vgic/vgic.c | 2 ++
> xen/include/asm-arm/irq.h | 2 ++
> xen/include/asm-arm/vgic.h | 17 +++++++++++------
> 10 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/configs/arm64_defconfig
> b/xen/arch/arm/configs/arm64_defconfig index e69de29..452c7ac 100644
> --- a/xen/arch/arm/configs/arm64_defconfig
> +++ b/xen/arch/arm/configs/arm64_defconfig
> @@ -0,0 +1 @@
> +# CONFIG_GICV3 is not set
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index e8c6e15..be64e17 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -32,6 +32,8 @@
> #include <asm/page.h>
> #include <asm/sysregs.h>
>
> +#error "The current gic/vgic/domain code does not support GICv3"
> +
I didn't check too thoroughly, but why do you need this? Your code
changes below seem to be guarded by #ifdef CONFIG_GICV3, so this file
(and the next) wouldn't even be build.
> /*
> * There could be a lot of LPIs on the host side, and they always go
> to
> * a guest. So having a struct irq_desc for each of them would be
> wasteful diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6fbc106..8e835b5 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -44,6 +44,8 @@
> #include <asm/io.h>
> #include <asm/sysregs.h>
>
> +#error "The current gic/vgic/domain code does not support GICv3"
> +
> /* Global state */
> static struct {
> void __iomem *map_dbase; /* Mapped address of distributor
> registers */ diff --git a/xen/arch/arm/gic-vgic.c
> b/xen/arch/arm/gic-vgic.c index 990399c..869987a 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -36,7 +36,9 @@ static inline void gic_set_lr(int lr, struct
> pending_irq *p, {
> ASSERT(!local_irq_is_enabled());
>
> +#ifdef CONFIG_GICV3
> clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
> +#endif
>
> gic_hw_ops->update_lr(lr, p->irq, p->priority,
> p->desc ? p->desc->irq : INVALID_IRQ,
> state); @@ -77,9 +79,11 @@ void gic_raise_inflight_irq(struct vcpu
> *v, unsigned int virtual_irq) {
> struct pending_irq *n = irq_to_pending(v, virtual_irq);
>
> +#ifdef CONFIG_GICV3
> /* If an LPI has been removed meanwhile, there is nothing left
> to raise. */ if ( unlikely(!n) )
> return;
> +#endif
So are you sure that this really saves you something? This check boils
down to a:
cbz x0, 670 <gic_raise_inflight_irq+0x88>
in assembly, which is basically free on modern CPUs.
Surprisingly removing this removes some more instructions from the
function, it would be interesting to know why.
In general I get the impression you optimise things just by looking at
the C code. Does saving a few instructions here and there really make a
difference? For instance I'd say that any non-memory instructions are
not worth to think about, and any stack accesses are probably caught in
the L1 cache.
In general I believe your performance drop is due to *exits* from the
guests due to the h/w interrupts, not necessarily because of the VGIC
code. So to reduce latency I think it would be more worthwhile to see
if we can reduce the world switch time, by avoiding unnessary
save/restores.
At least that saved a lot in KVM (with VHE), although the conditions
there are quite different.
> ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
> @@ -112,13 +116,14 @@ static unsigned int gic_find_unused_lr(struct
> vcpu *v, {
> unsigned int nr_lrs = gic_get_nr_lrs();
> unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
> - struct gic_lr lr_val;
>
> ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
> +#ifdef CONFIG_GICV3
I am not against removing GICv3 code from the code path if that is
not configured, but obviously those #ifdef's directly in the code are
hideous.
If you can prove that this saves you something, you could think
factoring out those functions, and define them as empty if CONFIG_GICV3
is not defined.
In this case you could do (outside of this function):
#ifdef CONFIG_GICV3
#define lpi_pristine_bit_set(p) \
test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &(p)->status)
#else
#define lpi_pristine_bit_set(p) false
#endif
That should make a fairly recent GCC remove the whole loop. Btw: Make
sure you use at *least* GCC 6, but preferably a newer version. They
improved quite a lot when it comes to dead code elimination.
> if ( unlikely(test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
> {
> unsigned int used_lr;
> + struct gic_lr lr_val;
>
> for_each_set_bit(used_lr, lr_mask, nr_lrs)
> {
> @@ -127,6 +132,7 @@ static unsigned int gic_find_unused_lr(struct
> vcpu *v, return used_lr;
> }
> }
> +#endif
>
> lr = find_next_zero_bit(lr_mask, nr_lrs, lr);
>
> @@ -142,9 +148,11 @@ void gic_raise_guest_irq(struct vcpu *v,
> unsigned int virtual_irq,
> ASSERT(spin_is_locked(&v->arch.vgic.lock));
>
> +#ifdef CONFIG_GICV3
> if ( unlikely(!p) )
> /* An unmapped LPI does not need to be raised. */
> return;
> +#endif
>
> if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
> {
> @@ -172,6 +180,8 @@ static void gic_update_one_lr(struct vcpu *v, int
> i) gic_hw_ops->read_lr(i, &lr_val);
> irq = lr_val.virq;
> p = irq_to_pending(v, irq);
> +
> +#ifdef CONFIG_GICV3
> /*
> * An LPI might have been unmapped, in which case we just clean
> up here.
> * If that LPI is marked as PRISTINE, the information in the LR
> is bogus, @@ -188,6 +198,7 @@ static void gic_update_one_lr(struct
> vcpu *v, int i)
> return;
> }
> +#endif
>
> if ( lr_val.active )
> {
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..77fc06f 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,7 +136,9 @@ int gic_route_irq_to_guest(struct domain *d,
> unsigned int virq, /* Caller has already checked that the IRQ is an
> SPI */ ASSERT(virq >= 32);
> ASSERT(virq < vgic_num_irqs(d));
> +#ifdef CONFIG_GICV3
> ASSERT(!is_lpi(virq));
> +#endif
Wait, did you actually benchmark a debug build?
ASSERTs define to nothing unless you have CONFIG_DEBUG=y. We insert
them excessively under this premise: Have seemingly obvious checks to
help catching bugs, they won't cost anything normally in production
builds.
> /*
> * When routing an IRQ to guest, the virtual state is not synced
> @@ -168,7 +170,9 @@ int gic_remove_irq_from_guest(struct domain *d,
> unsigned int virq,
> ASSERT(spin_is_locked(&desc->lock));
> ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> +#ifdef CONFIG_GICV3
> ASSERT(!is_lpi(virq));
> +#endif
>
> /*
> * Removing an interrupt while the domain is running may have
> @@ -391,6 +395,7 @@ void gic_interrupt(struct cpu_user_regs *regs,
> int is_fiq) do_IRQ(regs, irq, is_fiq);
> local_irq_disable();
> }
> +#ifdef CONFIG_GICV3
> else if ( is_lpi(irq) )
Similarly to what I wrote above, you could make sure that is_lpi()
always return false if GICv3 is not configured. GCC will then remove
any code using this function.
This would save you most of the chunks below and keep the code readable.
> {
> local_irq_enable();
> @@ -398,6 +403,7 @@ void gic_interrupt(struct cpu_user_regs *regs,
> int is_fiq) gic_hw_ops->do_LPI(irq);
> local_irq_disable();
> }
> +#endif
> else if ( unlikely(irq < 16) )
> {
> do_sgi(regs, irq);
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 5b73c4e..3c6693d 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -47,6 +47,7 @@
> #include <asm/vgic-emul.h>
> #include <asm/vreg.h>
>
> +#error "The current gic/vgic/domain code does not support GICv3"
> /*
> * Data structure to describe a virtual ITS.
> * If both the vcmd_lock and the its_lock are required, the
> vcmd_lock must diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f2608b0..a2419d0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -64,14 +64,18 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu
> *v, unsigned int irq)
> void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
> {
> +#ifdef CONFIG_GICV3
> /* The lpi_vcpu_id field must be big enough to hold a VCPU ID. */
> BUILD_BUG_ON(BIT(sizeof(p->lpi_vcpu_id) * 8) < MAX_VIRT_CPUS);
> +#endif
>
> memset(p, 0, sizeof(*p));
> INIT_LIST_HEAD(&p->inflight);
> INIT_LIST_HEAD(&p->lr_queue);
> p->irq = virq;
> +#ifdef CONFIG_GICV3
> p->lpi_vcpu_id = INVALID_VCPU_ID;
> +#endif
IIUC, vgic_init_pending_irq() is only called on domain/VGIC init (and on
mapping an LPI). So how does optimising this function help you with
your interrupt latency?
> }
>
> static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
> @@ -243,9 +247,11 @@ static int vgic_get_virq_priority(struct vcpu
> *v, unsigned int virq) {
> struct vgic_irq_rank *rank;
>
> +#ifdef CONFIG_GICV3
> /* LPIs don't have a rank, also store their priority separately.
> */ if ( is_lpi(virq) )
> return
> v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq);
> +#endif
> rank = vgic_rank_irq(v, virq);
> return ACCESS_ONCE(rank->priority[virq & INTERRUPT_RANK_MASK]);
> @@ -256,8 +262,10 @@ bool vgic_migrate_irq(struct vcpu *old, struct
> vcpu *new, unsigned int irq) unsigned long flags;
> struct pending_irq *p;
>
> +#ifdef CONFIG_GICV3
> /* This will never be called for an LPI, as we don't migrate
> them. */ ASSERT(!is_lpi(irq));
> +#endif
>
> spin_lock_irqsave(&old->arch.vgic.lock, flags);
>
> @@ -312,6 +320,7 @@ void arch_move_irqs(struct vcpu *v)
> struct vcpu *v_target;
> int i;
>
> +#ifdef CONFIG_GICV3
> /*
> * We don't migrate LPIs at the moment.
> * If we ever do, we must make sure that the struct pending_irq
> does @@ -322,6 +331,7 @@ void arch_move_irqs(struct vcpu *v)
> * don't participate.
> */
> ASSERT(!is_lpi(vgic_num_irqs(d) - 1));
> +#endif
>
> for ( i = 32; i < vgic_num_irqs(d); i++ )
> {
> @@ -343,8 +353,10 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t
> r, int n) int i = 0;
> struct vcpu *v_target;
>
> +#ifdef CONFIG_GICV3
> /* LPIs will never be disabled via this function. */
> ASSERT(!is_lpi(32 * n + 31));
> +#endif
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> @@ -393,8 +405,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
> r, int n) struct vcpu *v_target;
> struct domain *d = v->domain;
>
> +#ifdef CONFIG_GICV3
> /* LPIs will never be enabled via this function. */
> ASSERT(!is_lpi(32 * n + 31));
> +#endif
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> @@ -490,8 +504,10 @@ struct pending_irq *irq_to_pending(struct vcpu
> *v, unsigned int irq)
> * are used for SPIs; the rests are used for per cpu irqs */
> if ( irq < 32 )
> n = &v->arch.vgic.pending_irqs[irq];
> +#ifdef CONFIG_GICV3
> else if ( is_lpi(irq) )
> n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain,
> irq); +#endif
> else
> n = &v->domain->arch.vgic.pending_irqs[irq - 32];
> return n;
> @@ -550,12 +566,14 @@ void vgic_inject_irq(struct domain *d, struct
> vcpu *v, unsigned int virq, spin_lock_irqsave(&v->arch.vgic.lock,
> flags);
> n = irq_to_pending(v, virq);
> +#ifdef CONFIG_GICV3
> /* If an LPI has been removed, there is nothing to inject here.
> */ if ( unlikely(!n) )
> {
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> return;
> }
> +#endif
>
> /* vcpu offline */
> if ( test_bit(_VPF_down, &v->pause_flags) )
> @@ -607,8 +625,10 @@ bool vgic_evtchn_irq_pending(struct vcpu *v)
> struct pending_irq *p;
>
> p = irq_to_pending(v, v->domain->arch.evtchn_irq);
> +#ifdef CONFIG_GICV3
> /* Does not work for LPIs. */
> ASSERT(!is_lpi(v->domain->arch.evtchn_irq));
> +#endif
>
> return list_empty(&p->inflight);
> }
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index e2844dc..b8dbdaf 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -703,8 +703,10 @@ bool vgic_evtchn_irq_pending(struct vcpu *v)
> unsigned long flags;
> bool pending;
>
> +#ifdef CONFIG_GICV3
> /* Does not work for LPIs. */
> ASSERT(!is_lpi(v->domain->arch.evtchn_irq));
> +#endif
>
> irq = vgic_get_irq(v->domain, v, v->domain->arch.evtchn_irq);
> spin_lock_irqsave(&irq->irq_lock, flags);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index e45d574..4f1ef3c 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -63,10 +63,12 @@ struct irq_desc *__irq_to_desc(int irq);
>
> void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int
> is_fiq);
> +#ifdef CONFIG_GICV3
> static inline bool is_lpi(unsigned int irq)
> {
> return irq >= LPI_OFFSET;
> }
> +#endif
>
> #define domain_pirq_to_irq(d, pirq) (pirq)
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 447d24e..c5cb63f 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -63,27 +63,32 @@ struct pending_irq
> * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a
> different
> * vcpu while it is still inflight and on an GICH_LR register on
> the
> * old vcpu.
> - *
> - * GIC_IRQ_GUEST_PRISTINE_LPI: the IRQ is a newly mapped LPI,
> which
> - * has never been in an LR before. This means that any trace of
> an
> - * LPI with the same number in an LR must be from an older LPI,
> which
> - * has been unmapped before.
> - *
> */
> #define GIC_IRQ_GUEST_QUEUED 0
> #define GIC_IRQ_GUEST_ACTIVE 1
> #define GIC_IRQ_GUEST_VISIBLE 2
> #define GIC_IRQ_GUEST_ENABLED 3
> #define GIC_IRQ_GUEST_MIGRATING 4
> +#ifdef CONFIG_GICV3
> + /*
> + * GIC_IRQ_GUEST_PRISTINE_LPI: the IRQ is a newly mapped LPI,
> which
> + * has never been in an LR before. This means that any trace of
> an
> + * LPI with the same number in an LR must be from an older LPI,
> which
> + * has been unmapped before.
> + * Valid for GICV3 only.
> + */
> #define GIC_IRQ_GUEST_PRISTINE_LPI 5
> +#endif
> unsigned long status;
> struct irq_desc *desc; /* only set it the irq corresponds to a
> physical irq */ unsigned int irq;
> #define GIC_INVALID_LR (uint8_t)~0
> uint8_t lr;
> uint8_t priority;
> +#ifdef CONFIG_GICV3
> uint8_t lpi_priority; /* Caches the priority if this is an
> LPI. */ uint8_t lpi_vcpu_id; /* The VCPU for an LPI. */
> +#endif
Did you see any issues with the data structure being too big? I think we
spent some effort to make sure the size is reasonable.
Cheers,
Andre.
> /* inflight is used to append instances of pending_irq to
> * vgic.inflight_irqs */
> struct list_head inflight;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |