RE: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
>
> On 05.12.19 09:41, Peng Fan wrote:
> >> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in
> >> irqchip_inject_pending
> >>
> >> On 05.12.19 03:28, Peng Fan wrote:
> >>> Hi Jan,
> >>>
> >>>> -Original Message-
> >>>> From: Jan Kiszka
> >>>> Sent: 2019年12月3日 17:18
> >>>> To: Peng Fan ; jailhouse-dev@googlegroups.com
> >>>> Cc: Alice Guo
> >>>> 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
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> 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 =
> >> _cpu_public()->pending_irqs;
> >>>>>>>>> u16 irq_id, sender;
> >>>>>>>>>
> >>>>>>>>> + spin_lock(>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(>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(>lock);
> >>>>>>>>> +
> >>>>>>>>> /*
> >>>>>>>>> * The software interrupt queue is empty - turn off the
> >>>>>> maintenance
> >>>>>>>>> * interrupt.
> >>>>>>>>>
> >>>>>>>>
> >>>&g
Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
On 05.12.19 09:41, Peng Fan wrote:
>> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
>>
>> On 05.12.19 03:28, Peng Fan wrote:
>>> Hi Jan,
>>>
>>>> -Original Message-
>>>> From: Jan Kiszka
>>>> Sent: 2019年12月3日 17:18
>>>> To: Peng Fan ; jailhouse-dev@googlegroups.com
>>>> Cc: Alice Guo
>>>> 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
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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 =
>> _cpu_public()->pending_irqs;
>>>>>>>>> u16 irq_id, sender;
>>>>>>>>>
>>>>>>>>> + spin_lock(>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(>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(>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
>>&
RE: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
> Subject: Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
>
> On 05.12.19 03:28, Peng Fan wrote:
> > Hi Jan,
> >
> >> -Original Message-
> >> From: Jan Kiszka
> >> Sent: 2019年12月3日 17:18
> >> To: Peng Fan ; jailhouse-dev@googlegroups.com
> >> Cc: Alice Guo
> >> 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
> >>>>>>> ---
> >>>>>>>
> >>>>>>> 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 =
> _cpu_public()->pending_irqs;
> >>>>>>> u16 irq_id, sender;
> >>>>>>>
> >>>>>>> + spin_lock(>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(>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(>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,
> >
Re: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
On 05.12.19 03:28, Peng Fan wrote:
> Hi Jan,
>
>> -Original Message-
>> From: Jan Kiszka
>> Sent: 2019年12月3日 17:18
>> To: Peng Fan ; jailhouse-dev@googlegroups.com
>> Cc: Alice Guo
>> 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
>>>>>>> ---
>>>>>>>
>>>>>>> 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 = _cpu_public()->pending_irqs;
>>>>>>> u16 irq_id, sender;
>>>>>>>
>>>>>>> + spin_lock(>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(>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(>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 lac
RE: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
Hi Jan,
> -Original Message-
> From: Jan Kiszka
> Sent: 2019年12月3日 17:18
> To: Peng Fan ; jailhouse-dev@googlegroups.com
> Cc: Alice Guo
> 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
> >>>>> ---
> >>>>>
> >>>>> 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 = _cpu_public()->pending_irqs;
> >>>>> u16 irq_id, sender;
> >>>>>
> >>>>> + spin_lock(>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(>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(>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 mi
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
>>>>> ---
>>>>>
>>>>> 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 = _cpu_public()->pending_irqs;
>>>>> u16 irq_id, sender;
>>>>>
>>>>> + spin_lock(>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(>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(>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.
*/
memo
RE: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
> 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
> >>> ---
> >>>
> >>> 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 = _cpu_public()->pending_irqs;
> >>> u16 irq_id, sender;
> >>>
> >>> + spin_lock(>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(>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(>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.
Regards,
Peng.
>
> 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/AM0PR04MB44811BEC3EDE072313C1F3B888420%40AM0PR04MB4481.eurprd04.prod.outlook.com.
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
>>> ---
>>>
>>> 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 = _cpu_public()->pending_irqs;
>>> u16 irq_id, sender;
>>>
>>> + spin_lock(>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(>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(>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.
RE: [PATCH 1/2] arm: irqchip: add lock in irqchip_inject_pending
> 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
> > ---
> >
> > 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 = _cpu_public()->pending_irqs;
> > u16 irq_id, sender;
> >
> > + spin_lock(>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(>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(>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.
Thanks,
Peng.
>
> 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/AM0PR04MB4481FCD642715C2D03DA78EB88420%40AM0PR04MB4481.eurprd04.prod.outlook.com.
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
> ---
>
> 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 = _cpu_public()->pending_irqs;
> u16 irq_id, sender;
>
> + spin_lock(>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(>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(>lock);
> +
> /*
>* The software interrupt queue is empty - turn off the maintenance
>* interrupt.
>
Did you see real corruptions?
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.
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/3408fd87-22fa-31bc-657d-8661ad3998f3%40siemens.com.
