| Message ID | 20190311224752.8337-7-valentin.schneider@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | entry: preempt_schedule_irq() callers scrub | expand |
Hi Valentin, On Mon, Mar 11, 2019 at 10:47:44PM +0000, Valentin Schneider wrote: > Since the enabling and disabling of IRQs within preempt_schedule_irq() > is contained in a need_resched() loop, we don't need the outer arch > code loop. > > Do note that commit a18815abcdfd ("Use preempt_schedule_irq.") > initially removed the existing loop, but commit cdaed73afb61 ("Fix > preemption bug.") reintroduced it. It is however not clear what the > issue was and why such a loop was reintroduced, so I'm probably > missing something. It looks to me like commit a18815abcdfd ("Use preempt_schedule_irq.") forgot the branch to restore_all, so would have fallen through to ret_from_fork() & done weird things. Adding the branch to restore_all as you're doing here would have been a better fix than commit cdaed73afb61 ("Fix preemption bug."). > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Paul Burton <paul.burton@mips.com> > Cc: James Hogan <jhogan@kernel.org> > Cc: linux-mips@vger.kernel.org > --- > arch/mips/kernel/entry.S | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S > index d7de8adcfcc8..2240faeda62a 100644 > --- a/arch/mips/kernel/entry.S > +++ b/arch/mips/kernel/entry.S > @@ -58,7 +58,6 @@ resume_kernel: > local_irq_disable > lw t0, TI_PRE_COUNT($28) > bnez t0, restore_all > -need_resched: > LONG_L t0, TI_FLAGS($28) > andi t1, t0, _TIF_NEED_RESCHED > beqz t1, restore_all > @@ -66,7 +65,7 @@ need_resched: > andi t0, 1 > beqz t0, restore_all > jal preempt_schedule_irq > - b need_resched > + j restore_all One nit - why change from branch to jump? It's not a big deal, but I'd prefer we stick with the branch ("b") instruction for a few reasons: - restore_all is nearby so there's no issue with it being out of range of a branch in any variation of the MIPS ISA. - It's more consistent with the future of the MIPS architecture, eg. nanoMIPS in which branch instructions all use PC-relative immediate offsets & jump instructions are always of the "register" variety where the destination is specified by a register rather than an immediate encoded in the instruction (the assembler will fix it up & emit a branch anyway, but I generally prefer to invoke less magic in these areas...). - A PC-relative branch won't generate an extra reloc in a relocatable kernel, whereas a jump will. Even better would be if we take advantage of this being a tail call & do this: PTR_LA ra, restore_all j preempt_schedule_irq (Where I left the call to preempt_schedule_irq using a jump because it may be further away.) Though I don't mind if you wanna just s/j/b/ & leave the tail call optimisation for someone else to do as a later change. Thanks, Paul > #endif > > FEXPORT(ret_from_kernel_thread) > -- > 2.20.1 >
Hi Paul, On 14/03/2019 18:13, Paul Burton wrote: [...] > > It looks to me like commit a18815abcdfd ("Use preempt_schedule_irq.") > forgot the branch to restore_all, so would have fallen through to > ret_from_fork() & done weird things. > > Adding the branch to restore_all as you're doing here would have been a > better fix than commit cdaed73afb61 ("Fix preemption bug."). > I didn't notice the missing branch to restore_all in that first commit - that makes (more) sense now. [...] >> @@ -66,7 +65,7 @@ need_resched: >> andi t0, 1 >> beqz t0, restore_all >> jal preempt_schedule_irq >> - b need_resched >> + j restore_all > > One nit - why change from branch to jump? No actual reason there, I most likely deleted the branch, looked around, saw the "j restore_all" in @resume_userspace and went for that (shoddy I know...) > It's not a big deal, but I'd > prefer we stick with the branch ("b") instruction for a few reasons: > > - restore_all is nearby so there's no issue with it being out of range > of a branch in any variation of the MIPS ISA. > > - It's more consistent with the future of the MIPS architecture, eg. > nanoMIPS in which branch instructions all use PC-relative immediate > offsets & jump instructions are always of the "register" variety where > the destination is specified by a register rather than an immediate > encoded in the instruction (the assembler will fix it up & emit a > branch anyway, but I generally prefer to invoke less magic in these > areas...). > > - A PC-relative branch won't generate an extra reloc in a relocatable > kernel, whereas a jump will. > Makes total sense, thanks for the detailed reasoning! > Even better would be if we take advantage of this being a tail call & do > this: > > PTR_LA ra, restore_all > j preempt_schedule_irq > > (Where I left the call to preempt_schedule_irq using a jump because it > may be further away.) > Right, that's even better, I'll send a v2 with that. > Though I don't mind if you wanna just s/j/b/ & leave the tail call > optimisation for someone else to do as a later change. > > Thanks, > Paul > >> #endif >> >> FEXPORT(ret_from_kernel_thread) >> -- >> 2.20.1 >>
On Thu, 14 Mar 2019, Paul Burton wrote: > > @@ -66,7 +65,7 @@ need_resched: > > andi t0, 1 > > beqz t0, restore_all > > jal preempt_schedule_irq > > - b need_resched > > + j restore_all > > One nit - why change from branch to jump? It's not a big deal, but I'd > prefer we stick with the branch ("b") instruction for a few reasons: > > - restore_all is nearby so there's no issue with it being out of range > of a branch in any variation of the MIPS ISA. FYI, if it does go out of range for whatever reason, then for non-PIC code GAS will relax it to a jump anyway (with a relocation attached); for the regular MIPS ISA that is, where it has been doing that since forever (I meant to implement this for the microMIPS ISA too, but IIRC there was a complication there, probably coming from the existing more complex branch relaxation code and/or slightly different use of relocations, and then it fell through the cracks). Maciej
diff --git a/arch/mips/kernel/entry.S b/arch/mips/kernel/entry.S index d7de8adcfcc8..2240faeda62a 100644 --- a/arch/mips/kernel/entry.S +++ b/arch/mips/kernel/entry.S @@ -58,7 +58,6 @@ resume_kernel: local_irq_disable lw t0, TI_PRE_COUNT($28) bnez t0, restore_all -need_resched: LONG_L t0, TI_FLAGS($28) andi t1, t0, _TIF_NEED_RESCHED beqz t1, restore_all @@ -66,7 +65,7 @@ need_resched: andi t0, 1 beqz t0, restore_all jal preempt_schedule_irq - b need_resched + j restore_all #endif FEXPORT(ret_from_kernel_thread)
Since the enabling and disabling of IRQs within preempt_schedule_irq() is contained in a need_resched() loop, we don't need the outer arch code loop. Do note that commit a18815abcdfd ("Use preempt_schedule_irq.") initially removed the existing loop, but commit cdaed73afb61 ("Fix preemption bug.") reintroduced it. It is however not clear what the issue was and why such a loop was reintroduced, so I'm probably missing something. Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Paul Burton <paul.burton@mips.com> Cc: James Hogan <jhogan@kernel.org> Cc: linux-mips@vger.kernel.org --- arch/mips/kernel/entry.S | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)