x86: Rewrite exit-to-userspace code

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

This is incomplete, but it's finally good enough that I think it's time to get other opinions on it. It is a complete rewrite of the slow path code that handles exits to user mode.

The exit-to-usermode code is copied in several places and is written in a nasty combination of asm and C. It's not at all clear what it's supposed to do, and the way it's structured makes it very hard to work with. For example, it's not even clear why syscall exit hooks are called only once per syscall right now. (It seems to be a side effect of the way that rdi and rdx are handled in the asm loop, and it seems reliable, but it's still pointlessly complicated.) The existing code also makes context tracking overly complicated and hard to understand. Finally, it's nearly impossible for anyone to change what happens on exit to usermode, since the existing code is so fragile.

I tried to clean it up incrementally, but I decided it was too hard. Instead, this series just replaces the code. It seems to work.

Context tracking in particular works very differently now. The low-level entry code checks that we're in CONTEXT_USER and switches to CONTEXT_KERNEL. The exit code does the reverse. There is no need to track what CONTEXT_XYZ state we came from, because we already know. Similarly, SCHEDULE_USER is gone, since we can reschedule if needed by simply calling schedule() from C code.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Because I haven't converted the 32-bit code yet, all of the now-unnecessary unnecessary calls to exception_enter are still present in traps.c.

IRQ context tracking is still duplicated. We should probably clean it up by changing the core code to supply something like irq_enter_we_are_already_in_context_kernel.

Thoughts?

This is separate for ease of bisection.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 68 +++++------------------------------------------
 1 file changed, 7 insertions(+), 61 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index 33acc3dcc281..a5044d7a9d43 100644

Quoted 15 lines · Andy Lutomirski

This is separate for ease of bisection.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 68 +++++------------------------------------------
 1 file changed, 7 insertions(+), 61 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index 33acc3dcc281..a5044d7a9d43 100644
@@ -229,6 +229,11 @@ entry_SYSCALL_64_fastpath:
 	 */
 	USERGS_SYSRET64
 

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 235

Quoted 4 lines

arch/x86/entry/entry_64.S +4 -0

+GLOBAL(int_ret_from_sys_call_irqs_off)
+	TRACE_IRQS_ON
+	ENABLE_INTERRUPTS(CLBR_NONE)
+	jmp int_ret_from_sys_call

Any reason why irq state tracking cannot be done in C as well, like the rest of the irq state tracking code?

Quoted 3 lines · Andy Lutomirski

This is incomplete, but it's finally good enough that I think it's time to get other opinions on it. It is a complete rewrite of the slow path code that handles exits to user mode.

Modulo the small comments I made about the debug checks interface plus naming details the structure and intention of this series gives me warm fuzzy feelings.

Quoted 9 lines

The exit-to-usermode code is copied in several places and is written in a nasty combination of asm and C. It's not at all clear what it's supposed to do, and the way it's structured makes it very hard to work with. For example, it's not even clear why syscall exit hooks are called only once per syscall right now. (It seems to be a side effect of the way that rdi and rdx are handled in the asm loop, and it seems reliable, but it's still pointlessly complicated.) The existing code also makes context tracking overly complicated and hard to understand. Finally, it's nearly impossible for anyone to change what happens on exit to usermode, since the existing code is so fragile.

Amen.

Quoted 2 lines

I tried to clean it up incrementally, but I decided it was too hard. Instead, this series just replaces the code. It seems to work.

Any known bugs beyond UML build breakage?

Quoted 13 lines

Context tracking in particular works very differently now. The low-level entry code checks that we're in CONTEXT_USER and switches to CONTEXT_KERNEL. The exit code does the reverse. There is no need to track what CONTEXT_XYZ state we came from, because we already know. Similarly, SCHEDULE_USER is gone, since we can reschedule if needed by simply calling schedule() from C code.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Because I haven't converted the 32-bit code yet, all of the now-unnecessary unnecessary calls to exception_enter are still present in traps.c.

Quoted 5 lines

IRQ context tracking is still duplicated. We should probably clean it up by changing the core code to supply something like irq_enter_we_are_already_in_context_kernel.

Thoughts?

So assuming you fix the UML build I'm inclined to go for it, even in this incomplete form, to increase testing coverage.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Quoted 44 lines · Ingo Molnar

Quoted 3 lines · Andy Lutomirski

This is incomplete, but it's finally good enough that I think it's time to get other opinions on it. It is a complete rewrite of the slow path code that handles exits to user mode.

Modulo the small comments I made about the debug checks interface plus naming details the structure and intention of this series gives me warm fuzzy feelings.

Quoted 9 lines

The exit-to-usermode code is copied in several places and is written in a nasty combination of asm and C. It's not at all clear what it's supposed to do, and the way it's structured makes it very hard to work with. For example, it's not even clear why syscall exit hooks are called only once per syscall right now. (It seems to be a side effect of the way that rdi and rdx are handled in the asm loop, and it seems reliable, but it's still pointlessly complicated.) The existing code also makes context tracking overly complicated and hard to understand. Finally, it's nearly impossible for anyone to change what happens on exit to usermode, since the existing code is so fragile.

Amen.

Quoted 2 lines

I tried to clean it up incrementally, but I decided it was too hard. Instead, this series just replaces the code. It seems to work.

Any known bugs beyond UML build breakage?

Quoted 17 lines

Context tracking in particular works very differently now. The low-level entry code checks that we're in CONTEXT_USER and switches to CONTEXT_KERNEL. The exit code does the reverse. There is no need to track what CONTEXT_XYZ state we came from, because we already know. Similarly, SCHEDULE_USER is gone, since we can reschedule if needed by simply calling schedule() from C code.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Because I haven't converted the 32-bit code yet, all of the now-unnecessary unnecessary calls to exception_enter are still present in traps.c.

IRQ context tracking is still duplicated. We should probably clean it up by changing the core code to supply something like irq_enter_we_are_already_in_context_kernel.

Quoted 4 lines

Quoted 1 line

Thoughts?

So assuming you fix the UML build I'm inclined to go for it, even in this incomplete form, to increase testing coverage.

Andy, can you please share the build breakage you're facing? I'll happily help you fixing it.

Basically there would be a single C function we'd call, which returns a condition (or fixes up its return address on the stack directly) to determine between the SYSRET and IRET return paths.

Quoted 18 lines · Ingo Molnar

Quoted 15 lines · Andy Lutomirski

This is separate for ease of bisection.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S | 68 +++++------------------------------------------
 1 file changed, 7 insertions(+), 61 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index 33acc3dcc281..a5044d7a9d43 100644
@@ -229,6 +229,11 @@ entry_SYSCALL_64_fastpath:
     */
    USERGS_SYSRET64
 

Quoted 7 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 235

Quoted 4 lines

arch/x86/entry/entry_64.S +4 -0

+GLOBAL(int_ret_from_sys_call_irqs_off)
+   TRACE_IRQS_ON
+   ENABLE_INTERRUPTS(CLBR_NONE)
+   jmp int_ret_from_sys_call

Any reason why irq state tracking cannot be done in C as well, like the rest of the irq state tracking code?

Never mind, I see you've done exactly that in patch #12.

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Quoted 44 lines · Ingo Molnar

Quoted 3 lines · Andy Lutomirski

This is incomplete, but it's finally good enough that I think it's time to get other opinions on it. It is a complete rewrite of the slow path code that handles exits to user mode.

Modulo the small comments I made about the debug checks interface plus naming details the structure and intention of this series gives me warm fuzzy feelings.

Quoted 9 lines

The exit-to-usermode code is copied in several places and is written in a nasty combination of asm and C. It's not at all clear what it's supposed to do, and the way it's structured makes it very hard to work with. For example, it's not even clear why syscall exit hooks are called only once per syscall right now. (It seems to be a side effect of the way that rdi and rdx are handled in the asm loop, and it seems reliable, but it's still pointlessly complicated.) The existing code also makes context tracking overly complicated and hard to understand. Finally, it's nearly impossible for anyone to change what happens on exit to usermode, since the existing code is so fragile.

Amen.

Quoted 2 lines

I tried to clean it up incrementally, but I decided it was too hard. Instead, this series just replaces the code. It seems to work.

Any known bugs beyond UML build breakage?

Quoted 17 lines

Context tracking in particular works very differently now. The low-level entry code checks that we're in CONTEXT_USER and switches to CONTEXT_KERNEL. The exit code does the reverse. There is no need to track what CONTEXT_XYZ state we came from, because we already know. Similarly, SCHEDULE_USER is gone, since we can reschedule if needed by simply calling schedule() from C code.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Because I haven't converted the 32-bit code yet, all of the now-unnecessary unnecessary calls to exception_enter are still present in traps.c.

IRQ context tracking is still duplicated. We should probably clean it up by changing the core code to supply something like irq_enter_we_are_already_in_context_kernel.

Quoted 4 lines

Quoted 1 line

Thoughts?

So assuming you fix the UML build I'm inclined to go for it, even in this incomplete form, to increase testing coverage.

Andy, can you please share the build breakage you're facing? I'll happily help you fixing it.

Quoted 3 lines · Andy Lutomirski

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

So I'd suggest moving most of the SYSRET fast path to C too.

This is how it looks like now after your patches:

testl $_TIF_WORK_SYSCALL_ENTRY, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
jnz tracesys
entry_SYSCALL_64_fastpath:
#if __SYSCALL_MASK == ~0
cmpq $__NR_syscall_max, %rax
#else
andl $__SYSCALL_MASK, %eax
cmpl $__NR_syscall_max, %eax

#endif
        ja      1f                              /* return -ENOSYS (already in pt_regs->ax) */
        movq    %r10, %rcx
        call    *sys_call_table(, %rax, 8)
        movq    %rax, RAX(%rsp)

1:
/*
* Syscall return path ending with SYSRET (fast path).
* Has incompletely filled pt_regs.
*/
LOCKDEP_SYS_EXIT
/*
* We do not frame this tiny irq-off block with TRACE_IRQS_OFF/ON,
* it is too small to ever cause noticeable irq latency. */
DISABLE_INTERRUPTS(CLBR_NONE)

/*
* We must check ti flags with interrupts (or at least preemption)
* off because we must *never* return to userspace without
* processing exit work that is enqueued if we're preempted here.
* In particular, returning to userspace with any of the one-shot
* flags (TIF_NOTIFY_RESUME, TIF_USER_RETURN_NOTIFY, etc) set is
* very bad.
*/
testl $_TIF_ALLWORK_MASK, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
jnz int_ret_from_sys_call_irqs_off /* Go to the slow path */

Most of that can be done in C.

And I think we could also convert the IRET syscall return slow path to C too:

GLOBAL(int_ret_from_sys_call)
SAVE_EXTRA_REGS
movq %rsp, %rdi
call syscall_return_slowpath /* returns with IRQs disabled */
RESTORE_EXTRA_REGS

/*
* Try to use SYSRET instead of IRET if we're returning to
* a completely clean 64-bit userspace context. */
movq RCX(%rsp), %rcx
movq RIP(%rsp), %r11
cmpq %rcx, %r11 /* RCX == RIP */
jne opportunistic_sysret_failed

/*
* On Intel CPUs, SYSRET with non-canonical RCX/RIP will #GP
* in kernel space. This essentially lets the user take over
* the kernel, since userspace controls RSP.
*
* If width of "canonical tail" ever becomes variable, this will need
* to be updated to remain correct on both old and new CPUs. */
.ifne __VIRTUAL_MASK_SHIFT - 47
.error "virtual address width changed -- SYSRET checks need update" .endif

/* Change top 16 bits to be the sign-extension of 47th bit */
shl $(64 - (__VIRTUAL_MASK_SHIFT+1)), %rcx
sar $(64 - (__VIRTUAL_MASK_SHIFT+1)), %rcx

/* If this changed %rcx, it was not canonical */
cmpq %rcx, %r11
jne opportunistic_sysret_failed

cmpq $__USER_CS, CS(%rsp) /* CS must match SYSRET */
jne opportunistic_sysret_failed

movq R11(%rsp), %r11
cmpq %r11, EFLAGS(%rsp) /* R11 == RFLAGS */
jne opportunistic_sysret_failed

/*
* SYSRET can't restore RF. SYSRET can restore TF, but unlike IRET,
* restoring TF results in a trap from userspace immediately after
* SYSRET. This would cause an infinite loop whenever #DB happens
* with register state that satisfies the opportunistic SYSRET
* conditions. For example, single-stepping this user code:
*
* movq $stuck_here, %rcx
* pushfq
* popq %r11
* stuck_here:
*
* would never get past 'stuck_here'.
*/
testq $(X86_EFLAGS_RF|X86_EFLAGS_TF), %r11
jnz opportunistic_sysret_failed

/* nothing to check for RSP */

cmpq $__USER_DS, SS(%rsp) /* SS must match SYSRET */
jne opportunistic_sysret_failed

/*
* We win! This label is here just for ease of understanding
* perf profiles. Nothing jumps here.
*/
syscall_return_via_sysret:
/* rcx and r11 are already restored (see code above) */ RESTORE_C_REGS_EXCEPT_RCX_R11
movq RSP(%rsp), %rsp
USERGS_SYSRET64

opportunistic_sysret_failed:
SWAPGS
jmp restore_c_regs_and_iret
END(entry_SYSCALL_64)

Basically there would be a single C function we'd call, which returns a condition (or fixes up its return address on the stack directly) to determine between the SYSRET and IRET return paths.

Moving this to C too has immediate benefits: that way we could easily add instrumentation to see how efficient these various return methods are, etc.

I.e. I don't think there's two ways about this: once the entry code moves to the domain of C code, we get the best benefits by moving as much of it as possible.

The only low level bits remaining in assembly will be low level hardware ABI
details: saving registers and restoring registers to the expected format - no 'active' code whatsoever.

Quoted 2 lines · Andy Lutomirski

I tried to clean it up incrementally, but I decided it was too hard. Instead, this series just replaces the code. It seems to work.

Any known bugs beyond UML build breakage?

Quoted 18 lines

Context tracking in particular works very differently now. The low-level entry code checks that we're in CONTEXT_USER and switches to CONTEXT_KERNEL. The exit code does the reverse. There is no need to track what CONTEXT_XYZ state we came from, because we already know. Similarly, SCHEDULE_USER is gone, since we can reschedule if needed by simply calling schedule() from C code.

The main things that are missing are that I haven't done the 32-bit parts (anyone want to help?) and therefore I haven't deleted the old C code. I also think this may break UML for trivial reasons.

Because I haven't converted the 32-bit code yet, all of the now-unnecessary unnecessary calls to exception_enter are still present in traps.c.

IRQ context tracking is still duplicated. We should probably clean it up by changing the core code to supply something like irq_enter_we_are_already_in_context_kernel.

Quoted 1 line

Thoughts?

So assuming you fix the UML build I'm inclined to go for it, even in this incomplete form, to increase testing coverage.

Doing that will also decrease ongoing merge friction between your work and other entry code cleanups ...

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
 	if (context_tracking_is_enabled())
 		__context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+	rcu_lockdep_assert(!context_tracking_is_enabled() ||
+			   this_cpu_read(context_tracking.state) == state,
+			   "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
	WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

	context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Thanks,

Ingo

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

The assert macro avoids all these problems despite being a bit ugly.

Quoted 29 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
 	if (context_tracking_is_enabled())
 		__context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+	rcu_lockdep_assert(!context_tracking_is_enabled() ||
+			   this_cpu_read(context_tracking.state) == state,
+			   "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

Quoted 1 line

(And RCU should be fixed too I suspect.)

The thought is to rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() by analogy to WARN()? Easy to do if so! Or am I missing the point?

Quoted 3 lines

The only low level bits remaining in assembly will be low level hardware ABI
details: saving registers and restoring registers to the expected format - no 'active' code whatsoever.

I think this is true for syscalls. Getting the weird special cases (IRET and GS fault) for error_entry to work correctly in C could be tricky.

Quoted 5 lines

So assuming you fix the UML build I'm inclined to go for it, even in this incomplete form, to increase testing coverage.

Doing that will also decrease ongoing merge friction between your work and other entry code cleanups ...

Sounds good to me. I'm not convinced this is 4.2 material, though. Would it go in a separate branch for now?

On a related note, do you have any idea what work_notifysig_v86 in entry_32.S is for? It seems unnecessary and wrong to me. Unnecessary because we have return_to_32bit. Wrong because I don't see how we can reliably enter vm86 mode if we have exit work enabled -- one of the giant turds in vm86_32.c literally jumps from C code to resume_userspace on vm86 entry, and resume_userspace promptly checks for work and might land in work_notifysig_v86 before we ever make it to v8086/user mode.

I think it may actually be impossible to use vm86 under ptrace. ISTR I had some trouble when trying to strace my test case...

Quoted 4 lines

Quoted 2 lines

Any reason why irq state tracking cannot be done in C as well, like the rest of the irq state tracking code?

Never mind, I see you've done exactly that in patch #12.

There are still some TRACE_IRQS_ON, LOCKDEP_SYS_EXIT, and such scattered throughout the asm. it's plausible that even more of that could be moved to C.

We could also benchmark and find out how bad it would be if we just always filled pt_regs in completely in syscalls. If the performance hit isn't enough to matter, then we could potentially move the entire syscall path except pt_regs setup and sysret/iret into three C functions.

Quoted 9 lines · Andy Lutomirski

Quoted 4 lines

Quoted 2 lines

Any reason why irq state tracking cannot be done in C as well, like the rest of the irq state tracking code?

Never mind, I see you've done exactly that in patch #12.

There are still some TRACE_IRQS_ON, LOCKDEP_SYS_EXIT, and such scattered throughout the asm. it's plausible that even more of that could be moved to C.

Quoted 4 lines

We could also benchmark and find out how bad it would be if we just always filled pt_regs in completely in syscalls. If the performance hit isn't enough to matter, then we could potentially move the entire syscall path except pt_regs setup and sysret/iret into three C functions.

The thing is, I'd not be against simplifying pt_regs handling even if it slows down things a tiny bit. If anyone wants to reintroduce that complexity we'll see how it looks like in isolation, done cleanly.

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines

The only low level bits remaining in assembly will be low level hardware ABI
details: saving registers and restoring registers to the expected format - no 'active' code whatsoever.

I think this is true for syscalls. Getting the weird special cases (IRET and GS fault) for error_entry to work correctly in C could be tricky.

Correct, and I double checked the IRET fault path yesterday (fixup_bad_iret), and it looks like a straightforward exception handler with limited control flow. It can stay in asm just fine, it seems mostly orthogonal to the rest.

I didn't check the GS fault path, but that only affects 32-bit, as we use SWAPGS on 64-bit, right? In any case, that code too (32-bit RESTORE_REGS) belongs into the natural 'hardware ABI preparation code' that should stay in assembly. (Unless I missed some other code that might cause trouble.)

Quoted 48 lines · Andy Lutomirski

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 8 lines

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

Well:

- if # CONFIG_CONTEXT_TRACKING is not se, then CT_WARN_ON() does nothing.

- if CONFIG_CONTEXT_TRACKING=y, but !context_tracking_is_enabled(), then CT_WARN_ON() will evaluate 'cond', but won't calculate it.

- only if CONFIG_CONTEXT_TRACKING=y && context_tracking_is_enabled() should we get as far as ct_state() evaluation.

so I'm not sure I see the problem you are seeing.

Quoted 3 lines

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

My understanding is that if !context_tracking_is_enabled() then the compiler should not even try to evaluate the rest. This is why doing a NULL pointer check like this is safe:

Quoted 76 lines · Ingo Molnar

Quoted 48 lines · Andy Lutomirski

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 8 lines

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

Well:

- if # CONFIG_CONTEXT_TRACKING is not se, then CT_WARN_ON() does nothing.

- if CONFIG_CONTEXT_TRACKING=y, but !context_tracking_is_enabled(), then CT_WARN_ON() will evaluate 'cond', but won't calculate it.

- only if CONFIG_CONTEXT_TRACKING=y && context_tracking_is_enabled() should we get as far as ct_state() evaluation.

so I'm not sure I see the problem you are seeing.

Quoted 3 lines

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

Quoted 3 lines

My understanding is that if !context_tracking_is_enabled() then the compiler should not even try to evaluate the rest. This is why doing a NULL pointer check like this is safe:

I'm fine with everything you just covered. My only objection is that, if ct_state() exists, then someone might call it outside CT_WARN_ON, in which case it will break on non-context-tracking setups.

--Andy

Quoted 77 lines · Andy Lutomirski

Quoted 76 lines · Ingo Molnar

Quoted 48 lines · Andy Lutomirski

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 8 lines

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

Well:

- if # CONFIG_CONTEXT_TRACKING is not se, then CT_WARN_ON() does nothing.

- if CONFIG_CONTEXT_TRACKING=y, but !context_tracking_is_enabled(), then CT_WARN_ON() will evaluate 'cond', but won't calculate it.

- only if CONFIG_CONTEXT_TRACKING=y && context_tracking_is_enabled() should we get as far as ct_state() evaluation.

so I'm not sure I see the problem you are seeing.

Quoted 3 lines

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

Quoted 7 lines

Quoted 3 lines

My understanding is that if !context_tracking_is_enabled() then the compiler should not even try to evaluate the rest. This is why doing a NULL pointer check like this is safe:

I'm fine with everything you just covered. My only objection is that, if ct_state() exists, then someone might call it outside CT_WARN_ON, in which case it will break on non-context-tracking setups.

The more I think about it, the more I dislike ct_state(). We have in_atomic(), which is already problematic because the return value isn't reliable. ct_state(), if callable on non context-tracking kernels, will also be unreliable. I prefer things like lockdep_assert_held because they can't be misused.

It would be far too easy for someone to read:

CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

and add:

if (ct_state() == CONTEXT_KERNEL)
do_something();

and that would be bad.

Quoted 14 lines · Ingo Molnar

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines

The only low level bits remaining in assembly will be low level hardware ABI
details: saving registers and restoring registers to the expected format - no 'active' code whatsoever.

I think this is true for syscalls. Getting the weird special cases (IRET and GS fault) for error_entry to work correctly in C could be tricky.

Correct, and I double checked the IRET fault path yesterday (fixup_bad_iret), and it looks like a straightforward exception handler with limited control flow. It can stay in asm just fine, it seems mostly orthogonal to the rest.

Quoted 4 lines

I didn't check the GS fault path, but that only affects 32-bit, as we use SWAPGS on 64-bit, right? In any case, that code too (32-bit RESTORE_REGS) belongs into the natural 'hardware ABI preparation code' that should stay in assembly. (Unless I missed some other code that might cause trouble.)

Look for "gs_change". To change the gs selector, we do swapgs, then load gs, then swapgs again. If the gs load fails, then we trigger a special fixup.

Quoted 94 lines · Andy Lutomirski

Quoted 77 lines · Andy Lutomirski

Quoted 76 lines · Ingo Molnar

Quoted 48 lines · Andy Lutomirski

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 8 lines

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

Well:

- if # CONFIG_CONTEXT_TRACKING is not se, then CT_WARN_ON() does nothing.

- if CONFIG_CONTEXT_TRACKING=y, but !context_tracking_is_enabled(), then CT_WARN_ON() will evaluate 'cond', but won't calculate it.

- only if CONFIG_CONTEXT_TRACKING=y && context_tracking_is_enabled() should we get as far as ct_state() evaluation.

so I'm not sure I see the problem you are seeing.

Quoted 3 lines

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

Quoted 7 lines

Quoted 3 lines

My understanding is that if !context_tracking_is_enabled() then the compiler should not even try to evaluate the rest. This is why doing a NULL pointer check like this is safe:

I'm fine with everything you just covered. My only objection is that, if ct_state() exists, then someone might call it outside CT_WARN_ON, in which case it will break on non-context-tracking setups.

The more I think about it, the more I dislike ct_state(). We have in_atomic(), which is already problematic because the return value isn't reliable. ct_state(), if callable on non context-tracking kernels, will also be unreliable. I prefer things like lockdep_assert_held because they can't be misused.

It would be far too easy for someone to read:

Quoted 8 lines

CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

and add:

if (ct_state() == CONTEXT_KERNEL)
do_something();

and that would be bad.

But ct_state() could be made reliable: if !context_tracking_is_enabled() then it should return -1 or so.

I.e. we could make it something like:

        enum ctx_state {
                CONTEXT_DISABLED	= -1,
                CONTEXT_KERNEL		=  0,
                CONTEXT_USER		=  1,
                CONTEXT_GUEST		=  2,
        } state;

static inline enum ctx_state ct_state(void)

{
	if (context_tracking_is_enabled())
		return this_cpu_read(context_tracking.state))

	return CONTEXT_DISABLED;
}

and then CT_WARN_ON() DTRT.

Thanks,

Ingo

Quoted 123 lines · Ingo Molnar

Quoted 94 lines · Andy Lutomirski

Quoted 77 lines · Andy Lutomirski

Quoted 76 lines · Ingo Molnar

Quoted 48 lines · Andy Lutomirski

Quoted 47 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
      if (context_tracking_is_enabled())
              __context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+     rcu_lockdep_assert(!context_tracking_is_enabled() ||
+                        this_cpu_read(context_tracking.state) == state,
+                        "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

(And RCU should be fixed too I suspect.)

They are absolutely horrible on the brain when mixed with WARN_ON() interfaces, which are the dominant runtime check interface in the kernel.

Instead make it something like:

  #define ct_state() (this_cpu_read(context_tracking.state))

  #define CT_WARN_ON(cond) \
        WARN_ON(context_tracking_is_enabled() && (cond))

and then the debug checks can be written as:

        CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

This is IMHO _far_ more readable than:

Quoted 8 lines

Quoted 5 lines

        context_tracking_assert_state(CONTEXT_KERNEL);

ok?

(Assuming people will accept 'ct/CT' as an abbreviation for context tracking.)

Hmm, ok I guess. The part I don't like is having ct_state() at all on non-context-tracking kernels -- it seems like it's asking for trouble.

Well:

- if # CONFIG_CONTEXT_TRACKING is not se, then CT_WARN_ON() does nothing.

- if CONFIG_CONTEXT_TRACKING=y, but !context_tracking_is_enabled(), then CT_WARN_ON() will evaluate 'cond', but won't calculate it.

- only if CONFIG_CONTEXT_TRACKING=y && context_tracking_is_enabled() should we get as far as ct_state() evaluation.

so I'm not sure I see the problem you are seeing.

Quoted 3 lines

We could make CT_WARN_ON not even evaluate its argument if !CONFIG_CONTEXT_TRACKING, but then we still have ct_state() returning garbage if !context_tracking_is_enabled().

Quoted 7 lines

Quoted 3 lines

My understanding is that if !context_tracking_is_enabled() then the compiler should not even try to evaluate the rest. This is why doing a NULL pointer check like this is safe:

I'm fine with everything you just covered. My only objection is that, if ct_state() exists, then someone might call it outside CT_WARN_ON, in which case it will break on non-context-tracking setups.

The more I think about it, the more I dislike ct_state(). We have in_atomic(), which is already problematic because the return value isn't reliable. ct_state(), if callable on non context-tracking kernels, will also be unreliable. I prefer things like lockdep_assert_held because they can't be misused.

It would be far too easy for someone to read:

Quoted 8 lines

CT_WARN_ON(ct_state() != CONTEXT_KERNEL);

and add:

if (ct_state() == CONTEXT_KERNEL)
do_something();

and that would be bad.

But ct_state() could be made reliable: if !context_tracking_is_enabled() then it should return -1 or so.

I.e. we could make it something like:

enum ctx_state {
CONTEXT_DISABLED = -1,
CONTEXT_KERNEL = 0,
CONTEXT_USER = 1,
CONTEXT_GUEST = 2,
} state;

static inline enum ctx_state ct_state(void)
{
if (context_tracking_is_enabled())
return this_cpu_read(context_tracking.state))

Quoted 4 lines

      return CONTEXT_DISABLED;
}

and then CT_WARN_ON() DTRT.

That sounds good. With good layout of these things, the compiler should still be able to nop related code when context tracking is disabled.

Quoted 5 lines

Thanks,

Ingo

Quoted 30 lines · Paul E. McKenney

Quoted 29 lines · Ingo Molnar

Quoted 13 lines · Andy Lutomirski

This will let us sprinkle sanity checks around the kernel without making too much of a mess.

Signed-off-by Andy Lutomirski <[email protected]>

---
 include/linux/context_tracking.h | 8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/context_tracking.h +0 -0

index 2821838256b4..0fbea4b152e1 100644

include/linux/context_tracking.h static inline void context_tracking_task_switch(struct task_struct *prev, · line 66

Quoted 10 lines

include/linux/context_tracking.h +7 -0

@@ -57,6 +57,13 @@ static inline void context_tracking_task_switch(struct task_struct *prev,
 	if (context_tracking_is_enabled())
 		__context_tracking_task_switch(prev, next);
 }
+
+static inline void context_tracking_assert_state(enum ctx_state state)
+{
+	rcu_lockdep_assert(!context_tracking_is_enabled() ||
+			   this_cpu_read(context_tracking.state) == state,
+			   "context tracking state was wrong");
+}

Please don't introduce assert() style debug check interfaces!

Quoted 4 lines

Quoted 1 line

(And RCU should be fixed too I suspect.)

The thought is to rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() by analogy to WARN()? Easy to do if so! Or am I missing the point?

Yeah, and inverting the condition. Assuming the condition was assert()-style inverted to begin with! :-)

and lockdep should be fixed too I suspect, lockdep_assert_held() was really a poorly chosen name I suspect, it should be 'lockdep_check_held()' or so? It has very little to do with the assert() interface.

Quoted 1 line · Ingo Molnar

* Paul E. McKenney <[email protected]> wrote:

Quoted 5 lines

Quoted 2 lines

The thought is to rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() by analogy to WARN()? Easy to do if so! Or am I missing the point?

Yeah, and inverting the condition. Assuming the condition was assert()-style inverted to begin with! :-)

It appears to have been. ;-)

Please see below for an untested patch. I made this be one big patch, but could have one patch add RCU_LOCKDEP_WARN(), a series convert uses from rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove rcu_lockdep_assert(). Let me know!

Quoted 3 lines

and lockdep should be fixed too I suspect, lockdep_assert_held() was really a poorly chosen name I suspect, it should be 'lockdep_check_held()' or so? It has very little to do with the assert() interface.

Quoted 2 lines

Yeah, and inverting the condition. Assuming the condition was assert()-style inverted to begin with! :-)

It appears to have been. ;-)

Please see below for an untested patch. I made this be one big patch, but could have one patch add RCU_LOCKDEP_WARN(), a series convert uses from rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove rcu_lockdep_assert(). Let me know!

Quoted 9 lines · Paul E. McKenney

Quoted 2 lines

Yeah, and inverting the condition. Assuming the condition was assert()-style inverted to begin with! :-)

