Messages in this thread Patch in this message | | | From | Paul Burton <> | Subject | Re: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop | Date | Thu, 14 Mar 2019 18:13:07 +0000 |
| |
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 >
| |