Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/core: Minor optimize ttwu_runnable() | Date | Mon, 07 Nov 2022 12:56:55 +0000 |
| |
On 05/11/22 09:34, Chengming Zhou wrote: > On 2022/11/5 02:19, Valentin Schneider wrote: >> On 02/11/22 18:23, Chengming Zhou wrote: >>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >>> index 87c9cdf37a26..3785418de127 100644 >>> --- a/kernel/sched/core.c >>> +++ b/kernel/sched/core.c >>> @@ -3718,9 +3718,8 @@ static int ttwu_runnable(struct task_struct *p, int wake_flags) >>> >>> rq = __task_rq_lock(p, &rf); >>> if (task_on_rq_queued(p)) { >>> - /* check_preempt_curr() may use rq clock */ >>> - update_rq_clock(rq); >>> - ttwu_do_wakeup(rq, p, wake_flags, &rf); >>> + WRITE_ONCE(p->__state, TASK_RUNNING); >>> + trace_sched_wakeup(p); >> >> This also loses the class->task_woken() call, AFAICT we could have >> !p->on_cpu here (e.g. an IRQ hit before the task got to schedule()), but >> then again if there is a need to push we should have done that at the IRQ >> preempt via set_next_task_{rt, dl}()... So I'm starting to think this is >> OK, but that needs elaborating in the changelog. > > Sorry, I don't get why we could have !p->on_cpu here? > > I thought if we have task_on_rq_queued(p) here, it means p hasn't got to > __schedule() to deactivate_task(), so p should still be on_cpu? > > Thanks. >
So p is still on the rq for sure, but it may not be the currently running task. Consider, on a CONFIG_PREEMPT kernel, task p0 in a wait loop:
0 for (;;) { 1 set_current_state(TASK_UNINTERRUPTIBLE); 2 3 if (CONDITION) 4 break; 5 6 schedule(); 7 } 8 __set_current_state(TASK_RUNNING);
Now further consider p0 executes line 1, but then gets interrupted by an IRQ, at the end of which preempt_schedule_irq() happens. We enter __schedule(SM_PREEMPT), so p0 isn't dequeued due to its non-zero task state, but we *can* end up switching the CPU's current task.
ISTR that's why Peter renamed that function ttwu_*runnable*() and not ttwu_*running*().
>> >> >>> ret = 1; >>> } >>> __task_rq_unlock(rq, &rf); >>> -- >>> 2.37.2 >>
| |