Hi Jan,

> -----Original Message-----
> From: Jan Kiszka <jan.kis...@siemens.com>
> Sent: 2019年12月3日 17:18
> To: Peng Fan <peng....@nxp.com>; jailhouse-dev@googlegroups.com
> Cc: Alice Guo <alice....@nxp.com>
> 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 <peng....@nxp.com>
> >>>>> ---
> >>>>>
> >>>>> 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 jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/AM0PR04MB44813A2E63BF7C31B34C4E1C885C0%40AM0PR04MB4481.eurprd04.prod.outlook.com.

Reply via email to