[PATCH V2] cgroup: Introduce cgroup_detach_task().

[PATCH V2] cgroup: Introduce cgroup_detach_task().

[Dongsheng Yang]

Currently, the only method to detach a task from a cgroup is moving
it to others. It looks not natrual to me.

Inspired by cgroup_subtree_control_write(), this patch introduce allow
user to at-detach a process to/from a cgroup by echo "+/-pid" to
cgroup.procs. In addition, we keep the old method to allow user
echo "pid" without "+/-" to cgroup.procs as a attaching behavior.

Signed-off-by: Dongsheng Yang <yangds.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---

From v1:
	*Sorry for a incorrect comment in v1.

 kernel/cgroup.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7dc8788..a4bb604 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2348,6 +2348,43 @@ static int cgroup_attach_task(struct cgroup *dst_cgrp,
 	return ret;
 }
 
+/**
+ * cgroup_detach_task - detach a task or a whole threadgroup to a cgroup
+ * @src_cgrp: the cgroup to detach from
+ * @leader: the task or the leader of the threadgroup to be attached
+ * @threadgroup: attach the whole threadgroup?
+ *
+ * Call holding cgroup_mutex and threadgroup_lock of @leader.
+ */
+static int cgroup_detach_task(struct cgroup *src_cgrp __maybe_unused,
+			      struct task_struct *leader, bool threadgroup)
+{
+	LIST_HEAD(preloaded_csets);
+	struct task_struct *task;
+	int ret;
+
+	/* look up all src csets */
+	down_read(&css_set_rwsem);
+	rcu_read_lock();
+	task = leader;
+	do {
+		cgroup_migrate_add_src(task_css_set(task), &cgrp_dfl_root.cgrp,
+				       &preloaded_csets);
+		if (!threadgroup)
+			break;
+	} while_each_thread(leader, task);
+	rcu_read_unlock();
+	up_read(&css_set_rwsem);
+
+	/* prepare dst csets and commit */
+	ret = cgroup_migrate_prepare_dst(&cgrp_dfl_root.cgrp, &preloaded_csets);
+	if (!ret)
+		ret = cgroup_migrate(&cgrp_dfl_root.cgrp, leader, threadgroup);
+
+	cgroup_migrate_finish(&preloaded_csets);
+	return ret;
+}
+
 /*
  * Find the task_struct of the task to attach by vpid and pass it along to the
  * function to attach either it or all tasks in its threadgroup. Will lock
@@ -2361,8 +2398,25 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf,
 	struct cgroup *cgrp;
 	pid_t pid;
 	int ret;
+	bool attach;
 
-	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
+	/*
+	 * Parse input - pid prefixed with either + or -.
+	 */
+	buf = strstrip(buf);
+	if (*buf == '+') {
+		attach = true;
+		buf++;
+	} else if (*buf == '-') {
+		attach = false;
+		buf++;
+	} else {
+		if (!isdigit(*buf))
+			return -EINVAL;
+		attach = true;
+	}
+
+	if (kstrtoint(buf, 0, &pid) || pid < 0)
 		return -EINVAL;
 
 	cgrp = cgroup_kn_lock_live(of->kn);
@@ -2426,7 +2480,10 @@ retry_find_task:
 		}
 	}
 
-	ret = cgroup_attach_task(cgrp, tsk, threadgroup);
+	if (attach)
+		ret = cgroup_attach_task(cgrp, tsk, threadgroup);
+	else
+		ret = cgroup_detach_task(cgrp, tsk, threadgroup);
 
 	threadgroup_unlock(tsk);
 
-- 
1.8.4.2

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Tejun Heo]

On Mon, Aug 25, 2014 at 07:32:10PM +0800, Dongsheng Yang wrote:
> Currently, the only method to detach a task from a cgroup is moving
> it to others. It looks not natrual to me.

Ummm... how is this different from moving it to the root cgroup?
"just because" usually isn't a good enough rationale.

Thanks.

-- 
tejun

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Dongsheng Yang]

Hi tj.

On Mon, Aug 25, 2014 at 10:12 PM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Aug 25, 2014 at 07:32:10PM +0800, Dongsheng Yang wrote:
>> Currently, the only method to detach a task from a cgroup is moving
>> it to others. It looks not natrual to me.
>
> Ummm... how is this different from moving it to the root cgroup?
> "just because" usually isn't a good enough rationale.
>

Thanx for your reply :).

