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.

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/81533a5d-8385-9872-ef22-4bcaa30c98f5%40siemens.com.

Reply via email to