Messages in this thread | | | Date | Fri, 16 Jun 2023 15:56:25 +0800 | Subject | Re: [External] Re: [PATCH v5 2/4] sched/core: Avoid double calling update_rq_clock() in __balance_push_cpu_stop() | From | Hao Jia <> |
| |
On 2023/6/15 Peter Zijlstra wrote: > On Tue, Jun 13, 2023 at 04:20:10PM +0800, Hao Jia wrote: >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index a8be5415daba..1eca36299d8b 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -2398,7 +2398,6 @@ static struct rq *__migrate_task(struct rq *rq, struct rq_flags *rf, >> if (!is_cpu_allowed(p, dest_cpu)) >> return rq; >> >> - update_rq_clock(rq); >> rq = move_queued_task(rq, rf, p, dest_cpu); >> >> return rq; >> @@ -2456,10 +2455,12 @@ static int migration_cpu_stop(void *data) >> goto out; >> } >> >> - if (task_on_rq_queued(p)) >> + if (task_on_rq_queued(p)) { >> + update_rq_clock(rq); >> rq = __migrate_task(rq, &rf, p, arg->dest_cpu); >> - else >> + } else { >> p->wake_cpu = arg->dest_cpu; >> + } >> >> /* >> * XXX __migrate_task() can fail, at which point we might end > > So now you've got update_rq_clock() in both callers, why not remove it > from __balance_push_cpu_stop() ? > > Afaict nothing actually needs it before __migrate_task(). >
Thanks for your review. I'm afraid not, the rq clock also needs to be updated before select_fallback_rq() is called.
> --- > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -9435,8 +9435,6 @@ static int __balance_push_cpu_stop(void > raw_spin_lock_irq(&p->pi_lock); > rq_lock(rq, &rf); > > - update_rq_clock(rq); > - > if (task_rq(p) == rq && task_on_rq_queued(p)) { > cpu = select_fallback_rq(rq->cpu, p); > rq = __migrate_task(rq, &rf, p, cpu);
If we just remove update_rq_clock() from __balance_push_cpu_stop(), we will get this warning.
[ 1260.960166] rq->clock_update_flags < RQCF_ACT_SKIP [ 1260.960170] WARNING: CPU: 25 PID: 196 at kernel/sched/sched.h:1496 update_curr+0xf6/0x1f0
[ 1260.960318] Call Trace: [ 1260.960320] <TASK> [ 1260.960323] ? show_regs+0x5b/0x60 [ 1260.960327] ? __warn+0x89/0x140 [ 1260.960331] ? update_curr+0xf6/0x1f0 [ 1260.960334] ? report_bug+0x1b7/0x1e0 [ 1260.960338] ? __wake_up_klogd.part.25+0x5a/0x80 [ 1260.960342] ? handle_bug+0x45/0x80 [ 1260.960346] ? exc_invalid_op+0x18/0x70 [ 1260.960348] ? asm_exc_invalid_op+0x1b/0x20 [ 1260.960354] ? update_curr+0xf6/0x1f0 [ 1260.960356] ? update_curr+0xf6/0x1f0 [ 1260.960359] dequeue_entity+0x3b/0x410 [ 1260.960361] dequeue_task_fair+0xc7/0x3c0 [ 1260.960363] dequeue_task+0x30/0xf0 [ 1260.960365] __do_set_cpus_allowed+0x94/0x130 [ 1260.960366] do_set_cpus_allowed+0x38/0x60 [ 1260.960368] cpuset_cpus_allowed_fallback+0x70/0x80 [ 1260.960372] select_fallback_rq+0x20f/0x250 <---- [ 1260.960374] __balance_push_cpu_stop+0x13f/0x1a0 [ 1260.960377] ? migration_cpu_stop+0x2b0/0x2b0 [ 1260.960379] cpu_stopper_thread+0xaf/0x140 [ 1260.960382] smpboot_thread_fn+0x158/0x220 [ 1260.960387] ? sort_range+0x30/0x30 [ 1260.960390] kthread+0xfe/0x130 [ 1260.960392] ? kthread_complete_and_exit+0x20/0x20 [ 1260.960394] ret_from_fork+0x1f/0x30 [ 1260.960399] </TASK>
Thanks, Hao
| |