Skip to content

stop the unbound recursion in preempt_schedule_context()

Oleg Nesterov lkml 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest1 patch 1 replyIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/1stop the unbound recursion in preempt_schedule_context()arch/x86/include/asm/preempt.h kernel/context_tracking.c kernel/sched/core.c+42 -400 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [PATCH 0/1] stop the unbound recursion in preempt_schedule_context()
Quoted 19 lines · Oleg Nesterov
Quoted 17 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds

The real fix would appear to be to use "preempt_enable_no_resched_notrace()", which your patch did, but without the loop.

Actually, the real fix would be to not be stupid, and just make the code do something like

Quoted 6 lines
       if (likely(!preemptible()))
               return;

        __preempt_count_add(PREEMPT_ACTIVE);
        prev_ctx = exception_enter();
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
        __schedule();

        exception_exit(prev_ctx);
        __preempt_count_sub(PREEMPT_ACTIVE);

and *not* enable preemption around the scheduling at all.

Yes, I think you are right. And I hate to admit that I didn't think about this simplification. The only complication is that we should move this function from context_tracking.c to sched/core.c.

However, I still think we need the need_resched() loop, we can't do this only once.

And in fact the simplest fix could just turn it into

	local_irq_disable();
	preempt_schedule_irq();
	local_irq_enable();
Quoted 3 lines

Again, it is too late for me... Most probably I am wrong, but somehow it seems to me that the real fix should try to kill preempt_schedule_context() altogether and teach preempt_schedule() to play well with CONTEXT_TRACKING.

Yes, the very fact that

	preempt_enable() != trace_preempt_on() + preempt_enable_notrace()

looks simply wrong imo. And preempt_enable_notrace() has users outside of the tracing code which can run in IN_USER state. For example, why should __perf_sw_event() worry about context_tracking.state?

OTOH, if the caller of preempt_enable_notrace() actually needs to take care of potential IN_USER state, then it probably has other problems. And indeed, ftrace_ops_control_func() has to check rcu_is_watching() anyway.

IOW, imho 29bb9e5a75 "tracing/context-tracking: Add preempt_schedule_context() for tracing" was not a right solution. Perhaps the tracing functions can be changed to switch to IN_KERNEL mode, or at least perhaps we can add the special preempt_disable_ftrace() helper. But this needs another discussion.

Either way, I do think that preempt_schedule_context() in its current form must die.

Oleg.

Read the cover letter in its thread