It appears to have been. ;-)

Please see below for an untested patch. I made this be one big patch, but could have one patch add RCU_LOCKDEP_WARN(), a series convert uses from rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove rcu_lockdep_assert(). Let me know!

One big patch is perfect I think - it's a rename and a relatively mechanic inversion of conditions, no point in splitting it up any more IMHO. (But it's your call really.)

So I had a quick look at this patch, and IMHO the RCU_LOCKDEP_WARN() lines read a lot more 'naturally', because the new, inverted conditions now highlight buggy scenarios - which has the same logic parity as the kernel's historic WARN_ON()/BUG_ON() patterns:

Reviewed-by Ingo Molnar <[email protected]>

This one looked a bit weird:

kernel/rcu/update.c EXPORT_SYMBOL_GPL(call_rcu_tasks); · line 594

Quoted 12 lines

index a0a0dd03c73a..47268fb1d27b 100644

kernel/rcu/update.c +2 -2

@@ -589,8 +589,8 @@ EXPORT_SYMBOL_GPL(call_rcu_tasks);
 void synchronize_rcu_tasks(void)
 {
 	/* Complain if the scheduler has not started.  */
-	rcu_lockdep_assert(!rcu_scheduler_active,
-			   "synchronize_rcu_tasks called too soon");
+	RCU_LOCKDEP_WARN(rcu_scheduler_active,
+			 "synchronize_rcu_tasks called too soon");
 

So I'd assume that a flag called 'rcu_scheduler_active' would be 1 if the RCU scheduler is active.

So why do we warn on it being active? Shouldn't the condition be:

	RCU_LOCKDEP_WARN(!rcu_scheduler_active,
			 "synchronize_rcu_tasks called too soon");

I.e. we warn when the RCU scheduler is not yet active and we called synchronize_rcu_tasks() too soon?

So either the original assert() was wrong, or I'm missing something obvious?