Re: [PATCH 3/4] x86: Rewrite switch_to() code

From: Andy Lutomirski
Date: Sun May 22 2016 - 17:08:04 EST


On Sun, May 22, 2016 at 12:31 PM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
> On Sun, May 22, 2016 at 1:59 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> bottom of the stack of an inactive task?
>>
>> On May 21, 2016 9:05 AM, "Brian Gerst" <brgerst@xxxxxxxxx> wrote:
>>>
>>> Move the low-level context switch code to an out-of-line asm stub instead of
>>> using complex inline asm. This allows constructing a new stack frame for the
>>> child process to make it seamlessly flow to ret_from_fork without an extra
>>> test and branch in __switch_to(). It also improves code generation for
>>> __schedule() by using the C calling convention instead of clobbering all
>>> registers.
>>
>> I like the concept a lot.
>>
>>>
>>> Signed-off-by: Brian Gerst <brgerst@xxxxxxxxx>
>>> ---
>>> arch/x86/entry/entry_32.S | 38 ++++++++++
>>> arch/x86/entry/entry_64.S | 42 +++++++++++-
>>> arch/x86/include/asm/processor.h | 3 -
>>> arch/x86/include/asm/switch_to.h | 137 ++++++-------------------------------
>>> arch/x86/include/asm/thread_info.h | 2 -
>>> arch/x86/kernel/asm-offsets.c | 6 ++
>>> arch/x86/kernel/asm-offsets_32.c | 5 ++
>>> arch/x86/kernel/asm-offsets_64.c | 5 ++
>>> arch/x86/kernel/process_32.c | 8 ++-
>>> arch/x86/kernel/process_64.c | 7 +-
>>> arch/x86/kernel/smpboot.c | 1 -
>>> 11 files changed, 124 insertions(+), 130 deletions(-)
>>>
>>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>>> index ee6fea0..05e5340 100644
>>> --- a/arch/x86/entry/entry_32.S
>>> +++ b/arch/x86/entry/entry_32.S
>>> @@ -204,6 +204,44 @@
>>> POP_GS_EX
>>> .endm
>>>
>>> +/*
>>> + * %eax: prev task
>>> + * %edx: next task
>>> + */
>>> +ENTRY(__switch_to_asm)
>>> + /*
>>> + * Save callee-saved registers
>>> + * This must match the order in struct fork_frame
>>> + * Frame pointer must be last for get_wchan
>>> + */
>>> + pushl %ebx
>>> + pushl %edi
>>> + pushl %esi
>>> + pushl %ebp
>>> +
>>> + /* switch stack */
>>> + movl %esp, TASK_threadsp(%eax)
>>> + movl TASK_threadsp(%edx), %esp
>>> +
>>> +#ifdef CONFIG_CC_STACKPROTECTOR
>>> + movl TASK_stack_canary(%edx), %ebx
>>> + movl %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
>>> +#endif
>>> +
>>> + /* restore callee-saved registers */
>>> + popl %ebp
>>> + popl %esi
>>> + popl %edi
>>> + popl %ebx
>>
>> This is highly, highly magical. eax and edx are prev and next, and:
>
> What is so magical about the standard C calling convention (regarm(3)
> in the 32-bit case)? This just passes them right though to
> __switch_to().
>

I guess it's not highly magical, just a bit different from what I
expected. I guess it's okay.

--Andy

>>> +
>>> + jmp __switch_to
>>
>> leaves prev in eax. This works, but it might be worth a comment.
>
> Not quite, __switch_to() returns 'last', not 'prev'. The previous
> task when this is called is not the same task when the thread wakes
> up.

Right.

I wish switch_to were a normal function instead of a silly macro.

--Andy