On 03.12.19 09:58, Peng Fan wrote: >> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending >> >> On 03.12.19 09:27, Peng Fan wrote: >>> Thinking about core0 is inject SGI to core1, core1 is handling SGI >>> interrupt. >>> >>> That means core0 might be in path to enqueue SGI into the pending_irqs >>> array, core1 might be in path handling SGI and pick one from >>> pending_irqs array. So need to use lock to protect unqueue, not only >>> enqueue. >>> >>> Signed-off-by: Peng Fan <[email protected]> >>> --- >>> >>> V1: >>> The best case is only lock one entry, so no good solution, because >>> there is possibility that inject fail. >>> >>> hypervisor/arch/arm-common/irqchip.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/hypervisor/arch/arm-common/irqchip.c >>> b/hypervisor/arch/arm-common/irqchip.c >>> index 1c881b64..fbaa3099 100644 >>> --- a/hypervisor/arch/arm-common/irqchip.c >>> +++ b/hypervisor/arch/arm-common/irqchip.c >>> @@ -279,11 +279,14 @@ void irqchip_inject_pending(void) >>> struct pending_irqs *pending = &this_cpu_public()->pending_irqs; >>> u16 irq_id, sender; >>> >>> + spin_lock(&pending->lock); >>> + >>> while (pending->head != pending->tail) { >>> irq_id = pending->irqs[pending->head]; >>> sender = pending->sender[pending->head]; >>> >>> if (irqchip.inject_irq(irq_id, sender) == -EBUSY) { >>> + spin_unlock(&pending->lock); >>> /* >>> * The list registers are full, trigger maintenance >>> * interrupt and leave. >>> @@ -295,6 +298,8 @@ void irqchip_inject_pending(void) >>> pending->head = (pending->head + 1) % MAX_PENDING_IRQS; >>> } >>> >>> + spin_unlock(&pending->lock); >>> + >>> /* >>> * The software interrupt queue is empty - turn off the maintenance >>> * interrupt. >>> >> >> Did you see real corruptions? > > No. just code inspection currently. We met some SGI inject return -EBUSY, > so go through the code, and think this piece code needs a lock. > >> >> The idea behind this was that the injection to the lock-less queue happens in >> a way that it won't make changes visible to the consumer that are >> inconsistent, hence the barrier in irqchip_set_pending. Looking that again, >> though, we possibly need another barrier, right before updating >> pending->tail. > > Barrier could not prohibit enqueue/unqueue contention. > enqueue will check head, unqueue will update head.
Some topic as with lockless enqueuing: We need a barrier to shield the readout of the head entry from the update of pending->head. So, we are definitely lacking barriers here, but I don't think we need the spinlock between producer and consumer because there is only one consumer. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/81533a5d-8385-9872-ef22-4bcaa30c98f5%40siemens.com.
