cgroups: not to iterate other namespace process inside container

PATCH In review gowrishankar 5 participants 5 messages Dec 8, 2008 – Dec 9, 2008 2 messages not in the archive

Share & tools

gowrishankar

author

2008-12-08 06:52 UTC #0 lore raw

PATCH 1/1 cgroups: not to iterate other namespace process inside container

Once tasks are populated from system namespace inside cgroup, container replaces
other namespace task with 0 while listing tasks, inside container. Though this is expected behaviour
from container end, there is no use of showing unwanted 0s. In below patch, we check if a process is in same namespace before loading into pid array.

Signed-off-by Gowrishankar M <[email protected]>

Index: linux-2.6.28-rc3/kernel/cgroup.c
===================================================================
--- linux-2.6.28-rc3.orig/kernel/cgroup.c 2008-12-07
13:23:19.000000000 -0500
+++ linux-2.6.28-rc3/kernel/cgroup.c 2008-12-07 13:24:28.000000000 -0500
@@ -2011,14 +2011,15 @@
*/
static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp) {
- int n = 0;

+    int n = 0, ret;
     struct cgroup_iter it;
     struct task_struct *tsk;
     cgroup_iter_start(cgrp, &it);
     while ((tsk = cgroup_iter_next(cgrp, &it))) {
         if (unlikely(n == npids))
             break;
-        pidarray[n++] = task_pid_vnr(tsk);
+        if ((ret = task_pid_vnr(tsk)) > 0)
+            pidarray[n++] = ret;
     }
     cgroup_iter_end(cgrp, &it);
     return n;

A message that is not in the archive (sent off-list or not yet received): 493CC423.5060601-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org

Paul Menage

Acked-by

2008-12-08 06:31 UTC #2 lore raw

Quoted 9 lines · gowrishankar

Once tasks are populated from system namespace inside cgroup, container replaces
other namespace task with 0 while listing tasks, inside container. Though this is expected behaviour
from container end, there is no use of showing unwanted 0s. In below patch, we check if a process is in same namespace before loading into pid array.
Signed-off-by Gowrishankar M <[email protected]>

Looks sensible, thanks.

Acked-by Paul Menage <[email protected]>

Can you send it on to [email protected]?

A couple of small style issues:

- title would be better as "skip processes from other namespaces when listing a cgroup"

- use the local variable name "pid" rather than "ret"

Quoted 33 lines

Index: linux-2.6.28-rc3/kernel/cgroup.c
===================================================================
--- linux-2.6.28-rc3.orig/kernel/cgroup.c 2008-12-07
13:23:19.000000000 -0500
+++ linux-2.6.28-rc3/kernel/cgroup.c 2008-12-07 13:24:28.000000000 -0500
(quoted diff) +3 -2
@@ -2011,14 +2011,15 @@
 */
 static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp)
 {
-    int n = 0;
+    int n = 0, ret;
    struct cgroup_iter it;
    struct task_struct *tsk;
    cgroup_iter_start(cgrp, &it);
    while ((tsk = cgroup_iter_next(cgrp, &it))) {
        if (unlikely(n == npids))
            break;
-        pidarray[n++] = task_pid_vnr(tsk);
+        if ((ret = task_pid_vnr(tsk)) > 0)
+            pidarray[n++] = ret;
    }
    cgroup_iter_end(cgrp, &it);
    return n;
_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

A message that is not in the archive (sent off-list or not yet received): 6599ad830812072231l25aadbd8u6653cda3a22ddc2d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Balbir Singh

2008-12-08 06:56 UTC #4 lore raw

Quoted 16 lines · Paul Menage

Quoted 9 lines · gowrishankar
Once tasks are populated from system namespace inside cgroup, container replaces
other namespace task with 0 while listing tasks, inside container. Though this is expected behaviour
from container end, there is no use of showing unwanted 0s. In below patch, we check if a process is in same namespace before loading into pid array.
Signed-off-by Gowrishankar M <[email protected]>
Looks sensible, thanks.
Acked-by Paul Menage <[email protected]>

Quoted 9 lines

Can you send it on to [email protected]?
A couple of small style issues:
- title would be better as "skip processes from other namespaces when listing a cgroup"
- use the local variable name "pid" rather than "ret"

I would recommend using pid_t as type, since task_pid_vnr returns pid_t and vpid as the variable name. Although this fixes pid_array_load(), I am worried that cg_list would still have tasks from other namespaces that can affect controllers. We'll need other changes to get the controllers working correctly (a review of the code maybe?).

Quoted 23 lines

(quoted diff) +3 -2

@@ -2011,14 +2011,15 @@
 */
 static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp)
 {
-    int n = 0;
+    int n = 0, ret;
    struct cgroup_iter it;
    struct task_struct *tsk;
    cgroup_iter_start(cgrp, &it);
    while ((tsk = cgroup_iter_next(cgrp, &it))) {
        if (unlikely(n == npids))
            break;
-        pidarray[n++] = task_pid_vnr(tsk);
+        if ((ret = task_pid_vnr(tsk)) > 0)
+            pidarray[n++] = ret;
    }
    cgroup_iter_end(cgrp, &it);
    return n;

Signature · 1 line

	Balbir

Dave Hansen

2008-12-08 16:18 UTC #5 lore raw

In reply to a message not in the archive

Review

Quoted 4 lines · gowrishankar

static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp) {
(quoted diff) +1 -1
-    int n = 0;
+    int n = 0, ret;

Please declare these separately unless there's a really good reason not to.

Review

Quoted 9 lines

     struct cgroup_iter it;
     struct task_struct *tsk;
     cgroup_iter_start(cgrp, &it);
     while ((tsk = cgroup_iter_next(cgrp, &it))) {
         if (unlikely(n == npids))
             break;

(quoted diff) +2 -1

-        pidarray[n++] = task_pid_vnr(tsk);
+        if ((ret = task_pid_vnr(tsk)) > 0)
+            pidarray[n++] = ret;

We almost never write things this way in the kernel. Too many people mistake this if() form for a coding mistake. Please separate out the assignment and condition:

	ret = task_pid_vnr(tsk);
	if (ret > 0)
		...

Also, 'ret' is generally used to indicate that the variable is going to be returned from the function. This one is not.

Quoted 3 lines

     }
     cgroup_iter_end(cgrp, &it);
     return n;

I'm not sure this is a good patch, but it needs some CodingStyle love either way.

I could have sworn we had some function like task_is_in_current_active_pid_ns(), but I looked and couldn't find it. Maybe we should add one instead of open-coding this everywhere.

-- Dave

Sukadev Bhattiprolu

2008-12-09 23:37 UTC #6 lore raw

Dave Hansen [[email protected]] wrote:
| I could have sworn we had some function like
| task_is_in_current_active_pid_ns(), but I looked and couldn't find it.

It was in one of the many signals patchsets we tried but none of them got accepted. We do have a variant out for review, ns_of_pid() that could be used, this patch task_pid_vnr() should be fine.

| Maybe we should add one instead of open-coding this everywhere.

If there are lot of users, we should define the wrapper for 'task_pid_vnr() > 0' but there is no other user at present (kill_something_info() checks for > 1).