From: Kirill Tkhai <ktkhai@parallels.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Kirill Tkhai <tkhai@yandex.ru>, Oleg Nesterov <oleg@redhat.com>,
<linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@redhat.com>,
Burke Libbey <burke.libbey@shopify.com>,
Vladimir Davydov <vdavydov@parallels.com>
Subject: Re: [PATCH] sched: Fix race between task_group and sched_task_group
Date: Wed, 29 Oct 2014 14:13:02 +0300 [thread overview]
Message-ID: <1414581182.8574.66.camel@tkhai> (raw)
In-Reply-To: <20141029091640.GW3337@twins.programming.kicks-ass.net>
В Ср, 29/10/2014 в 10:16 +0100, Peter Zijlstra пишет:
> On Wed, Oct 29, 2014 at 06:20:48AM +0300, Kirill Tkhai wrote:
> > > And cgroup_task_migrate() can free ->cgroups via call_rcu(). Of course,
> > > in practice raw_spin_lock_irq() should also act as rcu_read_lock(), but
> > > we should not rely on implementation details.
> >
> > Do you mean cgroup_task_migrate()->put_css_set_locked()? It's not
> > possible there, because old_cset->refcount is lager than 1. We increment
> > it in cgroup_migrate_add_src() and real freeing happens in
> > cgroup_migrate_finish(). These functions are around task_migrate(), they
> > are pair brackets.
> >
> > > task_group = tsk->cgroups[cpu_cgrp_id] can't go away because yes, if we
> > > race with migrate then ->attach() was not called. But it seems that in
> > > theory it is not safe to dereference tsk->cgroups.
> >
> > old_cset can't be freed in cgroup_task_migrate(), so we can safely
> > dereference it. If we've got old_cset in
> > cgroup_post_fork()->sched_move_task(), the right sched_task_group will
> > be installed by attach->sched_move_task().
>
>
> Would it be fair to summarise your argument thusly:
>
> "Because sched_move_task() is only called from cgroup_subsys methods
> the cgroup infrastructure itself holds reference on the relevant
> css sets, and therefore their existence is guaranteed."
>
> ?
>
> The question then would be how do we guarantee/assert the assumption
> that sched_move_task() is indeed only ever called from such a method.
I mean the relationship between cgroup_task_migrate() and sched_move_task()
called from anywhere.
cgroup_task_migrate() is the only function which changes task_struct::cgroups.
This function is called only from cgroup_migrate().
(A) (B) (C)
| | |
v v v
cgroup_migrate_add_src()
get_css_set(src_cset)
cgroup_migrate()
cgroup_task_migrate()
old_cset = task_css_set(tsk)
get_css_set(new_cset)
rcu_assign_pointer(tsk->cgroups, new_cset)
/* old_cset.refcount > 1 here */
put_css_set_locked(old_cset)
/* not freed here */
css->ss->attach sched_move_task
cpu_cgroup_attach() task_rq_lock()
sched_move_task()
.... /* Possible use of old_cset */
.... task_rq_unlock()
.... ....
task_rq_lock()
...
task_rq_unlock()
sched_move_task()
task_rq_lock()
/* new_cset is used here */
task_rq_unlock()
cgroup_migrate_finish()
/* Possible freeing here */
put_css_set_locked(src_cset)
Even if (B) uses old_cset and old sched_task_group,
(A) will overwrite it before it's freed.
In case of (A) and (C), (C) reads new_cset, because
task_rq_lock() provides all necessary memory barriers.
Of course, cgroup_migrate_add_src() is used more
complex than I've drawn. But the idea is the same.
next prev parent reply other threads:[~2014-10-29 11:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-27 10:18 [PATCH] sched: Fix race between task_group and sched_task_group Kirill Tkhai
2014-10-27 12:21 ` Peter Zijlstra
2014-10-27 23:04 ` Oleg Nesterov
2014-10-28 5:24 ` Kirill Tkhai
2014-10-28 22:52 ` Oleg Nesterov
2014-10-29 3:20 ` Kirill Tkhai
2014-10-29 9:16 ` Peter Zijlstra
2014-10-29 11:13 ` Kirill Tkhai [this message]
2014-10-29 19:21 ` Oleg Nesterov
2014-11-04 16:07 ` [tip:sched/urgent] sched: Remove lockdep check in sched_move_task () tip-bot for Kirill Tkhai
2014-10-28 11:01 ` [tip:sched/core] sched: Fix race between task_group and sched_task_group tip-bot for Kirill Tkhai
2015-01-26 23:46 ` [PATCH] " Sasha Levin
2015-01-27 8:48 ` Peter Zijlstra
2015-01-27 9:31 ` Peter Zijlstra
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=1414581182.8574.66.camel@tkhai \
--to=ktkhai@parallels.com \
--cc=burke.libbey@shopify.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
--cc=tkhai@yandex.ru \
--cc=vdavydov@parallels.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).