Messages in this thread | | | Subject | Re: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop | From | Valentin Schneider <> | Date | Thu, 14 Mar 2019 18:38:33 +0000 |
| |
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 >>
| |