| Message ID | 20250227114141.242421-2-marco.crivellari@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Fix idle VS timer enqueue | expand |
Hi, Marco, On Thu, Feb 27, 2025 at 7:41 PM Marco Crivellari <marco.crivellari@suse.com> wrote: > > MIPS re-enables interrupts on its idle routine and performs > a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep. > > The IRQs firing between the check and the 'wait' instruction may set the > TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs > interrupting __r4k_wait() rollback their return address to the > beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked > again before going back to sleep. > > However idle IRQs can also queue timers that may require a tick > reprogramming through a new generic idle loop iteration but those timers > would go unnoticed here because __r4k_wait() only checks > TIF_NEED_RESCHED. It doesn't check for pending timers. > > Fix this with fast-forwarding idle IRQs return address to the end of the > idle routine instead of the beginning, so that the generic idle loop > handles both TIF_NEED_RESCHED and pending timers. > > CONFIG_CPU_MICROMIPS has been removed along with the nop instructions. > There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are > always 4 byte and remove the ifdef. Added ehb to make sure the hazard > is always cleared. > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > --- > arch/mips/kernel/genex.S | 40 ++++++++++++++++++++++------------------ > arch/mips/kernel/idle.c | 1 - > 2 files changed, 22 insertions(+), 19 deletions(-) > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > index a572ce36a24f..7b66ab239049 100644 > --- a/arch/mips/kernel/genex.S > +++ b/arch/mips/kernel/genex.S > @@ -104,25 +104,28 @@ handle_vcei: > > __FINIT > > - .align 5 /* 32 byte rollback region */ > + .align 5 > LEAF(__r4k_wait) > .set push > .set noreorder > - /* start of rollback region */ > - LONG_L t0, TI_FLAGS($28) > - nop > - andi t0, _TIF_NEED_RESCHED > - bnez t0, 1f > - nop > - nop > - nop > -#ifdef CONFIG_CPU_MICROMIPS > - nop > - nop > - nop > - nop > -#endif > + /* start of idle interrupt region */ > + MFC0 t0, CP0_STATUS > + /* Enable Interrput */ > + ori t0, 0x1f > + xori t0, 0x1e > + MTC0 t0, CP0_STATUS > + _ssnop > + _ssnop > + _ssnop > + _ehb > .set MIPS_ISA_ARCH_LEVEL_RAW > + /* > + * If an interrupt lands here, between enabling interrupts above and > + * going idle on the next instruction, we must *NOT* go idle since the > + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need > + * resched. Fall through -- see rollback_handler below -- and have > + * the idle loop take care of things. > + */ > wait > /* end of rollback region (the region size must be power of two) */ Now you add a _ehb in this version, so "(the region size must be power of two)" should be removed. > 1: > @@ -136,9 +139,10 @@ LEAF(__r4k_wait) > .set push > .set noat > MFC0 k0, CP0_EPC > - PTR_LA k1, __r4k_wait > - ori k0, 0x1f /* 32 byte rollback region */ > - xori k0, 0x1f > + PTR_LA k1, 1b > + /* 32 byte idle interrupt region */ And now the region is 36 bytes. :) Huacai > + ori k0, 0x1f > + PTR_ADDIU k0, 5 > bne k0, k1, \handler > MTC0 k0, CP0_EPC > .set pop > diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c > index 5abc8b7340f8..1f74c0589f7e 100644 > --- a/arch/mips/kernel/idle.c > +++ b/arch/mips/kernel/idle.c > @@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void) > > void __cpuidle r4k_wait(void) > { > - raw_local_irq_enable(); > __r4k_wait(); > raw_local_irq_disable(); > } > -- > 2.48.1 >
Hi Huacai, sorry, I completely forgot to check the comments! On Thu, Feb 27, 2025 at 12:56 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Marco, > > On Thu, Feb 27, 2025 at 7:41 PM Marco Crivellari > <marco.crivellari@suse.com> wrote: > > > > MIPS re-enables interrupts on its idle routine and performs > > a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep. > > > > The IRQs firing between the check and the 'wait' instruction may set the > > TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs > > interrupting __r4k_wait() rollback their return address to the > > beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked > > again before going back to sleep. > > > > However idle IRQs can also queue timers that may require a tick > > reprogramming through a new generic idle loop iteration but those timers > > would go unnoticed here because __r4k_wait() only checks > > TIF_NEED_RESCHED. It doesn't check for pending timers. > > > > Fix this with fast-forwarding idle IRQs return address to the end of the > > idle routine instead of the beginning, so that the generic idle loop > > handles both TIF_NEED_RESCHED and pending timers. > > > > CONFIG_CPU_MICROMIPS has been removed along with the nop instructions. > > There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are > > always 4 byte and remove the ifdef. Added ehb to make sure the hazard > > is always cleared. > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > --- > > arch/mips/kernel/genex.S | 40 ++++++++++++++++++++++------------------ > > arch/mips/kernel/idle.c | 1 - > > 2 files changed, 22 insertions(+), 19 deletions(-) > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > index a572ce36a24f..7b66ab239049 100644 > > --- a/arch/mips/kernel/genex.S > > +++ b/arch/mips/kernel/genex.S > > @@ -104,25 +104,28 @@ handle_vcei: > > > > __FINIT > > > > - .align 5 /* 32 byte rollback region */ > > + .align 5 > > LEAF(__r4k_wait) > > .set push > > .set noreorder > > - /* start of rollback region */ > > - LONG_L t0, TI_FLAGS($28) > > - nop > > - andi t0, _TIF_NEED_RESCHED > > - bnez t0, 1f > > - nop > > - nop > > - nop > > -#ifdef CONFIG_CPU_MICROMIPS > > - nop > > - nop > > - nop > > - nop > > -#endif > > + /* start of idle interrupt region */ > > + MFC0 t0, CP0_STATUS > > + /* Enable Interrput */ > > + ori t0, 0x1f > > + xori t0, 0x1e > > + MTC0 t0, CP0_STATUS > > + _ssnop > > + _ssnop > > + _ssnop > > + _ehb > > .set MIPS_ISA_ARCH_LEVEL_RAW > > + /* > > + * If an interrupt lands here, between enabling interrupts above and > > + * going idle on the next instruction, we must *NOT* go idle since the > > + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need > > + * resched. Fall through -- see rollback_handler below -- and have > > + * the idle loop take care of things. > > + */ > > wait > > /* end of rollback region (the region size must be power of two) */ > Now you add a _ehb in this version, so "(the region size must be power > of two)" should be removed. > > > 1: > > @@ -136,9 +139,10 @@ LEAF(__r4k_wait) > > .set push > > .set noat > > MFC0 k0, CP0_EPC > > - PTR_LA k1, __r4k_wait > > - ori k0, 0x1f /* 32 byte rollback region */ > > - xori k0, 0x1f > > + PTR_LA k1, 1b > > + /* 32 byte idle interrupt region */ > And now the region is 36 bytes. :) > > Huacai > > > + ori k0, 0x1f > > + PTR_ADDIU k0, 5 > > bne k0, k1, \handler > > MTC0 k0, CP0_EPC > > .set pop > > diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c > > index 5abc8b7340f8..1f74c0589f7e 100644 > > --- a/arch/mips/kernel/idle.c > > +++ b/arch/mips/kernel/idle.c > > @@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void) > > > > void __cpuidle r4k_wait(void) > > { > > - raw_local_irq_enable(); > > __r4k_wait(); > > raw_local_irq_disable(); > > } > > -- > > 2.48.1 > >
diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S index a572ce36a24f..7b66ab239049 100644 --- a/arch/mips/kernel/genex.S +++ b/arch/mips/kernel/genex.S @@ -104,25 +104,28 @@ handle_vcei: __FINIT - .align 5 /* 32 byte rollback region */ + .align 5 LEAF(__r4k_wait) .set push .set noreorder - /* start of rollback region */ - LONG_L t0, TI_FLAGS($28) - nop - andi t0, _TIF_NEED_RESCHED - bnez t0, 1f - nop - nop - nop -#ifdef CONFIG_CPU_MICROMIPS - nop - nop - nop - nop -#endif + /* start of idle interrupt region */ + MFC0 t0, CP0_STATUS + /* Enable Interrput */ + ori t0, 0x1f + xori t0, 0x1e + MTC0 t0, CP0_STATUS + _ssnop + _ssnop + _ssnop + _ehb .set MIPS_ISA_ARCH_LEVEL_RAW + /* + * If an interrupt lands here, between enabling interrupts above and + * going idle on the next instruction, we must *NOT* go idle since the + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need + * resched. Fall through -- see rollback_handler below -- and have + * the idle loop take care of things. + */ wait /* end of rollback region (the region size must be power of two) */ 1: @@ -136,9 +139,10 @@ LEAF(__r4k_wait) .set push .set noat MFC0 k0, CP0_EPC - PTR_LA k1, __r4k_wait - ori k0, 0x1f /* 32 byte rollback region */ - xori k0, 0x1f + PTR_LA k1, 1b + /* 32 byte idle interrupt region */ + ori k0, 0x1f + PTR_ADDIU k0, 5 bne k0, k1, \handler MTC0 k0, CP0_EPC .set pop diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c index 5abc8b7340f8..1f74c0589f7e 100644 --- a/arch/mips/kernel/idle.c +++ b/arch/mips/kernel/idle.c @@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void) void __cpuidle r4k_wait(void) { - raw_local_irq_enable(); __r4k_wait(); raw_local_irq_disable(); }
MIPS re-enables interrupts on its idle routine and performs a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep. The IRQs firing between the check and the 'wait' instruction may set the TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs interrupting __r4k_wait() rollback their return address to the beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked again before going back to sleep. However idle IRQs can also queue timers that may require a tick reprogramming through a new generic idle loop iteration but those timers would go unnoticed here because __r4k_wait() only checks TIF_NEED_RESCHED. It doesn't check for pending timers. Fix this with fast-forwarding idle IRQs return address to the end of the idle routine instead of the beginning, so that the generic idle loop handles both TIF_NEED_RESCHED and pending timers. CONFIG_CPU_MICROMIPS has been removed along with the nop instructions. There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are always 4 byte and remove the ifdef. Added ehb to make sure the hazard is always cleared. Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> --- arch/mips/kernel/genex.S | 40 ++++++++++++++++++++++------------------ arch/mips/kernel/idle.c | 1 - 2 files changed, 22 insertions(+), 19 deletions(-)