* [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
@ 2025-03-24 17:18 Jan Kiszka
2025-03-24 18:58 ` Florian Bezdeka
2025-03-24 22:52 ` Florian Bezdeka
0 siblings, 2 replies; 9+ messages in thread
From: Jan Kiszka @ 2025-03-24 17:18 UTC (permalink / raw)
To: Philippe Gerum, Florian Bezdeka, Xenomai
From: Jan Kiszka <jan.kiszka@siemens.com>
Code and comment were no in line, but the comment actually make more
sense here, given that unlock_vector_lock will unconditionally reenable
hard IRQs.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
arch/x86/kernel/apic/vector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index a08b0dff066ac..af60f138d590b 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -142,7 +142,7 @@ void lock_vector_lock(void)
* entry. In addition, we assume that hard irqs are on as well
* (which is the regular case).
*/
- WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
+ WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
hard_cond_local_irq_disable();
/* Used to the online set of cpus does not change
* during assign_irq_vector.
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-24 17:18 [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock Jan Kiszka
@ 2025-03-24 18:58 ` Florian Bezdeka
2025-03-24 22:52 ` Florian Bezdeka
1 sibling, 0 replies; 9+ messages in thread
From: Florian Bezdeka @ 2025-03-24 18:58 UTC (permalink / raw)
To: Jan Kiszka, Philippe Gerum, Xenomai
On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Code and comment were no in line, but the comment actually make more
^^ not ^^ makes
> sense here, given that unlock_vector_lock will unconditionally reenable
> hard IRQs.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> arch/x86/kernel/apic/vector.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index a08b0dff066ac..af60f138d590b 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -142,7 +142,7 @@ void lock_vector_lock(void)
> * entry. In addition, we assume that hard irqs are on as well
> * (which is the regular case).
> */
> - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
> + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
> hard_cond_local_irq_disable();
> /* Used to the online set of cpus does not change
> * during assign_irq_vector.
> --
> 2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-24 17:18 [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock Jan Kiszka
2025-03-24 18:58 ` Florian Bezdeka
@ 2025-03-24 22:52 ` Florian Bezdeka
2025-03-25 9:50 ` Philippe Gerum
1 sibling, 1 reply; 9+ messages in thread
From: Florian Bezdeka @ 2025-03-24 22:52 UTC (permalink / raw)
To: Jan Kiszka, Philippe Gerum, Xenomai
On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Code and comment were no in line, but the comment actually make more
> sense here, given that unlock_vector_lock will unconditionally reenable
> hard IRQs.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> arch/x86/kernel/apic/vector.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index a08b0dff066ac..af60f138d590b 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -142,7 +142,7 @@ void lock_vector_lock(void)
> * entry. In addition, we assume that hard irqs are on as well
> * (which is the regular case).
> */
> - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
> + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
> hard_cond_local_irq_disable();
> /* Used to the online set of cpus does not change
> * during assign_irq_vector.
> --
> 2.43.0
With that applied there is still one warning left in 6.14:
[ 17.921496] ------------[ cut here ]------------
[ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
[ 17.921512] Modules linked in:
[ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
[ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 17.921523] IRQ stage: Linux
[ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
[ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
[ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
[ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
[ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
[ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
[ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
[ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
[ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
[ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
[ 17.921579] Call Trace:
[ 17.921622] <TASK>
[ 17.921625] ? __warn+0x85/0x140
[ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
[ 17.921634] ? report_bug+0x164/0x190
[ 17.921641] ? handle_bug+0x65/0xd0
[ 17.921645] ? exc_invalid_op+0xb2/0xd0
[ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
[ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
[ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
[ 17.921661] sched_cpu_wait_empty+0x93/0xc0
[ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
[ 17.921669] cpuhp_thread_fun+0xee/0x190
[ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
[ 17.921678] smpboot_thread_fn+0xdd/0x1d0
[ 17.921683] kthread+0xf1/0x1e0
[ 17.921689] ? __pfx_kthread+0x10/0x10
[ 17.921693] ret_from_fork+0x34/0x50
[ 17.921698] ? __pfx_kthread+0x10/0x10
[ 17.921701] ret_from_fork_asm+0x1a/0x30
[ 17.921717] </TASK>
[ 17.921719] ---[ end trace 0000000000000000 ]---
There were some changes to the scheduler code in between. I'm currently
testing with:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 50b082b41e7d..4427fc13086d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
if (mm != &init_mm) {
mmgrab_lazy_tlb(&init_mm);
- local_irq_disable();
+ hard_local_irq_disable();
current->active_mm = &init_mm;
switch_mm_irqs_off(mm, &init_mm, current);
- local_irq_enable();
+ hard_local_irq_enable();
finish_arch_post_lock_switch();
mmdrop_lazy_tlb(mm);
}
Florian
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-24 22:52 ` Florian Bezdeka
@ 2025-03-25 9:50 ` Philippe Gerum
2025-03-26 12:13 ` Florian Bezdeka
2025-03-26 12:14 ` Florian Bezdeka
0 siblings, 2 replies; 9+ messages in thread
From: Philippe Gerum @ 2025-03-25 9:50 UTC (permalink / raw)
To: Florian Bezdeka; +Cc: Jan Kiszka, Xenomai
Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Code and comment were no in line, but the comment actually make more
>> sense here, given that unlock_vector_lock will unconditionally reenable
>> hard IRQs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> arch/x86/kernel/apic/vector.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index a08b0dff066ac..af60f138d590b 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -142,7 +142,7 @@ void lock_vector_lock(void)
>> * entry. In addition, we assume that hard irqs are on as well
>> * (which is the regular case).
>> */
>> - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
>> + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
>> hard_cond_local_irq_disable();
>> /* Used to the online set of cpus does not change
>> * during assign_irq_vector.
>> --
>> 2.43.0
>
> With that applied there is still one warning left in 6.14:
>
> [ 17.921496] ------------[ cut here ]------------
> [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
> [ 17.921512] Modules linked in:
> [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
> [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 17.921523] IRQ stage: Linux
> [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
> [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
> [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
> [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
> [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
> [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
> [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
> [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
> [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
> [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
> [ 17.921579] Call Trace:
> [ 17.921622] <TASK>
> [ 17.921625] ? __warn+0x85/0x140
> [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
> [ 17.921634] ? report_bug+0x164/0x190
> [ 17.921641] ? handle_bug+0x65/0xd0
> [ 17.921645] ? exc_invalid_op+0xb2/0xd0
> [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
> [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
> [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
> [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
> [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
> [ 17.921669] cpuhp_thread_fun+0xee/0x190
> [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
> [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
> [ 17.921683] kthread+0xf1/0x1e0
> [ 17.921689] ? __pfx_kthread+0x10/0x10
> [ 17.921693] ret_from_fork+0x34/0x50
> [ 17.921698] ? __pfx_kthread+0x10/0x10
> [ 17.921701] ret_from_fork_asm+0x1a/0x30
> [ 17.921717] </TASK>
> [ 17.921719] ---[ end trace 0000000000000000 ]---
>
> There were some changes to the scheduler code in between. I'm currently
> testing with:
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 50b082b41e7d..4427fc13086d 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
>
> if (mm != &init_mm) {
> mmgrab_lazy_tlb(&init_mm);
> - local_irq_disable();
> + hard_local_irq_disable();
> current->active_mm = &init_mm;
> switch_mm_irqs_off(mm, &init_mm, current);
> - local_irq_enable();
> + hard_local_irq_enable();
> finish_arch_post_lock_switch();
> mmdrop_lazy_tlb(mm);
> }
>
> Florian
Correct. However, since this is an inband call, we want to keep the
virtual interrupt state in line with the expectations of the kernel, so
we need to keep the original local_irq* calls in place. Dovetail
provides a pair of calls namely (un)protect_inband_mm() which is
dedicated to guarding the mm switch code from hardware interrupts, the
general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
these calls in this particular case serves as code annotation as well.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-25 9:50 ` Philippe Gerum
@ 2025-03-26 12:13 ` Florian Bezdeka
2025-03-26 12:14 ` Florian Bezdeka
1 sibling, 0 replies; 9+ messages in thread
From: Florian Bezdeka @ 2025-03-26 12:13 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, Xenomai
On Tue, 2025-03-25 at 10:50 +0100, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>
> > On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
> > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > >
> > > Code and comment were no in line, but the comment actually make more
> > > sense here, given that unlock_vector_lock will unconditionally reenable
> > > hard IRQs.
> > >
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > > arch/x86/kernel/apic/vector.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> > > index a08b0dff066ac..af60f138d590b 100644
> > > --- a/arch/x86/kernel/apic/vector.c
> > > +++ b/arch/x86/kernel/apic/vector.c
> > > @@ -142,7 +142,7 @@ void lock_vector_lock(void)
> > > * entry. In addition, we assume that hard irqs are on as well
> > > * (which is the regular case).
> > > */
> > > - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
> > > + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
> > > hard_cond_local_irq_disable();
> > > /* Used to the online set of cpus does not change
> > > * during assign_irq_vector.
> > > --
> > > 2.43.0
> >
> > With that applied there is still one warning left in 6.14:
> >
> > [ 17.921496] ------------[ cut here ]------------
> > [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921512] Modules linked in:
> > [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
> > [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > [ 17.921523] IRQ stage: Linux
> > [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
> > [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
> > [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
> > [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
> > [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
> > [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
> > [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
> > [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
> > [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
> > [ 17.921579] Call Trace:
> > [ 17.921622] <TASK>
> > [ 17.921625] ? __warn+0x85/0x140
> > [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921634] ? report_bug+0x164/0x190
> > [ 17.921641] ? handle_bug+0x65/0xd0
> > [ 17.921645] ? exc_invalid_op+0xb2/0xd0
> > [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
> > [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
> > [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
> > [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
> > [ 17.921669] cpuhp_thread_fun+0xee/0x190
> > [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
> > [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
> > [ 17.921683] kthread+0xf1/0x1e0
> > [ 17.921689] ? __pfx_kthread+0x10/0x10
> > [ 17.921693] ret_from_fork+0x34/0x50
> > [ 17.921698] ? __pfx_kthread+0x10/0x10
> > [ 17.921701] ret_from_fork_asm+0x1a/0x30
> > [ 17.921717] </TASK>
> > [ 17.921719] ---[ end trace 0000000000000000 ]---
> >
> > There were some changes to the scheduler code in between. I'm currently
> > testing with:
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 50b082b41e7d..4427fc13086d 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
> >
> > if (mm != &init_mm) {
> > mmgrab_lazy_tlb(&init_mm);
> > - local_irq_disable();
> > + hard_local_irq_disable();
> > current->active_mm = &init_mm;
> > switch_mm_irqs_off(mm, &init_mm, current);
> > - local_irq_enable();
> > + hard_local_irq_enable();
> > finish_arch_post_lock_switch();
> > mmdrop_lazy_tlb(mm);
> > }
> >
> > Florian
>
> Correct. However, since this is an inband call, we want to keep the
> virtual interrupt state in line with the expectations of the kernel, so
> we need to keep the original local_irq* calls in place. Dovetail
> provides a pair of calls namely (un)protect_inband_mm() which is
> dedicated to guarding the mm switch code from hardware interrupts, the
> general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
> these calls in this particular case serves as code annotation as well.
This survived testing as well:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 50b082b41e7d..8bcfb8e7046c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8029,12 +8029,15 @@ void sched_setnuma(struct task_struct *p, int
nid)
static void sched_force_init_mm(void)
{
struct mm_struct *mm = current->active_mm;
+ long flags;
if (mm != &init_mm) {
mmgrab_lazy_tlb(&init_mm);
local_irq_disable();
+ protect_inband_mm(flags);
current->active_mm = &init_mm;
switch_mm_irqs_off(mm, &init_mm, current);
+ unprotect_inband_mm(flags);
local_irq_enable();
finish_arch_post_lock_switch();
mmdrop_lazy_tlb(mm);
That should propably squashed into
8646b1d5622f ("x86: dovetail: enable alternate scheduling")
Agreed?
>
> --
> Philippe.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-25 9:50 ` Philippe Gerum
2025-03-26 12:13 ` Florian Bezdeka
@ 2025-03-26 12:14 ` Florian Bezdeka
2025-03-26 13:56 ` Philippe Gerum
1 sibling, 1 reply; 9+ messages in thread
From: Florian Bezdeka @ 2025-03-26 12:14 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, Xenomai
On Tue, 2025-03-25 at 10:50 +0100, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>
> > On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
> > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > >
> > > Code and comment were no in line, but the comment actually make more
> > > sense here, given that unlock_vector_lock will unconditionally reenable
> > > hard IRQs.
> > >
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > > arch/x86/kernel/apic/vector.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> > > index a08b0dff066ac..af60f138d590b 100644
> > > --- a/arch/x86/kernel/apic/vector.c
> > > +++ b/arch/x86/kernel/apic/vector.c
> > > @@ -142,7 +142,7 @@ void lock_vector_lock(void)
> > > * entry. In addition, we assume that hard irqs are on as well
> > > * (which is the regular case).
> > > */
> > > - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
> > > + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
> > > hard_cond_local_irq_disable();
> > > /* Used to the online set of cpus does not change
> > > * during assign_irq_vector.
> > > --
> > > 2.43.0
> >
> > With that applied there is still one warning left in 6.14:
> >
> > [ 17.921496] ------------[ cut here ]------------
> > [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921512] Modules linked in:
> > [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
> > [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > [ 17.921523] IRQ stage: Linux
> > [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
> > [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
> > [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
> > [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
> > [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
> > [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
> > [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
> > [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
> > [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
> > [ 17.921579] Call Trace:
> > [ 17.921622] <TASK>
> > [ 17.921625] ? __warn+0x85/0x140
> > [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921634] ? report_bug+0x164/0x190
> > [ 17.921641] ? handle_bug+0x65/0xd0
> > [ 17.921645] ? exc_invalid_op+0xb2/0xd0
> > [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
> > [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
> > [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
> > [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
> > [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
> > [ 17.921669] cpuhp_thread_fun+0xee/0x190
> > [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
> > [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
> > [ 17.921683] kthread+0xf1/0x1e0
> > [ 17.921689] ? __pfx_kthread+0x10/0x10
> > [ 17.921693] ret_from_fork+0x34/0x50
> > [ 17.921698] ? __pfx_kthread+0x10/0x10
> > [ 17.921701] ret_from_fork_asm+0x1a/0x30
> > [ 17.921717] </TASK>
> > [ 17.921719] ---[ end trace 0000000000000000 ]---
> >
> > There were some changes to the scheduler code in between. I'm currently
> > testing with:
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 50b082b41e7d..4427fc13086d 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
> >
> > if (mm != &init_mm) {
> > mmgrab_lazy_tlb(&init_mm);
> > - local_irq_disable();
> > + hard_local_irq_disable();
> > current->active_mm = &init_mm;
> > switch_mm_irqs_off(mm, &init_mm, current);
> > - local_irq_enable();
> > + hard_local_irq_enable();
> > finish_arch_post_lock_switch();
> > mmdrop_lazy_tlb(mm);
> > }
> >
> > Florian
>
> Correct. However, since this is an inband call, we want to keep the
> virtual interrupt state in line with the expectations of the kernel, so
> we need to keep the original local_irq* calls in place. Dovetail
> provides a pair of calls namely (un)protect_inband_mm() which is
> dedicated to guarding the mm switch code from hardware interrupts, the
> general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
> these calls in this particular case serves as code annotation as well.
This survived testing as well:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 50b082b41e7d..8bcfb8e7046c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8029,12 +8029,15 @@ void sched_setnuma(struct task_struct *p, int
nid)
static void sched_force_init_mm(void)
{
struct mm_struct *mm = current->active_mm;
+ long flags;
if (mm != &init_mm) {
mmgrab_lazy_tlb(&init_mm);
local_irq_disable();
+ protect_inband_mm(flags);
current->active_mm = &init_mm;
switch_mm_irqs_off(mm, &init_mm, current);
+ unprotect_inband_mm(flags);
local_irq_enable();
finish_arch_post_lock_switch();
mmdrop_lazy_tlb(mm);
That should probably be squashed into
8646b1d5622f ("x86: dovetail: enable alternate scheduling")
for the 6.14 branch (only).
Agreed?
>
> --
> Philippe.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-26 12:14 ` Florian Bezdeka
@ 2025-03-26 13:56 ` Philippe Gerum
2025-03-26 14:12 ` Philippe Gerum
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Gerum @ 2025-03-26 13:56 UTC (permalink / raw)
To: Florian Bezdeka; +Cc: Jan Kiszka, Xenomai
Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> On Tue, 2025-03-25 at 10:50 +0100, Philippe Gerum wrote:
>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>>
>> > On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
>> > > From: Jan Kiszka <jan.kiszka@siemens.com>
>> > >
>> > > Code and comment were no in line, but the comment actually make more
>> > > sense here, given that unlock_vector_lock will unconditionally reenable
>> > > hard IRQs.
>> > >
>> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> > > ---
>> > > arch/x86/kernel/apic/vector.c | 2 +-
>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> > > index a08b0dff066ac..af60f138d590b 100644
>> > > --- a/arch/x86/kernel/apic/vector.c
>> > > +++ b/arch/x86/kernel/apic/vector.c
>> > > @@ -142,7 +142,7 @@ void lock_vector_lock(void)
>> > > * entry. In addition, we assume that hard irqs are on as well
>> > > * (which is the regular case).
>> > > */
>> > > - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
>> > > + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
>> > > hard_cond_local_irq_disable();
>> > > /* Used to the online set of cpus does not change
>> > > * during assign_irq_vector.
>> > > --
>> > > 2.43.0
>> >
>> > With that applied there is still one warning left in 6.14:
>> >
>> > [ 17.921496] ------------[ cut here ]------------
>> > [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
>> > [ 17.921512] Modules linked in:
>> > [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
>> > [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
>> > [ 17.921523] IRQ stage: Linux
>> > [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
>> > [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
>> > [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
>> > [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
>> > [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
>> > [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
>> > [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
>> > [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
>> > [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
>> > [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> > [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
>> > [ 17.921579] Call Trace:
>> > [ 17.921622] <TASK>
>> > [ 17.921625] ? __warn+0x85/0x140
>> > [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
>> > [ 17.921634] ? report_bug+0x164/0x190
>> > [ 17.921641] ? handle_bug+0x65/0xd0
>> > [ 17.921645] ? exc_invalid_op+0xb2/0xd0
>> > [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
>> > [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
>> > [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
>> > [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
>> > [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
>> > [ 17.921669] cpuhp_thread_fun+0xee/0x190
>> > [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
>> > [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
>> > [ 17.921683] kthread+0xf1/0x1e0
>> > [ 17.921689] ? __pfx_kthread+0x10/0x10
>> > [ 17.921693] ret_from_fork+0x34/0x50
>> > [ 17.921698] ? __pfx_kthread+0x10/0x10
>> > [ 17.921701] ret_from_fork_asm+0x1a/0x30
>> > [ 17.921717] </TASK>
>> > [ 17.921719] ---[ end trace 0000000000000000 ]---
>> >
>> > There were some changes to the scheduler code in between. I'm currently
>> > testing with:
>> >
>> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> > index 50b082b41e7d..4427fc13086d 100644
>> > --- a/kernel/sched/core.c
>> > +++ b/kernel/sched/core.c
>> > @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
>> >
>> > if (mm != &init_mm) {
>> > mmgrab_lazy_tlb(&init_mm);
>> > - local_irq_disable();
>> > + hard_local_irq_disable();
>> > current->active_mm = &init_mm;
>> > switch_mm_irqs_off(mm, &init_mm, current);
>> > - local_irq_enable();
>> > + hard_local_irq_enable();
>> > finish_arch_post_lock_switch();
>> > mmdrop_lazy_tlb(mm);
>> > }
>> >
>> > Florian
>>
>> Correct. However, since this is an inband call, we want to keep the
>> virtual interrupt state in line with the expectations of the kernel, so
>> we need to keep the original local_irq* calls in place. Dovetail
>> provides a pair of calls namely (un)protect_inband_mm() which is
>> dedicated to guarding the mm switch code from hardware interrupts, the
>> general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
>> these calls in this particular case serves as code annotation as well.
>
> This survived testing as well:
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 50b082b41e7d..8bcfb8e7046c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8029,12 +8029,15 @@ void sched_setnuma(struct task_struct *p, int
> nid)
> static void sched_force_init_mm(void)
> {
> struct mm_struct *mm = current->active_mm;
> + long flags;
>
> if (mm != &init_mm) {
> mmgrab_lazy_tlb(&init_mm);
> local_irq_disable();
> + protect_inband_mm(flags);
> current->active_mm = &init_mm;
> switch_mm_irqs_off(mm, &init_mm, current);
> + unprotect_inband_mm(flags);
> local_irq_enable();
> finish_arch_post_lock_switch();
> mmdrop_lazy_tlb(mm);
>
> That should probably be squashed into
> 8646b1d5622f ("x86: dovetail: enable alternate scheduling")
> for the 6.14 branch (only).
>
> Agreed?
>
Ack.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-26 13:56 ` Philippe Gerum
@ 2025-03-26 14:12 ` Philippe Gerum
2025-03-26 21:57 ` Florian Bezdeka
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Gerum @ 2025-03-26 14:12 UTC (permalink / raw)
To: Florian Bezdeka; +Cc: Jan Kiszka, Xenomai
Philippe Gerum <rpm@xenomai.org> writes:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>
>> On Tue, 2025-03-25 at 10:50 +0100, Philippe Gerum wrote:
>>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>>>
>>> > On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
>>> > > From: Jan Kiszka <jan.kiszka@siemens.com>
>>> > >
>>> > > Code and comment were no in line, but the comment actually make more
>>> > > sense here, given that unlock_vector_lock will unconditionally reenable
>>> > > hard IRQs.
>>> > >
>>> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> > > ---
>>> > > arch/x86/kernel/apic/vector.c | 2 +-
>>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> > >
>>> > > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>>> > > index a08b0dff066ac..af60f138d590b 100644
>>> > > --- a/arch/x86/kernel/apic/vector.c
>>> > > +++ b/arch/x86/kernel/apic/vector.c
>>> > > @@ -142,7 +142,7 @@ void lock_vector_lock(void)
>>> > > * entry. In addition, we assume that hard irqs are on as well
>>> > > * (which is the regular case).
>>> > > */
>>> > > - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
>>> > > + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
>>> > > hard_cond_local_irq_disable();
>>> > > /* Used to the online set of cpus does not change
>>> > > * during assign_irq_vector.
>>> > > --
>>> > > 2.43.0
>>> >
>>> > With that applied there is still one warning left in 6.14:
>>> >
>>> > [ 17.921496] ------------[ cut here ]------------
>>> > [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
>>> > [ 17.921512] Modules linked in:
>>> > [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
>>> > [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
>>> > [ 17.921523] IRQ stage: Linux
>>> > [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
>>> > [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
>>> > [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
>>> > [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
>>> > [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
>>> > [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
>>> > [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
>>> > [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
>>> > [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
>>> > [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> > [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
>>> > [ 17.921579] Call Trace:
>>> > [ 17.921622] <TASK>
>>> > [ 17.921625] ? __warn+0x85/0x140
>>> > [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
>>> > [ 17.921634] ? report_bug+0x164/0x190
>>> > [ 17.921641] ? handle_bug+0x65/0xd0
>>> > [ 17.921645] ? exc_invalid_op+0xb2/0xd0
>>> > [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
>>> > [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
>>> > [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
>>> > [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
>>> > [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
>>> > [ 17.921669] cpuhp_thread_fun+0xee/0x190
>>> > [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
>>> > [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
>>> > [ 17.921683] kthread+0xf1/0x1e0
>>> > [ 17.921689] ? __pfx_kthread+0x10/0x10
>>> > [ 17.921693] ret_from_fork+0x34/0x50
>>> > [ 17.921698] ? __pfx_kthread+0x10/0x10
>>> > [ 17.921701] ret_from_fork_asm+0x1a/0x30
>>> > [ 17.921717] </TASK>
>>> > [ 17.921719] ---[ end trace 0000000000000000 ]---
>>> >
>>> > There were some changes to the scheduler code in between. I'm currently
>>> > testing with:
>>> >
>>> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> > index 50b082b41e7d..4427fc13086d 100644
>>> > --- a/kernel/sched/core.c
>>> > +++ b/kernel/sched/core.c
>>> > @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
>>> >
>>> > if (mm != &init_mm) {
>>> > mmgrab_lazy_tlb(&init_mm);
>>> > - local_irq_disable();
>>> > + hard_local_irq_disable();
>>> > current->active_mm = &init_mm;
>>> > switch_mm_irqs_off(mm, &init_mm, current);
>>> > - local_irq_enable();
>>> > + hard_local_irq_enable();
>>> > finish_arch_post_lock_switch();
>>> > mmdrop_lazy_tlb(mm);
>>> > }
>>> >
>>> > Florian
>>>
>>> Correct. However, since this is an inband call, we want to keep the
>>> virtual interrupt state in line with the expectations of the kernel, so
>>> we need to keep the original local_irq* calls in place. Dovetail
>>> provides a pair of calls namely (un)protect_inband_mm() which is
>>> dedicated to guarding the mm switch code from hardware interrupts, the
>>> general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
>>> these calls in this particular case serves as code annotation as well.
>>
>> This survived testing as well:
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 50b082b41e7d..8bcfb8e7046c 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -8029,12 +8029,15 @@ void sched_setnuma(struct task_struct *p, int
>> nid)
>> static void sched_force_init_mm(void)
>> {
>> struct mm_struct *mm = current->active_mm;
>> + long flags;
>>
>> if (mm != &init_mm) {
>> mmgrab_lazy_tlb(&init_mm);
>> local_irq_disable();
>> + protect_inband_mm(flags);
>> current->active_mm = &init_mm;
>> switch_mm_irqs_off(mm, &init_mm, current);
>> + unprotect_inband_mm(flags);
>> local_irq_enable();
>> finish_arch_post_lock_switch();
>> mmdrop_lazy_tlb(mm);
>>
>> That should probably be squashed into
>> 8646b1d5622f ("x86: dovetail: enable alternate scheduling")
>> for the 6.14 branch (only).
>>
>> Agreed?
>>
>
> Ack.
Nope, this is wrong. This should rather go to the generic Dovetail bits in:
f185b0770c116 dovetail: add core support
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock
2025-03-26 14:12 ` Philippe Gerum
@ 2025-03-26 21:57 ` Florian Bezdeka
0 siblings, 0 replies; 9+ messages in thread
From: Florian Bezdeka @ 2025-03-26 21:57 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, Xenomai
On Wed, 2025-03-26 at 15:12 +0100, Philippe Gerum wrote:
> Philippe Gerum <rpm@xenomai.org> writes:
>
> > Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> >
> > > On Tue, 2025-03-25 at 10:50 +0100, Philippe Gerum wrote:
> > > > Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> > > >
> > > > > On Mon, 2025-03-24 at 18:18 +0100, Jan Kiszka wrote:
> > > > > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > >
> > > > > > Code and comment were no in line, but the comment actually make more
> > > > > > sense here, given that unlock_vector_lock will unconditionally reenable
> > > > > > hard IRQs.
> > > > > >
> > > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > > ---
> > > > > > arch/x86/kernel/apic/vector.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> > > > > > index a08b0dff066ac..af60f138d590b 100644
> > > > > > --- a/arch/x86/kernel/apic/vector.c
> > > > > > +++ b/arch/x86/kernel/apic/vector.c
> > > > > > @@ -142,7 +142,7 @@ void lock_vector_lock(void)
> > > > > > * entry. In addition, we assume that hard irqs are on as well
> > > > > > * (which is the regular case).
> > > > > > */
> > > > > > - WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
> > > > > > + WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
> > > > > > hard_cond_local_irq_disable();
> > > > > > /* Used to the online set of cpus does not change
> > > > > > * during assign_irq_vector.
> > > > > > --
> > > > > > 2.43.0
> > > > >
> > > > > With that applied there is still one warning left in 6.14:
> > > > >
> > > > > [ 17.921496] ------------[ cut here ]------------
> > > > > [ 17.921502] WARNING: CPU: 3 PID: 33 at arch/x86/mm/tlb.c:516 switch_mm_irqs_off+0x39d/0x4f0
> > > > > [ 17.921512] Modules linked in:
> > > > > [ 17.921517] CPU: 3 UID: 0 PID: 33 Comm: cpuhp/3 Not tainted 6.14.0-rc7+ #82
> > > > > [ 17.921521] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > > > > [ 17.921523] IRQ stage: Linux
> > > > > [ 17.921526] RIP: 0010:switch_mm_irqs_off+0x39d/0x4f0
> > > > > [ 17.921531] Code: 06 75 de 65 c6 05 e6 82 db 7e 00 e9 77 fd ff ff 48 8b 05 3e 79 18 01 b9 49 00 00 00 48 89 c2 48 c1 ea 20 0f 30 e9 fb fc ff ff <0f> 0b e9 9f fc ff ff 0f 0b e9 4f fd ff ff 65 48 c7 05e
> > > > > [ 17.921533] RSP: 0018:ffffc90000133de8 EFLAGS: 00010202
> > > > > [ 17.921537] RAX: 0000000000000001 RBX: ffffffff8270e000 RCX: 0000000000000207
> > > > > [ 17.921539] RDX: 0000000000000206 RSI: ffffffff8270e000 RDI: ffff8880065ae3c0
> > > > > [ 17.921541] RBP: ffff88800378ab80 R08: ffff88803e79c5c8 R09: 000000000000019d
> > > > > [ 17.921543] R10: ffff88803e79c5a0 R11: 0000000000000001 R12: ffff8880065ae3c0
> > > > > [ 17.921545] R13: 0000000000000000 R14: 0000000000000003 R15: ffff88803e79c5c8
> > > > > [ 17.921558] FS: 0000000000000000(0000) GS:ffff88803e780000(0000) knlGS:0000000000000000
> > > > > [ 17.921574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > [ 17.921577] CR2: 000055ed7e138e28 CR3: 0000000003ea8000 CR4: 00000000003506f0
> > > > > [ 17.921579] Call Trace:
> > > > > [ 17.921622] <TASK>
> > > > > [ 17.921625] ? __warn+0x85/0x140
> > > > > [ 17.921630] ? switch_mm_irqs_off+0x39d/0x4f0
> > > > > [ 17.921634] ? report_bug+0x164/0x190
> > > > > [ 17.921641] ? handle_bug+0x65/0xd0
> > > > > [ 17.921645] ? exc_invalid_op+0xb2/0xd0
> > > > > [ 17.921648] ? asm_exc_invalid_op+0x1a/0x20
> > > > > [ 17.921653] ? switch_mm_irqs_off+0x39d/0x4f0
> > > > > [ 17.921657] ? __pfx_sched_cpu_wait_empty+0x10/0x10
> > > > > [ 17.921661] sched_cpu_wait_empty+0x93/0xc0
> > > > > [ 17.921665] cpuhp_invoke_callback+0x114/0x4b0
> > > > > [ 17.921669] cpuhp_thread_fun+0xee/0x190
> > > > > [ 17.921674] ? __pfx_smpboot_thread_fn+0x10/0x10
> > > > > [ 17.921678] smpboot_thread_fn+0xdd/0x1d0
> > > > > [ 17.921683] kthread+0xf1/0x1e0
> > > > > [ 17.921689] ? __pfx_kthread+0x10/0x10
> > > > > [ 17.921693] ret_from_fork+0x34/0x50
> > > > > [ 17.921698] ? __pfx_kthread+0x10/0x10
> > > > > [ 17.921701] ret_from_fork_asm+0x1a/0x30
> > > > > [ 17.921717] </TASK>
> > > > > [ 17.921719] ---[ end trace 0000000000000000 ]---
> > > > >
> > > > > There were some changes to the scheduler code in between. I'm currently
> > > > > testing with:
> > > > >
> > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > > index 50b082b41e7d..4427fc13086d 100644
> > > > > --- a/kernel/sched/core.c
> > > > > +++ b/kernel/sched/core.c
> > > > > @@ -8032,10 +8032,10 @@ static void sched_force_init_mm(void)
> > > > >
> > > > > if (mm != &init_mm) {
> > > > > mmgrab_lazy_tlb(&init_mm);
> > > > > - local_irq_disable();
> > > > > + hard_local_irq_disable();
> > > > > current->active_mm = &init_mm;
> > > > > switch_mm_irqs_off(mm, &init_mm, current);
> > > > > - local_irq_enable();
> > > > > + hard_local_irq_enable();
> > > > > finish_arch_post_lock_switch();
> > > > > mmdrop_lazy_tlb(mm);
> > > > > }
> > > > >
> > > > > Florian
> > > >
> > > > Correct. However, since this is an inband call, we want to keep the
> > > > virtual interrupt state in line with the expectations of the kernel, so
> > > > we need to keep the original local_irq* calls in place. Dovetail
> > > > provides a pair of calls namely (un)protect_inband_mm() which is
> > > > dedicated to guarding the mm switch code from hardware interrupts, the
> > > > general logic is visible from switch_mm() in arch/x86/mm/tlb.c. Using
> > > > these calls in this particular case serves as code annotation as well.
> > >
> > > This survived testing as well:
> > >
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 50b082b41e7d..8bcfb8e7046c 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -8029,12 +8029,15 @@ void sched_setnuma(struct task_struct *p, int
> > > nid)
> > > static void sched_force_init_mm(void)
> > > {
> > > struct mm_struct *mm = current->active_mm;
> > > + long flags;
> > >
> > > if (mm != &init_mm) {
> > > mmgrab_lazy_tlb(&init_mm);
> > > local_irq_disable();
> > > + protect_inband_mm(flags);
> > > current->active_mm = &init_mm;
> > > switch_mm_irqs_off(mm, &init_mm, current);
> > > + unprotect_inband_mm(flags);
> > > local_irq_enable();
> > > finish_arch_post_lock_switch();
> > > mmdrop_lazy_tlb(mm);
> > >
> > > That should probably be squashed into
> > > 8646b1d5622f ("x86: dovetail: enable alternate scheduling")
> > > for the 6.14 branch (only).
> > >
> > > Agreed?
> > >
> >
> > Ack.
>
> Nope, this is wrong. This should rather go to the generic Dovetail bits in:
> f185b0770c116 dovetail: add core support
>
The wip/v6.14-dovetail-rebase branch is up-to-date now and already
based on the final 6.14 release.
Testing looks good so far.
> --
> Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-26 21:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-24 17:18 [PATCH] x86: dovetail: Fix inverted debug check in lock_vector_lock Jan Kiszka
2025-03-24 18:58 ` Florian Bezdeka
2025-03-24 22:52 ` Florian Bezdeka
2025-03-25 9:50 ` Philippe Gerum
2025-03-26 12:13 ` Florian Bezdeka
2025-03-26 12:14 ` Florian Bezdeka
2025-03-26 13:56 ` Philippe Gerum
2025-03-26 14:12 ` Philippe Gerum
2025-03-26 21:57 ` Florian Bezdeka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.