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
>>
