Re: [Devel] [PATCH] cgroups: not to iterate other namespace process inside container

Re: [Devel] [PATCH] cgroups: not to iterate other namespace process inside container

[Paul Menage]

On Sun, Dec 7, 2008 at 10:52 PM, gowrishankar
<gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> 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 <gowrishankar.m-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>

Looks sensible, thanks.

Acked-by: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Can you send it on to akpm-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org?

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"

> 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;
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
>
> _______________________________________________
> Devel mailing list
> Devel-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org
> https://openvz.org/mailman/listinfo/devel
>

[PATCH] cgroups: not to iterate other namespace process inside container

[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 <gowrishankar.m-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>
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;

Re: [Devel] [PATCH] cgroups: not to iterate other namespace process inside container

[Balbir Singh]

* Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> [2008-12-07 22:31:03]:

> On Sun, Dec 7, 2008 at 10:52 PM, gowrishankar
> <gomuthuk-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> > 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 <gowrishankar.m-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>
> 
> Looks sensible, thanks.
> 
> Acked-by: Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> Can you send it on to akpm-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org?
> 
> 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?).

> > 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;

-- 
	Balbir

Re: [PATCH] cgroups: not to iterate other namespace process inside container

[Dave Hansen]

On Mon, 2008-12-08 at 12:22 +0530, gowrishankar wrote:
>  static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp)
>  {
> -    int n = 0;
> +    int n = 0, ret;

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

>      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;

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.

>      }
>      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

Re: [PATCH] cgroups: not to iterate other namespace process inside container

[Sukadev Bhattiprolu]

Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] 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).