My point here is that attaching and detaching are a pair of operations.
And the things involved in these operations should be a process
and a cgroup. But currently, when we want to detach a process from
a cgroup (A), we have to attach it to another cgroup (B). I think it is not
easy to understand. People would say: "Why I want to detach a process
from A, I have to care about another cgroup."

So I created this patch here wanting to make these operations more easy to
understand. And make attaching and detaching to be antonymous in action.

I think maybe we can use it in the Cgroup V2 which is in developing. :)

That's all my reason for this patch. But you could think it makes no sense.
It's okey to me.

Thanx

> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Tejun Heo]

On Mon, Aug 25, 2014 at 10:46:03PM +0800, Dongsheng Yang wrote:
> My point here is that attaching and detaching are a pair of operations.

There is no detaching from a cgroup.  A task is always attached to a
cgroup whether that's a root or non-root cgroup.

Thanks.

-- 
tejun

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Dongsheng Yang]

On Mon, Aug 25, 2014 at 10:47 PM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Aug 25, 2014 at 10:46:03PM +0800, Dongsheng Yang wrote:
>> My point here is that attaching and detaching are a pair of operations.
>
> There is no detaching from a cgroup.  A task is always attached to a
> cgroup whether that's a root or non-root cgroup.

Okey, I should not think it as attaching and detaching. Just treat them as
a move between root and non-root cgroup.

It sounds reasonable to me now.

Thanx.
>
> Thanks.
>
> --
> tejun

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Li Zefan]

On 2014/8/25 23:00, Dongsheng Yang wrote:
> On Mon, Aug 25, 2014 at 10:47 PM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Mon, Aug 25, 2014 at 10:46:03PM +0800, Dongsheng Yang wrote:
>>> My point here is that attaching and detaching are a pair of operations.
>>
>> There is no detaching from a cgroup.  A task is always attached to a
>> cgroup whether that's a root or non-root cgroup.
> 
> Okey, I should not think it as attaching and detaching. Just treat them as
> a move between root and non-root cgroup.
> 
> It sounds reasonable to me now.
> 

I from time to time have to explain this to other people.

Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Dongsheng Yang]

On Tue, Aug 26, 2014 at 9:35 AM, Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> On 2014/8/25 23:00, Dongsheng Yang wrote:
>> On Mon, Aug 25, 2014 at 10:47 PM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> On Mon, Aug 25, 2014 at 10:46:03PM +0800, Dongsheng Yang wrote:
>>>> My point here is that attaching and detaching are a pair of operations.
>>>
>>> There is no detaching from a cgroup.  A task is always attached to a
>>> cgroup whether that's a root or non-root cgroup.
>>
>> Okey, I should not think it as attaching and detaching. Just treat them as
>> a move between root and non-root cgroup.
>>
>> It sounds reasonable to me now.
>>
>
> I from time to time have to explain this to other people.

Ha, we usually want to find a detach function when we saw a function
named as cgroup_attach_task().
>

RE: [PATCH V2] cgroup: Introduce cgroup_detach_task().

[Ni, Xun]

Maybe your point can be added to the README or FAQ for cgroup. Try to shoot a patch for that, it will be more effective...

Thanks,
Xun

-----Original Message-----
From: cgroups-owner@vger.kernel.org [mailto:cgroups-owner@vger.kernel.org] On Behalf Of Dongsheng Yang
Sent: Tuesday, August 26, 2014 10:16 AM
To: Li Zefan
Cc: Tejun Heo; Dongsheng Yang; cgroups@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] cgroup: Introduce cgroup_detach_task().

On Tue, Aug 26, 2014 at 9:35 AM, Li Zefan <lizefan@huawei.com> wrote:
> On 2014/8/25 23:00, Dongsheng Yang wrote:
>> On Mon, Aug 25, 2014 at 10:47 PM, Tejun Heo <tj@kernel.org> wrote:
>>> On Mon, Aug 25, 2014 at 10:46:03PM +0800, Dongsheng Yang wrote:
>>>> My point here is that attaching and detaching are a pair of operations.
>>>
>>> There is no detaching from a cgroup.  A task is always attached to a 
>>> cgroup whether that's a root or non-root cgroup.
>>
>> Okey, I should not think it as attaching and detaching. Just treat 
>> them as a move between root and non-root cgroup.
>>
>> It sounds reasonable to me now.
>>
>
> I from time to time have to explain this to other people.

Ha, we usually want to find a detach function when we saw a function named as cgroup_attach_task().
>
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html