[lkml]   [2019]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 06/14] MIPS: entry: Remove unneeded need_resched() loop
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

 \ /
  Last update: 2019-03-14 19:38    [W:0.125 / U:25.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site