From: Vincent Guittot <vincent.guittot@linaro.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Yuyang Du <yuyang.du@intel.com>, Ingo Molnar <mingo@kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Mike Galbraith <umgwanakikbuti@gmail.com>,
Benjamin Segall <bsegall@google.com>,
Paul Turner <pjt@google.com>,
Morten Rasmussen <morten.rasmussen@arm.com>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Matt Fleming <matt@codeblueprint.co.uk>
Subject: Re: [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork()
Date: Fri, 17 Jun 2016 15:58:37 +0200 [thread overview]
Message-ID: <CAKfTPtAuh2=V5PXFc0jL7m1FphUAM-O0P=3wya7NYXLF0JVtcw@mail.gmail.com> (raw)
In-Reply-To: <20160617120454.080767343@infradead.org>
On 17 June 2016 at 14:01, Peter Zijlstra <peterz@infradead.org> wrote:
> From: Vincent Guittot <vincent.guittot@linaro.org>
>
> A new fair task is detached and attached from/to task_group with:
>
> cgroup_post_fork()
> ss->fork(child) := cpu_cgroup_fork()
> sched_move_task()
> task_move_group_fair()
>
> Which is wrong, because at this point in fork() the task isn't fully
> initialized and it cannot 'move' to another group, because its not
> attached to any group as yet.
>
> In fact, cpu_cgroup_fork needs a small part of sched_move_task so we
> can just call this small part directly instead sched_move_task. And
> the task doesn't really migrate because it is not yet attached so we
> need the sequence:
>
> do_fork()
> sched_fork()
> __set_task_cpu()
>
> cgroup_post_fork()
> set_task_rq() # set task group and runqueue
>
> wake_up_new_task()
> select_task_rq() can select a new cpu
> __set_task_cpu
> post_init_entity_util_avg
> attach_task_cfs_rq()
> activate_task
> enqueue_task
>
> This patch makes that happen.
>
With this patch and patch 1, the fork sequence looks correct in my test
> Maybe-Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
You can remove the Maybe if you want
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> kernel/sched/core.c | 67 ++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 47 insertions(+), 20 deletions(-)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7743,27 +7743,17 @@ void sched_offline_group(struct task_gro
> spin_unlock_irqrestore(&task_group_lock, flags);
> }
>
> -/* change task's runqueue when it moves between groups.
> - * The caller of this function should have put the task in its new group
> - * by now. This function just updates tsk->se.cfs_rq and tsk->se.parent to
> - * reflect its new group.
> +/*
> + * Set task's runqueue and group.
> + *
> + * In case of a move between group, we update src and dst group thanks to
> + * sched_class->task_move_group. Otherwise, we just need to set runqueue and
> + * group pointers. The task will be attached to the runqueue during its wake
> + * up.
> */
> -void sched_move_task(struct task_struct *tsk)
> +static void sched_set_group(struct task_struct *tsk, bool move)
> {
> struct task_group *tg;
> - int queued, running;
> - struct rq_flags rf;
> - struct rq *rq;
> -
> - rq = task_rq_lock(tsk, &rf);
> -
> - running = task_current(rq, tsk);
> - queued = task_on_rq_queued(tsk);
> -
> - if (queued)
> - dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
> - if (unlikely(running))
> - put_prev_task(rq, tsk);
>
> /*
> * All callers are synchronized by task_rq_lock(); we do not use RCU
> @@ -7776,11 +7766,37 @@ void sched_move_task(struct task_struct
> tsk->sched_task_group = tg;
>
> #ifdef CONFIG_FAIR_GROUP_SCHED
> - if (tsk->sched_class->task_move_group)
> + if (move && tsk->sched_class->task_move_group)
> tsk->sched_class->task_move_group(tsk);
> else
> #endif
> set_task_rq(tsk, task_cpu(tsk));
> +}
> +
> +/*
> + * Change task's runqueue when it moves between groups.
> + *
> + * The caller of this function should have put the task in its new group by
> + * now. This function just updates tsk->se.cfs_rq and tsk->se.parent to reflect
> + * its new group.
> + */
> +void sched_move_task(struct task_struct *tsk)
> +{
> + int queued, running;
> + struct rq_flags rf;
> + struct rq *rq;
> +
> + rq = task_rq_lock(tsk, &rf);
> +
> + running = task_current(rq, tsk);
> + queued = task_on_rq_queued(tsk);
> +
> + if (queued)
> + dequeue_task(rq, tsk, DEQUEUE_SAVE | DEQUEUE_MOVE);
> + if (unlikely(running))
> + put_prev_task(rq, tsk);
> +
> + sched_set_group(tsk, true);
>
> if (unlikely(running))
> tsk->sched_class->set_curr_task(rq);
> @@ -8208,9 +8224,20 @@ static void cpu_cgroup_css_free(struct c
> sched_free_group(tg);
> }
>
> +/*
> + * This is called before wake_up_new_task(), therefore we really only
> + * have to set its group bits, all the other stuff does not apply.
> + */
> static void cpu_cgroup_fork(struct task_struct *task)
> {
> - sched_move_task(task);
> + struct rq_flags rf;
> + struct rq *rq;
> +
> + rq = task_rq_lock(task, &rf);
> +
> + sched_set_group(task, false);
> +
> + task_rq_unlock(rq, task, &rf);
> }
>
> static int cpu_cgroup_can_attach(struct cgroup_taskset *tset)
>
>
next prev parent reply other threads:[~2016-06-17 13:59 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-17 12:01 [PATCH 0/4] sched/fair: Fix PELT wobblies Peter Zijlstra
2016-06-17 12:01 ` [PATCH 1/4] sched: Optimize fork() paths Peter Zijlstra
2016-06-17 12:01 ` [PATCH 2/4] sched/fair: Fix PELT integrity for new groups Peter Zijlstra
2016-06-17 13:51 ` Vincent Guittot
2016-06-17 12:01 ` [PATCH 3/4] sched,cgroup: Fix cpu_cgroup_fork() Peter Zijlstra
2016-06-17 13:58 ` Vincent Guittot [this message]
2016-06-17 14:06 ` Peter Zijlstra
2016-06-17 12:01 ` [PATCH 4/4] sched,fair: Fix PELT integrity for new tasks Peter Zijlstra
2016-06-17 14:09 ` Vincent Guittot
2016-06-17 14:28 ` Peter Zijlstra
2016-06-17 16:02 ` Peter Zijlstra
2016-06-17 16:14 ` Vincent Guittot
2016-06-17 16:18 ` Peter Zijlstra
2016-06-19 22:55 ` Yuyang Du
2016-06-20 9:23 ` Vincent Guittot
2016-06-20 9:52 ` Peter Zijlstra
2016-06-20 10:07 ` Vincent Guittot
2016-06-21 11:43 ` Peter Zijlstra
2016-06-21 12:36 ` Vincent Guittot
2016-06-21 12:47 ` Peter Zijlstra
2016-06-21 12:56 ` Vincent Guittot
2016-06-20 11:35 ` Dietmar Eggemann
2016-06-20 12:35 ` Vincent Guittot
2016-06-20 14:49 ` Dietmar Eggemann
2016-06-21 8:41 ` Peter Zijlstra
2016-06-21 4:51 ` Yuyang Du
2016-06-24 13:03 ` Peter Zijlstra
2016-08-09 23:19 ` Yuyang Du
2016-06-21 13:17 ` Peter Zijlstra
2016-06-21 13:29 ` Vincent Guittot
2016-06-22 11:46 ` Peter Zijlstra
2016-06-23 15:35 ` Dietmar Eggemann
2016-06-23 17:15 ` Peter Zijlstra
2016-06-20 14:27 ` Peter Zijlstra
2016-06-23 11:19 ` Peter Zijlstra
2016-08-01 7:30 ` Wanpeng Li
2016-08-01 9:31 ` Mike Galbraith
2016-08-01 9:56 ` Wanpeng Li
2016-08-01 11:52 ` Mike Galbraith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKfTPtAuh2=V5PXFc0jL7m1FphUAM-O0P=3wya7NYXLF0JVtcw@mail.gmail.com' \
--to=vincent.guittot@linaro.org \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=mingo@kernel.org \
--cc=morten.rasmussen@arm.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=umgwanakikbuti@gmail.com \
--cc=yuyang.du@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).