Messages in this thread | | | Date | Mon, 7 Nov 2022 21:19:16 +0800 | Subject | Re: [PATCH] sched/core: Minor optimize ttwu_runnable() | From | Chengming Zhou <> |
| |
On 2022/11/7 20:56, Valentin Schneider wrote: > 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.
Thank you very much for this detailed explanation, I get it. Yes, this process can be seen on CONFIG_PREEMPT kernel.
> > ISTR that's why Peter renamed that function ttwu_*runnable*() and not > ttwu_*running*().
So this task p didn't really sleep, it's preempted, later scheduled in, get to execute line 6 schedule(), but its state has been set to TASK_RUNNING, it won't deactivate_task(p).
Looks like this patch is still reasonable? Should I put this detailed explanation in the changelog and send v2?
Thanks!
> >>> >>> >>>> ret = 1; >>>> } >>>> __task_rq_unlock(rq, &rf); >>>> -- >>>> 2.37.2 >>> >
| |