Re: [v10 2/6] mm: implement mem_cgroup_scan_tasks() for the root memory cgroup

David Rientjes
2017-10-04T20:10:48Z

On Wed, 4 Oct 2017, Roman Gushchin wrote:

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d5f3a62887cf..b4de17a78dc1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -917,7 +917,8 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
> * value, the function breaks the iteration loop and returns the value.
> * Otherwise, it will iterate over all tasks and return 0.
> *
> - * This function must not be called for the root memory cgroup.
> + * If memcg is the root memory cgroup, this function will iterate only
> + * over tasks belonging directly to the root memory cgroup.
> */
> int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
> int (*fn)(struct task_struct *, void *), void *arg)
> @@ -925,8 +926,6 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
> struct mem_cgroup *iter;
> int ret = 0;
>
> - BUG_ON(memcg == root_mem_cgroup);
> -
> for_each_mem_cgroup_tree(iter, memcg) {
> struct css_task_iter it;
> struct task_struct *task;
> @@ -935,7 +934,7 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
> while (!ret && (task = css_task_iter_next(&it)))
> ret = fn(task, arg);
> css_task_iter_end(&it);
> - if (ret) {
> + if (ret || memcg == root_mem_cgroup) {
> mem_cgroup_iter_break(memcg, iter);
> break;
> }

I think this is fine, but a little strange to start an iteration that
never loops :) No objection to the patch but it could also be extracted
into a new mem_cgroup_scan_tasks() which actually scans the tasks in that
mem cgroup and then a wrapper that does the iteration that calls into it,
say, mem_cgroup_scan_tasks_tree().