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;
        }
 
        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();
+
                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();
+
                pending->head = (pending->head + 1) % MAX_PENDING_IRQS;
        }
 

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/46f71ed9-d4a2-d695-2ce5-307bfd13d1ca%40siemens.com.

Reply via email to