Bruce Ashfield
2024-07-12 03:00
I'd still like more explanation on this change and if it was tested in a
heavily loaded system (and for how long). Looking at the spinlock change and
the comments immediately below the patch, we have a write memory barrier AND a
mention of ipis. When a rawlock is taken, we need to understand what happens
between the take and release. I'm seeing a lot of retry logic / looping, etc.
If that lock is held for too long or there's some way that the current thread
might be suspended, we could easily end up with high latency or a system
deadlock.
The frequency of IPI loss is low. After half an hour of stress-ng testing, I
captured
only one instance of IPI retransmission.
This mechanism detects and resends IPIs, and despite the looping code, it does
not cause significant latency or deadlock.
Thanks,
Ruiqiang
________________________________
From: Bruce Ashfield <bruce.ashfi...@gmail.com>
Sent: Friday, July 12, 2024 03:00
To: Hao, Ruiqiang <ruiqiang....@windriver.com>
Cc: linux-yocto@lists.yoctoproject.org <linux-yocto@lists.yoctoproject.org>
Subject: Re: [PATCH] irqchip/gic-v3: change gic_ipi_track::lock type
CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know
the content is safe.
In message: [PATCH] irqchip/gic-v3: change gic_ipi_track::lock type
on 10/07/2024 Ruiqiang Hao wrote:
> From: Ruiqiang Hao <ruiqiang....@windriver.com>
>
> When start with preempt-rt kernel, the following problem occurs.
> Change the type of gic_ipi_track::lock from spinlock_t to raw_spinlock_t
> to fix this.
>
> BUG: sleeping function called from invalid context at
> kernel/locking/spinlock_rt.c:46
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/15
> preempt_count: 6, expected: 0
> RCU nest depth: 0, expected: 0
> Preemption disabled at:
> [<ffff800008246370>] __create_object+0x110/0x380
> CPU: 15 PID: 0 Comm: swapper/15 Tainted: G W
> 6.1.90-rt18-yocto-preempt-rt #1
> Hardware name: Marvell OcteonTX CN96XX board (DT)
> Call trace:
> dump_backtrace.part.0+0xe8/0xf4
> show_stack+0x20/0x30
> dump_stack_lvl+0x64/0x80
> dump_stack+0x18/0x34
> __might_resched+0x160/0x1c0
> rt_spin_lock+0x38/0xd0
> gic_write_sgi1r_retry+0x58/0x120
> gic_ipi_send_mask+0x148/0x184
> __ipi_send_mask+0x34/0x11c
> smp_cross_call+0x4c/0x10c
> arch_irq_work_raise+0x3c/0x4c
> __irq_work_queue_local+0x9c/0xb0
> irq_work_queue+0x48/0x80
> sugov_update_shared+0x258/0x260
> enqueue_top_rt_rq+0x7c/0x124
> enqueue_task_rt+0x1cc/0x2ec
> ttwu_do_activate+0x84/0x170
> try_to_wake_up+0x21c/0x5c0
> wake_up_process+0x20/0x30
> irq_exit_rcu+0x134/0x140
> el1_interrupt+0x38/0x70
> el1h_64_irq_handler+0x18/0x2c
> el1h_64_irq+0x64/0x68
> arch_cpu_idle+0x18/0x2c
> default_idle_call+0x40/0x1d0
> do_idle+0x230/0x2a0
> cpu_startup_entry+0x3c/0x4c
> secondary_start_kernel+0x120/0x14c
> __secondary_switched+0xb0/0xb4
>
> Signed-off-by: Ruiqiang Hao <ruiqiang....@windriver.com>
> ---
> drivers/irqchip/irq-gic-v3-fixes.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-fixes.c
> b/drivers/irqchip/irq-gic-v3-fixes.c
> index 7d88e3295686..62d41cb4e9a4 100644
> --- a/drivers/irqchip/irq-gic-v3-fixes.c
> +++ b/drivers/irqchip/irq-gic-v3-fixes.c
> @@ -31,7 +31,7 @@ enum ipi_msg_type {
> struct gic_ipi_track {
> atomic_t tx_count;
> atomic_t rx_count;
> - spinlock_t lock;
> + raw_spinlock_t lock;
> };
>
> static struct gic_ipi_track gic_ipitrack[NR_CPUS][NR_IPIS];
> @@ -79,7 +79,7 @@ void gic_write_sgi1r_retry(int dest_cpu, int irq, u64 val)
> {
> unsigned long flags;
>
> - spin_lock_irqsave(&gic_ipitrack[dest_cpu][irq].lock, flags);
> + raw_spin_lock_irqsave(&gic_ipitrack[dest_cpu][irq].lock, flags);
Because this is only destined for a BSP branch, I'm not overly
concerned about the changes.
BUT
I'd still like more explanation on this change and if it was
tested in a heavily loaded system (and for how long).
Looking at the spinlock change and the comments immediately
below the patch, we have a write memory barrier AND a mention
of ipis.
When a rawlock is taken, we need to understand what happens
between the take and release. I'm seeing a lot of retry
logic / looping, etc.
If that lock is held for too long or there's some way that
the current thread might be suspended, we could easily
end up with high latency or a system deadlock.
Bruce
> wmb(); /* Ensure lock is acquired before we generate an IPI */
> retry:
> gic_write_sgi1r(val);
> @@ -95,7 +95,7 @@ void gic_write_sgi1r_retry(int dest_cpu, int irq, u64 val)
> wmb(); /* Ensure the write is completed before we start again */
> goto retry;
> out:
> - spin_unlock_irqrestore(&gic_ipitrack[dest_cpu][irq].lock, flags);
> + raw_spin_unlock_irqrestore(&gic_ipitrack[dest_cpu][irq].lock, flags);
> }
>
> static bool __maybe_unused gicv3_enable_quirk_otx(void *data)
> @@ -107,7 +107,7 @@ static bool __maybe_unused gicv3_enable_quirk_otx(void
> *data)
> /* Initialize necessary lock */
> for_each_possible_cpu(cpu)
> for (ipi = 0; ipi < NR_IPIS; ipi++)
> - spin_lock_init(&gic_ipitrack[cpu][ipi].lock);
> + raw_spin_lock_init(&gic_ipitrack[cpu][ipi].lock);
>
> return true;
> }
> --
> 2.45.0
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#14176):
https://lists.yoctoproject.org/g/linux-yocto/message/14176
Mute This Topic: https://lists.yoctoproject.org/mt/107139152/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
