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

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

[Sebastian Andrzej Siewior]

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

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() \
-- 
2.39.2

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

[Thomas Gleixner]

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

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

[Peter Zijlstra]

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

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

[Sebastian Andrzej Siewior]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

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

[-- Attachment #2: CONFIG-noinstr.xz --]
[-- Type: application/x-xz, Size: 27328 bytes --]