Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes: > On Sun, Apr 02, 2023 at 10:22:32PM -0700, Ankur Arora wrote: >> Allow threads marked TIF_ALLOW_RESCHED to be rescheduled in irqexit. >> >> This is only necessary under !preempt_model_preemptible() for which >> we reuse the same logic as irqentry_exit_code_resched(). >> >> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx> >> --- >> kernel/entry/common.c | 8 ++++++++ >> kernel/sched/core.c | 36 +++++++++++++++++++++--------------- >> 2 files changed, 29 insertions(+), 15 deletions(-) >> >> diff --git a/kernel/entry/common.c b/kernel/entry/common.c >> index be61332c66b5..f1005595ebe7 100644 >> --- a/kernel/entry/common.c >> +++ b/kernel/entry/common.c >> @@ -390,6 +390,9 @@ void raw_irqentry_exit_cond_resched(void) >> preempt_schedule_irq(); >> } >> } >> + >> +void irqentry_exit_allow_resched(void) __alias(raw_irqentry_exit_cond_resched); > > Because typing raw_irqentry_exit_cond_resched() was too much effort? Probably unnecessary but wanted to underscore that irqentry_exit_allow_resched() handled a separate user path. >> + >> #ifdef CONFIG_PREEMPT_DYNAMIC >> #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL) >> DEFINE_STATIC_CALL(irqentry_exit_cond_resched, raw_irqentry_exit_cond_resched); >> @@ -431,6 +434,11 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state) >> instrumentation_begin(); >> if (IS_ENABLED(CONFIG_PREEMPTION)) >> irqentry_exit_cond_resched(); >> + /* >> + * We care about this clause only in the dynamic !preemptible case. >> + */ >> + if (unlikely(!preempt_model_preemptible() && resched_allowed())) >> + irqentry_exit_allow_resched(); > > This is wrong, if we have dynamic preemption then we have > CONFIG_PREEMPTION and we'll have that irqentry_exit_cond_resched() call. > > Basically what you've written above is something like: > > static_call(foo); // raw_foo() when A, NOP if !A Right, so: CONFIG_PREEMPTION: raw_foo() CONFIG_PREEMPTION && (preempt_dynamic_mode == preempt_dynamic_full): raw_foo() This is NOP if CONFIG_PREEMPTION && preempt_dynamic_mode != preempt_dynamic_full. > if (!A) > raw_foo(); So I would call raw_foo() for the CONFIG_PREEMPTION=n case. > What you really care about is the CONFIG_PREEMPTION=n case, but that you > don't actually handle. I don't see that. The CONFIG_PREEMPTION=n (or its dynamic version) is being handled here. Ignoring the RT case, preempt_model_preemptible() is either: IS_ENABLED(CONFIG_PREEMPT) or: preempt_dynamic_mode == preempt_dynamic_full So, I'm handling both the static and the dynamic case here. if (!IS_ENABLED(CONFIG_PREEMPTION)) raw_foo() or if (preempt_dynamic_mode != preempt_dynamic_full) raw_foo() > And yeah, you've got the extra resched_allowed() thing in there, but > that doesn't make it any better -- this is all quite horrible. I don't disagree. There's a quite a lot of static/dynamic config options here and adding this clause didn't improve things. I think just going with a static call here for the allow-resched case might have been cleaner. Alternately I'll see if it can be cleanly folded in with the irqentry_exit_cond_resched() logic. >> /* Covers both tracing and lockdep */ >> trace_hardirqs_on(); >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index 0d18c3969f90..11845a91b691 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c > >> @@ -8597,28 +8599,32 @@ EXPORT_SYMBOL(__cond_resched_rwlock_write); >> * SC:preempt_schedule >> * SC:preempt_schedule_notrace >> * SC:irqentry_exit_cond_resched >> + * SC:irqentry_exit_allow_resched >> * >> * >> * NONE: >> - * cond_resched <- __cond_resched >> - * might_resched <- RET0 >> - * preempt_schedule <- NOP >> - * preempt_schedule_notrace <- NOP >> - * irqentry_exit_cond_resched <- NOP >> + * cond_resched <- __cond_resched >> + * might_resched <- RET0 >> + * preempt_schedule <- NOP >> + * preempt_schedule_notrace <- NOP >> + * irqentry_exit_cond_resched <- NOP >> + * irqentry_exit_allow_resched <- irqentry_exit_allow_resched >> * >> * VOLUNTARY: >> - * cond_resched <- __cond_resched >> - * might_resched <- __cond_resched >> - * preempt_schedule <- NOP >> - * preempt_schedule_notrace <- NOP >> - * irqentry_exit_cond_resched <- NOP >> + * cond_resched <- __cond_resched >> + * might_resched <- __cond_resched >> + * preempt_schedule <- NOP >> + * preempt_schedule_notrace <- NOP >> + * irqentry_exit_cond_resched <- NOP >> + * irqentry_exit_allow_resched <- irqentry_exit_allow_resched >> * >> * FULL: >> - * cond_resched <- RET0 >> - * might_resched <- RET0 >> - * preempt_schedule <- preempt_schedule >> - * preempt_schedule_notrace <- preempt_schedule_notrace >> - * irqentry_exit_cond_resched <- irqentry_exit_cond_resched >> + * cond_resched <- RET0 >> + * might_resched <- RET0 >> + * preempt_schedule <- preempt_schedule >> + * preempt_schedule_notrace <- preempt_schedule_notrace >> + * irqentry_exit_cond_resched <- irqentry_exit_cond_resched >> + * irqentry_exit_allow_resched <- NOP >> */ > > This ^ is all complete nonsense.. irqentry_exit_allow_resched() is not > a static call. It's an alias of raw_irqentry_exit_cond_resched which > circumvents the static call entirely. Yeah you are right. I added the SC:irqentry_exit_allow_resched prematurely. Will fix. Thanks for the detailed comments. -- ankur
