Hi Ben, Kame,
Sorry for the delay in getting to look at this,
On Tue, Aug 3, 2010 at 6:08 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
Marking as releasable should be fine - the only time this is cleared
is when you write to notify_on_release.
I think that the put_css_set(oldcg) and synchronize_rcu() is safe, by
the following logic:
- we took cgroup_lock in cgroup_tasks_write()
- after this point, oldcgrp was initialized from the task's cgroup;
therefore oldcgrp still existed at this point
- even if all the threads (including the one being operated on) exit
(and hence leave oldcgrp) while we're doing the attach, we're holding
cgroup_lock so no-one else can delete oldcgrp
So it's certainly possible that the task in question has exited by the
time we call the subsys attach methods, but oldcgrp should still be
alive.
Whether we need an additional synchronize_rcu() after the attach()
calls is harder to determine - I guess it's better to be safe than
sorry, unless people are seeing specific performance issues with this.
I think the css_set_check_fetched() function needs more comments
explaining its behaviour and what its return value indicates.
More than that - even if we don't get an ENOMEM, we can't safely sleep
in the RCU section, so we'd either have to do all memory allocations
atomically (which would be bad and unreliable) or else we avoid the
need to allocate in the RCU section (which is the choice taken here).
By passing "true" as the "threadgroup" parameter to can_attach(),
we're letting the subsystem decide if it needs to do per-thread
checks. For most subsystems, calling them once for each thread would
be unnecessary.
It's possible that some threads from the process are exiting while
we're trying to move the entire process. As long as we move at least
one thread, we shouldn't care if some of its threads are exiting.
Which means that after we've done the prefetch loop, we should
probably check that the newcg_list isn't empty, and return -ESRCH in
that case.
/* acquire a new css_set for the leader */
Maybe better to say "in the worst case this is O(n^2) on the number of
threads; however, in the vast majority of cases all the threads will
be in the same cgroups as the leader and we'll make a just single pass
through the list with no additional allocations needed".
Yes, because we don't sleep so the RCU list is still valid.
/* See if we already have an appropriate css_set for this thread */
/* Since we may have slept in css_set_prefetch(), the RCU list is no
longer valid, so we must begin the iteration again; Any threads that
we've previously processed will pass the css_set_check_fetched() test
on subsequent iterations since we hold cgroup_lock, so we're
guaranteed to make progress. */
In general there'll only be one cgroup so it'll be a single pass
through the list.
The problem with this is that remember_need_to_allocate() will itself
need to allocate memory in order to allow need_to_allocate_array to
expand arbitrarily. Which can't be done without GFP_ATOMIC or else
sleeping in the RCU section, neither of which are good.
I wonder if we might need a synchronize_rcu() here?
Why are you adding this requirement, here and in sched.c? (ns_cgroup.c
doesn't matter since it's being deleted).
Paul
--
