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

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 <[email protected]>
---
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



2023-04-18 12:21:07

by Thomas Gleixner

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

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

2023-04-18 13:37:20

by Peter Zijlstra

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

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

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

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


Attachments:
(No filename) (1.97 kB)
CONFIG-noinstr.xz (26.69 kB)
Download all attachments