Hi Jan,
> -----Original Message-----
> From: Jan Kiszka <[email protected]>
> Sent: 2019年12月3日 17:18
> To: Peng Fan <[email protected]>; [email protected]
> Cc: Alice Guo <[email protected]>
> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
>
> On 03.12.19 10:15, Peng Fan wrote:
> >> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in
> >> irqchip_inject_pending
> >>
> >> 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.
> >
> > Lock-free should be possible, let me work out a non-lock solution.
>
> This is what comes to my mind so far, but please re-check carefully:
>
> diff --git a/hypervisor/arch/arm-common/irqchip.c
> b/hypervisor/arch/arm-common/irqchip.c
> index 1c881b64..a83cd81d 100644
> --- a/hypervisor/arch/arm-common/irqchip.c
> +++ b/hypervisor/arch/arm-common/irqchip.c
> @@ -246,12 +246,12 @@ void irqchip_set_pending(struct public_per_cpu
> *cpu_public, u16 irq_id)
> if (new_tail != pending->head) {
> pending->irqs[pending->tail] = irq_id;
> pending->sender[pending->tail] = sender;
> - pending->tail = new_tail;
> /*
> - * Make the change to pending_irqs.tail visible before the
> - * caller sends SGI_INJECT.
> + * Make the entry content visible before updating the tail
> + * index.
> */
> memory_barrier();
> + pending->tail = new_tail;
The spin_unlock implies memory barrier. I think no need memory_barrier here.
> }
>
> spin_unlock(&pending->lock);
> @@ -264,6 +264,12 @@ void irqchip_set_pending(struct public_per_cpu
> *cpu_public, u16 irq_id)
> if (local_injection) {
> irqchip.enable_maint_irq(true);
> } else {
> + /*
> + * Make the change to pending_irqs.tail visible before the
> + * caller sends SGI_INJECT.
> + */
> + memory_barrier();
Not needed, see above, spin_unlock already done this.
> +
> sgi.targets = irqchip_get_cpu_target(cpu_public->cpu_id);
> sgi.cluster_id =
> irqchip_get_cluster_target(cpu_public->cpu_id);
> @@ -292,6 +298,12 @@ void irqchip_inject_pending(void)
> return;
> }
>
> + /*
> + * Ensure that the entry was read befor updating the head
> + * index.
> + */
> + memory_barrier();
No need. The index update will not be speculative before reading pending->head.
> +
> pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
Need a barrier here, to make update visible to producer.
> }
My version,
diff --git a/hypervisor/arch/arm-common/irqchip.c
b/hypervisor/arch/arm-common/irqchip.c
index 1c881b64..5abf1c37 100644
--- a/hypervisor/arch/arm-common/irqchip.c
+++ b/hypervisor/arch/arm-common/irqchip.c
@@ -247,13 +247,12 @@ void irqchip_set_pending(struct public_per_cpu
*cpu_public, u16 irq_id)
pending->irqs[pending->tail] = irq_id;
pending->sender[pending->tail] = sender;
pending->tail = new_tail;
- /*
- * Make the change to pending_irqs.tail visible before the
- * caller sends SGI_INJECT.
- */
- memory_barrier();
}
+ /*
+ * spin_unlock will make the change to pending_irqs.tail and
+ * entry content visible before the caller sends SGI_INJECT.
+ */
spin_unlock(&pending->lock);
/*
@@ -293,6 +292,9 @@ void irqchip_inject_pending(void)
}
pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
+
+ /* Make the change to pending->head visible to enqueue. */
+ memory_barrier();
}
/*
Thanks,
Peng.
>
>
> Thanks for bringing this up!
>
> 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/AM0PR04MB44813A2E63BF7C31B34C4E1C885C0%40AM0PR04MB4481.eurprd04.prod.outlook.com.
