diff mbox series

[RFC] preempt: Put preempt_enable() within an instrumentation*() section.

Message ID 20230309072724.3F6zRkvw@linutronix.de
State New
Headers
Series [RFC] preempt: Put preempt_enable() within an instrumentation*() section. |

Commit Message

Sebastian Andrzej Siewior March 9, 2023, 7:27 a.m. UTC 
  Callers of preempt_enable() can be within an noinstr section leading to:
| vmlinux.o: warning: objtool: native_sched_clock+0x97: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: local_clock+0xb4: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: syscall_enter_from_user_mode+0x140: call to preempt_schedule_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: syscall_enter_from_user_mode_prepare+0xf2: call to preempt_schedule_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: irqentry_enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section

This only happens on CONFIG_PREEMPT+ due to the conditional call into the scheduler.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/preempt.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Thomas Gleixner April 18, 2023, 12:16 p.m. UTC | #1 
On Thu, Mar 09 2023 at 08:27, Sebastian Andrzej Siewior wrote:
> Callers of preempt_enable() can be within an noinstr section leading to:
> | vmlinux.o: warning: objtool: native_sched_clock+0x97: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: local_clock+0xb4: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section

> | vmlinux.o: warning: objtool: enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: syscall_enter_from_user_mode+0x140: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: syscall_enter_from_user_mode_prepare+0xf2: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: irqentry_enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section 

I'm confused where this preempt_enable is in those
*_enter_from_user_mode() functions.

Kernel config and compiler version please.

>  #define preempt_enable() \
>  do { \
>  	barrier(); \
> -	if (unlikely(preempt_count_dec_and_test())) \
> +	if (unlikely(preempt_count_dec_and_test())) { \
> +		instrumentation_begin(); \
>  		__preempt_schedule(); \
> +		instrumentation_end(); \
> +	} \
>  } while (0)

This would paper over a misplaced preempt_disable/enable() pair in
noinstr code. I'm not really happy about that.

Thanks,

        tglx
Peter Zijlstra April 18, 2023, 1:25 p.m. UTC | #2 
On Thu, Mar 09, 2023 at 08:27:24AM +0100, Sebastian Andrzej Siewior wrote:
> Callers of preempt_enable() can be within an noinstr section leading to:
> | vmlinux.o: warning: objtool: native_sched_clock+0x97: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: local_clock+0xb4: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: syscall_enter_from_user_mode+0x140: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: syscall_enter_from_user_mode_prepare+0xf2: call to preempt_schedule_thunk() leaves .noinstr.text section
> | vmlinux.o: warning: objtool: irqentry_enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section
> 
> This only happens on CONFIG_PREEMPT+ due to the conditional call into the scheduler.

I think I see; but I can't reproduce. I use:

  defconfig + CONFIG_PREEMPT + CONFIG_DEBUG_ENTRY

let me try and figure out wtf to do about this :/
Sebastian Andrzej Siewior April 18, 2023, 1:49 p.m. UTC | #3 
On 2023-04-18 14:16:11 [+0200], Thomas Gleixner wrote:
> On Thu, Mar 09 2023 at 08:27, Sebastian Andrzej Siewior wrote:
> > Callers of preempt_enable() can be within an noinstr section leading to:
> > | vmlinux.o: warning: objtool: native_sched_clock+0x97: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> > | vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> > | vmlinux.o: warning: objtool: local_clock+0xb4: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
> 
> > | vmlinux.o: warning: objtool: enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section
> > | vmlinux.o: warning: objtool: syscall_enter_from_user_mode+0x140: call to preempt_schedule_thunk() leaves .noinstr.text section
> > | vmlinux.o: warning: objtool: syscall_enter_from_user_mode_prepare+0xf2: call to preempt_schedule_thunk() leaves .noinstr.text section
> > | vmlinux.o: warning: objtool: irqentry_enter_from_user_mode+0xea: call to preempt_schedule_thunk() leaves .noinstr.text section 
> 
> I'm confused where this preempt_enable is in those
> *_enter_from_user_mode() functions.

These are gone since commit
	f87d28673b71b ("entry: Fix noinstr warning in __enter_from_user_mode()")

which is part of v6.3-rc4.

> Kernel config and compiler version please.

The former three still exist as of v6.3-rc7. I've attached the kernel
config. I'm using gcc version 12.2.0 (Debian 12.2.0-14).

> >  #define preempt_enable() \
> >  do { \
> >  	barrier(); \
> > -	if (unlikely(preempt_count_dec_and_test())) \
> > +	if (unlikely(preempt_count_dec_and_test())) { \
> > +		instrumentation_begin(); \
> >  		__preempt_schedule(); \
> > +		instrumentation_end(); \
> > +	} \
> >  } while (0)
> 
> This would paper over a misplaced preempt_disable/enable() pair in
> noinstr code. I'm not really happy about that.

okay.

> Thanks,
> 
>         tglx

Sebastian
diff mbox series

Patch

diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 12f59cdaaedda..05338f00a5907 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -238,15 +238,21 @@  do { \
 #define preempt_enable() \
 do { \
 	barrier(); \
-	if (unlikely(preempt_count_dec_and_test())) \
+	if (unlikely(preempt_count_dec_and_test())) { \
+		instrumentation_begin(); \
 		__preempt_schedule(); \
+		instrumentation_end(); \
+	} \
 } while (0)
 
 #define preempt_enable_notrace() \
 do { \
 	barrier(); \
-	if (unlikely(__preempt_count_dec_and_test())) \
+	if (unlikely(__preempt_count_dec_and_test())) { \
+		instrumentation_begin(); \
 		__preempt_schedule_notrace(); \
+		instrumentation_end(); \
+	} \
 } while (0)
 
 #define preempt_check_resched() \