[PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

Hello,

Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.

 - tasks: list of tasks (by PID) attached to that cgroup.  This list
   is not guaranteed to be sorted. 
 - cgroup.procs: list of thread group IDs in the cgroup.  This list is
   not guaranteed to be sorted or free of duplicate TGIDs, and userspace
   should sort/uniquify the list if this property is required.

This patch remove the sorting function to make the default behavior of pid list be consistent with
the document.

Without patch:
# echo 2942 >> /cgroup/test/tasks 
# echo 2803 >> /cgroup/test/tasks 
# echo 2911 >> /cgroup/test/tasks
# echo 2761 >> /cgroup/test/tasks 
# cat /cgroup/test/tasks 
2761
2803
2911
2942
# cat /cgroup/test/cgroup.procs
2761
2803
2911
2942

Patch applied:
# echo 3042 >> /cgroup/testfixed/tasks
# echo 2916 >> /cgroup/testfixed/tasks
# echo 2781 >> /cgroup/testfixed/tasks
# echo 2527 >> /cgroup/testfixed/tasks
# cat /cgroup/testfixed/tasks 
2916
3042
2527
2781
# cat /cgroup/testfixed/cgroup.procs 
2916
3042
2527
2781


Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

---
 kernel/cgroup.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f24f724..2b5b477 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -47,7 +47,6 @@
 #include <linux/magic.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
-#include <linux/sort.h>
 #include <linux/kmod.h>
 #include <linux/module.h>
 #include <linux/delayacct.h>
@@ -3374,11 +3373,6 @@ after:
 	return dest;
 }
 
-static int cmppid(const void *a, const void *b)
-{
-	return *(pid_t *)a - *(pid_t *)b;
-}
-
 /*
  * find the appropriate pidlist for our purpose (given procs vs tasks)
  * returns with the lock on that pidlist already held, and takes care
@@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	}
 	cgroup_iter_end(cgrp, &it);
 	length = n;
-	/* now sort & (if procs) strip out duplicates */
-	sort(array, length, sizeof(pid_t), cmppid, NULL);
+	/* if procs, strip out duplicates */
 	if (type == CGROUP_FILE_PROCS)
 		length = pidlist_uniq(&array, length);
 	l = cgroup_pidlist_find(cgrp, type);
-- 
1.7.9.5

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

Sorry, I forgot removing the codes regarding free of duplicate TGIDs at
cgroup.procs, will re-sent another patch later.

Thanks,
-Jeff

On 01/15/2013 01:28 PM, Jeff Liu wrote:
> Hello,
> 
> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> 
>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>    is not guaranteed to be sorted. 
>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>    should sort/uniquify the list if this property is required.
> 
> This patch remove the sorting function to make the default behavior of pid list be consistent with
> the document.
> 
> Without patch:
> # echo 2942 >> /cgroup/test/tasks 
> # echo 2803 >> /cgroup/test/tasks 
> # echo 2911 >> /cgroup/test/tasks
> # echo 2761 >> /cgroup/test/tasks 
> # cat /cgroup/test/tasks 
> 2761
> 2803
> 2911
> 2942
> # cat /cgroup/test/cgroup.procs
> 2761
> 2803
> 2911
> 2942
> 
> Patch applied:
> # echo 3042 >> /cgroup/testfixed/tasks
> # echo 2916 >> /cgroup/testfixed/tasks
> # echo 2781 >> /cgroup/testfixed/tasks
> # echo 2527 >> /cgroup/testfixed/tasks
> # cat /cgroup/testfixed/tasks 
> 2916
> 3042
> 2527
> 2781
> # cat /cgroup/testfixed/cgroup.procs 
> 2916
> 3042
> 2527
> 2781
> 
> 
> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  kernel/cgroup.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f24f724..2b5b477 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -47,7 +47,6 @@
>  #include <linux/magic.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> -#include <linux/sort.h>
>  #include <linux/kmod.h>
>  #include <linux/module.h>
>  #include <linux/delayacct.h>
> @@ -3374,11 +3373,6 @@ after:
>  	return dest;
>  }
>  
> -static int cmppid(const void *a, const void *b)
> -{
> -	return *(pid_t *)a - *(pid_t *)b;
> -}
> -
>  /*
>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>   * returns with the lock on that pidlist already held, and takes care
> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>  	}
>  	cgroup_iter_end(cgrp, &it);
>  	length = n;
> -	/* now sort & (if procs) strip out duplicates */
> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
> +	/* if procs, strip out duplicates */
>  	if (type == CGROUP_FILE_PROCS)
>  		length = pidlist_uniq(&array, length);
>  	l = cgroup_pidlist_find(cgrp, type);
> 

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> Hello,
> 
> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> 
>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>    is not guaranteed to be sorted. 
>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>    should sort/uniquify the list if this property is required.
> 
> This patch remove the sorting function to make the default behavior of pid list be consistent with
> the document.
> 
> Without patch:
> # echo 2942 >> /cgroup/test/tasks 
> # echo 2803 >> /cgroup/test/tasks 
> # echo 2911 >> /cgroup/test/tasks
> # echo 2761 >> /cgroup/test/tasks 
> # cat /cgroup/test/tasks 
> 2761
> 2803
> 2911
> 2942
> # cat /cgroup/test/cgroup.procs
> 2761
> 2803
> 2911
> 2942
> 
> Patch applied:
> # echo 3042 >> /cgroup/testfixed/tasks
> # echo 2916 >> /cgroup/testfixed/tasks
> # echo 2781 >> /cgroup/testfixed/tasks
> # echo 2527 >> /cgroup/testfixed/tasks
> # cat /cgroup/testfixed/tasks 
> 2916
> 3042
> 2527
> 2781
> # cat /cgroup/testfixed/cgroup.procs 
> 2916
> 3042
> 2527
> 2781
> 
> 
> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  kernel/cgroup.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f24f724..2b5b477 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -47,7 +47,6 @@
>  #include <linux/magic.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> -#include <linux/sort.h>
>  #include <linux/kmod.h>
>  #include <linux/module.h>
>  #include <linux/delayacct.h>
> @@ -3374,11 +3373,6 @@ after:
>  	return dest;
>  }
>  
> -static int cmppid(const void *a, const void *b)
> -{
> -	return *(pid_t *)a - *(pid_t *)b;
> -}
> -
>  /*
>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>   * returns with the lock on that pidlist already held, and takes care
> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>  	}
>  	cgroup_iter_end(cgrp, &it);
>  	length = n;
> -	/* now sort & (if procs) strip out duplicates */
> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
> +	/* if procs, strip out duplicates */
>  	if (type == CGROUP_FILE_PROCS)
>  		length = pidlist_uniq(&array, length);

But...  pidlist_uniq() depends on array being sorted

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> Hello,
> 
> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> 
>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>    is not guaranteed to be sorted. 
>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>    should sort/uniquify the list if this property is required.
> 
> This patch remove the sorting function to make the default behavior of pid list be consistent with
> the document.
> 
> Without patch:
> # echo 2942 >> /cgroup/test/tasks 
> # echo 2803 >> /cgroup/test/tasks 
> # echo 2911 >> /cgroup/test/tasks
> # echo 2761 >> /cgroup/test/tasks 
> # cat /cgroup/test/tasks 
> 2761
> 2803
> 2911
> 2942
> # cat /cgroup/test/cgroup.procs
> 2761
> 2803
> 2911
> 2942
> 
> Patch applied:
> # echo 3042 >> /cgroup/testfixed/tasks
> # echo 2916 >> /cgroup/testfixed/tasks
> # echo 2781 >> /cgroup/testfixed/tasks
> # echo 2527 >> /cgroup/testfixed/tasks
> # cat /cgroup/testfixed/tasks 
> 2916
> 3042
> 2527
> 2781
> # cat /cgroup/testfixed/cgroup.procs 
> 2916
> 3042
> 2527
> 2781
> 
> 
> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  kernel/cgroup.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f24f724..2b5b477 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -47,7 +47,6 @@
>  #include <linux/magic.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> -#include <linux/sort.h>
>  #include <linux/kmod.h>
>  #include <linux/module.h>
>  #include <linux/delayacct.h>
> @@ -3374,11 +3373,6 @@ after:
>  	return dest;
>  }
>  
> -static int cmppid(const void *a, const void *b)
> -{
> -	return *(pid_t *)a - *(pid_t *)b;
> -}
> -
>  /*
>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>   * returns with the lock on that pidlist already held, and takes care
> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>  	}
>  	cgroup_iter_end(cgrp, &it);
>  	length = n;
> -	/* now sort & (if procs) strip out duplicates */
> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
> +	/* if procs, strip out duplicates */
>  	if (type == CGROUP_FILE_PROCS)
>  		length = pidlist_uniq(&array, length);

But...  pidlist_uniq() depends on array being sorted

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> Hello,
> 
> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> 
>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>    is not guaranteed to be sorted. 
>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>    should sort/uniquify the list if this property is required.
> 
> This patch remove the sorting function to make the default behavior of pid list be consistent with
> the document.
> 
> Without patch:
> # echo 2942 >> /cgroup/test/tasks 
> # echo 2803 >> /cgroup/test/tasks 
> # echo 2911 >> /cgroup/test/tasks
> # echo 2761 >> /cgroup/test/tasks 
> # cat /cgroup/test/tasks 
> 2761
> 2803
> 2911
> 2942
> # cat /cgroup/test/cgroup.procs
> 2761
> 2803
> 2911
> 2942
> 
> Patch applied:
> # echo 3042 >> /cgroup/testfixed/tasks
> # echo 2916 >> /cgroup/testfixed/tasks
> # echo 2781 >> /cgroup/testfixed/tasks
> # echo 2527 >> /cgroup/testfixed/tasks
> # cat /cgroup/testfixed/tasks 
> 2916
> 3042
> 2527
> 2781
> # cat /cgroup/testfixed/cgroup.procs 
> 2916
> 3042
> 2527
> 2781
> 
> 
> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  kernel/cgroup.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f24f724..2b5b477 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -47,7 +47,6 @@
>  #include <linux/magic.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> -#include <linux/sort.h>
>  #include <linux/kmod.h>
>  #include <linux/module.h>
>  #include <linux/delayacct.h>
> @@ -3374,11 +3373,6 @@ after:
>  	return dest;
>  }
>  
> -static int cmppid(const void *a, const void *b)
> -{
> -	return *(pid_t *)a - *(pid_t *)b;
> -}
> -
>  /*
>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>   * returns with the lock on that pidlist already held, and takes care
> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>  	}
>  	cgroup_iter_end(cgrp, &it);
>  	length = n;
> -	/* now sort & (if procs) strip out duplicates */
> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
> +	/* if procs, strip out duplicates */
>  	if (type == CGROUP_FILE_PROCS)
>  		length = pidlist_uniq(&array, length);

But...  pidlist_uniq() depends on array being sorted

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>> Hello,
>>
>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
>>
>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>>    is not guaranteed to be sorted. 
>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>>    should sort/uniquify the list if this property is required.
>>
>> This patch remove the sorting function to make the default behavior of pid list be consistent with
>> the document.
>>
>> Without patch:
>> # echo 2942 >> /cgroup/test/tasks 
>> # echo 2803 >> /cgroup/test/tasks 
>> # echo 2911 >> /cgroup/test/tasks
>> # echo 2761 >> /cgroup/test/tasks 
>> # cat /cgroup/test/tasks 
>> 2761
>> 2803
>> 2911
>> 2942
>> # cat /cgroup/test/cgroup.procs
>> 2761
>> 2803
>> 2911
>> 2942
>>
>> Patch applied:
>> # echo 3042 >> /cgroup/testfixed/tasks
>> # echo 2916 >> /cgroup/testfixed/tasks
>> # echo 2781 >> /cgroup/testfixed/tasks
>> # echo 2527 >> /cgroup/testfixed/tasks
>> # cat /cgroup/testfixed/tasks 
>> 2916
>> 3042
>> 2527
>> 2781
>> # cat /cgroup/testfixed/cgroup.procs 
>> 2916
>> 3042
>> 2527
>> 2781
>>
>>
>> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>>  kernel/cgroup.c |    9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index f24f724..2b5b477 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -47,7 +47,6 @@
>>  #include <linux/magic.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/string.h>
>> -#include <linux/sort.h>
>>  #include <linux/kmod.h>
>>  #include <linux/module.h>
>>  #include <linux/delayacct.h>
>> @@ -3374,11 +3373,6 @@ after:
>>  	return dest;
>>  }
>>  
>> -static int cmppid(const void *a, const void *b)
>> -{
>> -	return *(pid_t *)a - *(pid_t *)b;
>> -}
>> -
>>  /*
>>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>>   * returns with the lock on that pidlist already held, and takes care
>> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>>  	}
>>  	cgroup_iter_end(cgrp, &it);
>>  	length = n;
>> -	/* now sort & (if procs) strip out duplicates */
>> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
>> +	/* if procs, strip out duplicates */
>>  	if (type == CGROUP_FILE_PROCS)
>>  		length = pidlist_uniq(&array, length);
> 
> But...  pidlist_uniq() depends on array being sorted
Yes, I forgot removing this function according to cgroup.procs documents.

cgroup.procs: list of thread group IDs in the cgroup.  This list is
not guaranteed to be sorted or free of duplicate TGIDs

We should remove pidlist_uniq() as well.

Thanks,
-Jeff

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>> Hello,
>>
>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
>>
>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>>    is not guaranteed to be sorted. 
>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>>    should sort/uniquify the list if this property is required.
>>
>> This patch remove the sorting function to make the default behavior of pid list be consistent with
>> the document.
>>
>> Without patch:
>> # echo 2942 >> /cgroup/test/tasks 
>> # echo 2803 >> /cgroup/test/tasks 
>> # echo 2911 >> /cgroup/test/tasks
>> # echo 2761 >> /cgroup/test/tasks 
>> # cat /cgroup/test/tasks 
>> 2761
>> 2803
>> 2911
>> 2942
>> # cat /cgroup/test/cgroup.procs
>> 2761
>> 2803
>> 2911
>> 2942
>>
>> Patch applied:
>> # echo 3042 >> /cgroup/testfixed/tasks
>> # echo 2916 >> /cgroup/testfixed/tasks
>> # echo 2781 >> /cgroup/testfixed/tasks
>> # echo 2527 >> /cgroup/testfixed/tasks
>> # cat /cgroup/testfixed/tasks 
>> 2916
>> 3042
>> 2527
>> 2781
>> # cat /cgroup/testfixed/cgroup.procs 
>> 2916
>> 3042
>> 2527
>> 2781
>>
>>
>> Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>>  kernel/cgroup.c |    9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index f24f724..2b5b477 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -47,7 +47,6 @@
>>  #include <linux/magic.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/string.h>
>> -#include <linux/sort.h>
>>  #include <linux/kmod.h>
>>  #include <linux/module.h>
>>  #include <linux/delayacct.h>
>> @@ -3374,11 +3373,6 @@ after:
>>  	return dest;
>>  }
>>  
>> -static int cmppid(const void *a, const void *b)
>> -{
>> -	return *(pid_t *)a - *(pid_t *)b;
>> -}
>> -
>>  /*
>>   * find the appropriate pidlist for our purpose (given procs vs tasks)
>>   * returns with the lock on that pidlist already held, and takes care
>> @@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
>>  	}
>>  	cgroup_iter_end(cgrp, &it);
>>  	length = n;
>> -	/* now sort & (if procs) strip out duplicates */
>> -	sort(array, length, sizeof(pid_t), cmppid, NULL);
>> +	/* if procs, strip out duplicates */
>>  	if (type == CGROUP_FILE_PROCS)
>>  		length = pidlist_uniq(&array, length);
> 
> But...  pidlist_uniq() depends on array being sorted
Yes, I forgot removing this function according to cgroup.procs documents.

cgroup.procs: list of thread group IDs in the cgroup.  This list is
not guaranteed to be sorted or free of duplicate TGIDs

We should remove pidlist_uniq() as well.

Thanks,
-Jeff

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> > Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >> Hello,
> >>
> >> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> >> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> >>
> >>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
> >>    is not guaranteed to be sorted. 
> >>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
> >>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
> >>    should sort/uniquify the list if this property is required.
> >>
> >> This patch remove the sorting function to make the default behavior of pid list be consistent with
> >> the document.

...

I've gotta say, as someone who tends to play with those files by hand, I
don't mind not having to type | sort | uniq every time.

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> > Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >> Hello,
> >>
> >> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> >> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> >>
> >>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
> >>    is not guaranteed to be sorted. 
> >>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
> >>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
> >>    should sort/uniquify the list if this property is required.
> >>
> >> This patch remove the sorting function to make the default behavior of pid list be consistent with
> >> the document.

...

I've gotta say, as someone who tends to play with those files by hand, I
don't mind not having to type | sort | uniq every time.

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

On 01/15/2013 02:34 PM, Serge Hallyn wrote:
> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
>>> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>>>> Hello,
>>>>
>>>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
>>>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
>>>>
>>>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>>>>    is not guaranteed to be sorted. 
>>>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>>>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>>>>    should sort/uniquify the list if this property is required.
>>>>
>>>> This patch remove the sorting function to make the default behavior of pid list be consistent with
>>>> the document.
> 
> ...
> 
> I've gotta say, as someone who tends to play with those files by hand, I
> don't mind not having to type | sort | uniq every time.

I did this for two reasons, one is for the documents, another is per
Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542

. Misc issues

  * Sort & unique when listing tasks.  Even the documentation says it
    doesn't happen but we have a good hunk of code doing it in
    cgroup.c.  I'm gonna rip it out at some point.  Again, if you
    don't like it, scream.

So that we can clean out around 70-lines source code as well as shrink
the cgroup.o a bit.

Thanks,
-Jeff

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

On 01/15/2013 02:34 PM, Serge Hallyn wrote:
> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
>>> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
>>>> Hello,
>>>>
>>>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
>>>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
>>>>
>>>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
>>>>    is not guaranteed to be sorted. 
>>>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
>>>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
>>>>    should sort/uniquify the list if this property is required.
>>>>
>>>> This patch remove the sorting function to make the default behavior of pid list be consistent with
>>>> the document.
> 
> ...
> 
> I've gotta say, as someone who tends to play with those files by hand, I
> don't mind not having to type | sort | uniq every time.

I did this for two reasons, one is for the documents, another is per
Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542

. Misc issues

  * Sort & unique when listing tasks.  Even the documentation says it
    doesn't happen but we have a good hunk of code doing it in
    cgroup.c.  I'm gonna rip it out at some point.  Again, if you
    don't like it, scream.

So that we can clean out around 70-lines source code as well as shrink
the cgroup.o a bit.

Thanks,
-Jeff

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> On 01/15/2013 02:34 PM, Serge Hallyn wrote:
> > Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> >>> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >>>> Hello,
> >>>>
> >>>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> >>>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> >>>>
> >>>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
> >>>>    is not guaranteed to be sorted. 
> >>>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
> >>>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
> >>>>    should sort/uniquify the list if this property is required.
> >>>>
> >>>> This patch remove the sorting function to make the default behavior of pid list be consistent with
> >>>> the document.
> > 
> > ...
> > 
> > I've gotta say, as someone who tends to play with those files by hand, I
> > don't mind not having to type | sort | uniq every time.
> 
> I did this for two reasons, one is for the documents, another is per
> Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> 
> . Misc issues
> 
>   * Sort & unique when listing tasks.  Even the documentation says it
>     doesn't happen but we have a good hunk of code doing it in
>     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
>     don't like it, scream.

Yeah, I figured, and since he also wants to move to a place where
cgroups are always manipulated using a library, not by hand, it
would make sense to get rid of the overhead there.  I just don't
really like either one of those :)  And in the list of things to
clean up in cgroups, this seems like one that can wait.

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge Hallyn]

Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> On 01/15/2013 02:34 PM, Serge Hallyn wrote:
> > Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >> On 01/15/2013 02:25 PM, Serge Hallyn wrote:
> >>> Quoting Jeff Liu (jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org):
> >>>> Hello,
> >>>>
> >>>> Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
> >>>> guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.
> >>>>
> >>>>  - tasks: list of tasks (by PID) attached to that cgroup.  This list
> >>>>    is not guaranteed to be sorted. 
> >>>>  - cgroup.procs: list of thread group IDs in the cgroup.  This list is
> >>>>    not guaranteed to be sorted or free of duplicate TGIDs, and userspace
> >>>>    should sort/uniquify the list if this property is required.
> >>>>
> >>>> This patch remove the sorting function to make the default behavior of pid list be consistent with
> >>>> the document.
> > 
> > ...
> > 
> > I've gotta say, as someone who tends to play with those files by hand, I
> > don't mind not having to type | sort | uniq every time.
> 
> I did this for two reasons, one is for the documents, another is per
> Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> 
> . Misc issues
> 
>   * Sort & unique when listing tasks.  Even the documentation says it
>     doesn't happen but we have a good hunk of code doing it in
>     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
>     don't like it, scream.

Yeah, I figured, and since he also wants to move to a place where
cgroups are always manipulated using a library, not by hand, it
would make sense to get rid of the overhead there.  I just don't
really like either one of those :)  And in the list of things to
clean up in cgroups, this seems like one that can wait.

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Tejun Heo]

Hello, guys.

On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
> > I did this for two reasons, one is for the documents, another is per
> > Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> > 
> > . Misc issues
> > 
> >   * Sort & unique when listing tasks.  Even the documentation says it
> >     doesn't happen but we have a good hunk of code doing it in
> >     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
> >     don't like it, scream.
> 
> Yeah, I figured, and since he also wants to move to a place where
> cgroups are always manipulated using a library, not by hand, it
> would make sense to get rid of the overhead there.  I just don't
> really like either one of those :)  And in the list of things to
> clean up in cgroups, this seems like one that can wait.

I want it gone but last time I pinged there were some people depending
on it, so I don't think I can.  I think we'll have to put it on ice
for now at least.  :(

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Tejun Heo]

Hello, guys.

On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
> > I did this for two reasons, one is for the documents, another is per
> > Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> > 
> > . Misc issues
> > 
> >   * Sort & unique when listing tasks.  Even the documentation says it
> >     doesn't happen but we have a good hunk of code doing it in
> >     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
> >     don't like it, scream.
> 
> Yeah, I figured, and since he also wants to move to a place where
> cgroups are always manipulated using a library, not by hand, it
> would make sense to get rid of the overhead there.  I just don't
> really like either one of those :)  And in the list of things to
> clean up in cgroups, this seems like one that can wait.

I want it gone but last time I pinged there were some people depending
on it, so I don't think I can.  I think we'll have to put it on ice
for now at least.  :(

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge E. Hallyn]

Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> Hello, guys.
> 
> On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
> > > I did this for two reasons, one is for the documents, another is per
> > > Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> > > 
> > > . Misc issues
> > > 
> > >   * Sort & unique when listing tasks.  Even the documentation says it
> > >     doesn't happen but we have a good hunk of code doing it in
> > >     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
> > >     don't like it, scream.
> > 
> > Yeah, I figured, and since he also wants to move to a place where
> > cgroups are always manipulated using a library, not by hand, it
> > would make sense to get rid of the overhead there.  I just don't
> > really like either one of those :)  And in the list of things to
> > clean up in cgroups, this seems like one that can wait.
> 
> I want it gone but last time I pinged there were some people depending
> on it, so I don't think I can.  I think we'll have to put it on ice
> for now at least.  :(

Phew :)

Seriously, once we switch over to using a library anyway, it makes sense.
Now I do see that removing the sorting would help move people over to
using a library...  is there a separate place where the library is being
discussed/designed?  Requirements being gathered?

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Serge E. Hallyn]

Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> Hello, guys.
> 
> On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
> > > I did this for two reasons, one is for the documents, another is per
> > > Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
> > > 
> > > . Misc issues
> > > 
> > >   * Sort & unique when listing tasks.  Even the documentation says it
> > >     doesn't happen but we have a good hunk of code doing it in
> > >     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
> > >     don't like it, scream.
> > 
> > Yeah, I figured, and since he also wants to move to a place where
> > cgroups are always manipulated using a library, not by hand, it
> > would make sense to get rid of the overhead there.  I just don't
> > really like either one of those :)  And in the list of things to
> > clean up in cgroups, this seems like one that can wait.
> 
> I want it gone but last time I pinged there were some people depending
> on it, so I don't think I can.  I think we'll have to put it on ice
> for now at least.  :(

Phew :)

Seriously, once we switch over to using a library anyway, it makes sense.
Now I do see that removing the sorting would help move people over to
using a library...  is there a separate place where the library is being
discussed/designed?  Requirements being gathered?

-serge

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Tejun Heo]

Hey, Serge.

On Tue, Jan 15, 2013 at 04:14:20PM +0000, Serge E. Hallyn wrote:
> Seriously, once we switch over to using a library anyway, it makes sense.
> Now I do see that removing the sorting would help move people over to
> using a library...  is there a separate place where the library is being
> discussed/designed?  Requirements being gathered?

It's still just a pipe dream of mine.  I have some requirements on
mind but don't yet have any concrete plan on how to actually do it.
For me, it's queued after unified hierarchy support in kernel, which I
think we're progressing towards better than I originally expected.  If
anyone is intereted in jumping in, please be my guest.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Tejun Heo]

Hey, Serge.

On Tue, Jan 15, 2013 at 04:14:20PM +0000, Serge E. Hallyn wrote:
> Seriously, once we switch over to using a library anyway, it makes sense.
> Now I do see that removing the sorting would help move people over to
> using a library...  is there a separate place where the library is being
> discussed/designed?  Requirements being gathered?

It's still just a pipe dream of mine.  I have some requirements on
mind but don't yet have any concrete plan on how to actually do it.
For me, it's queued after unified hierarchy support in kernel, which I
think we're progressing towards better than I originally expected.  If
anyone is intereted in jumping in, please be my guest.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

Hi Serge and Tejun,
On 01/15/2013 11:43 PM, Tejun Heo wrote:
> Hello, guys.
> 
> On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
>>> I did this for two reasons, one is for the documents, another is per
>>> Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
>>>
>>> . Misc issues
>>>
>>>   * Sort & unique when listing tasks.  Even the documentation says it
>>>     doesn't happen but we have a good hunk of code doing it in
>>>     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
>>>     don't like it, scream.
>>
>> Yeah, I figured, and since he also wants to move to a place where
>> cgroups are always manipulated using a library, not by hand, it
>> would make sense to get rid of the overhead there.  I just don't
>> really like either one of those :)  And in the list of things to
>> clean up in cgroups, this seems like one that can wait.
> 
> I want it gone but last time I pinged there were some people depending
> on it, so I don't think I can.  I think we'll have to put it on ice
> for now at least.  :(
Thanks both of you for the response :)  So I'll put it aside for now.

Regards,
-Jeff

Re: [PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

Hi Serge and Tejun,
On 01/15/2013 11:43 PM, Tejun Heo wrote:
> Hello, guys.
> 
> On Tue, Jan 15, 2013 at 08:08:11AM -0600, Serge Hallyn wrote:
>>> I did this for two reasons, one is for the documents, another is per
>>> Cgroup TODO list from Tejun -- https://lkml.org/lkml/2012/9/13/542
>>>
>>> . Misc issues
>>>
>>>   * Sort & unique when listing tasks.  Even the documentation says it
>>>     doesn't happen but we have a good hunk of code doing it in
>>>     cgroup.c.  I'm gonna rip it out at some point.  Again, if you
>>>     don't like it, scream.
>>
>> Yeah, I figured, and since he also wants to move to a place where
>> cgroups are always manipulated using a library, not by hand, it
>> would make sense to get rid of the overhead there.  I just don't
>> really like either one of those :)  And in the list of things to
>> clean up in cgroups, this seems like one that can wait.
> 
> I want it gone but last time I pinged there were some people depending
> on it, so I don't think I can.  I think we'll have to put it on ice
> for now at least.  :(
Thanks both of you for the response :)  So I'll put it aside for now.

Regards,
-Jeff

[PATCH] cgroup: don't show pid list on tasks/procs in ascending order

[Jeff Liu]

Hello,

Currently, the pid list shown on cgroup->procs & tasks is in ascending order.  However, this list is not
guaranteed to be sorted according to the following records mentioned at Documentation/cgroups/cgroups.txt.

 - tasks: list of tasks (by PID) attached to that cgroup.  This list
   is not guaranteed to be sorted. 
 - cgroup.procs: list of thread group IDs in the cgroup.  This list is
   not guaranteed to be sorted or free of duplicate TGIDs, and userspace
   should sort/uniquify the list if this property is required.

This patch remove the sorting function to make the default behavior of pid list be consistent with
the document.

Without patch:
# echo 2942 >> /cgroup/test/tasks 
# echo 2803 >> /cgroup/test/tasks 
# echo 2911 >> /cgroup/test/tasks
# echo 2761 >> /cgroup/test/tasks 
# cat /cgroup/test/tasks 
2761
2803
2911
2942
# cat /cgroup/test/cgroup.procs
2761
2803
2911
2942

Patch applied:
# echo 3042 >> /cgroup/testfixed/tasks
# echo 2916 >> /cgroup/testfixed/tasks
# echo 2781 >> /cgroup/testfixed/tasks
# echo 2527 >> /cgroup/testfixed/tasks
# cat /cgroup/testfixed/tasks 
2916
3042
2527
2781
# cat /cgroup/testfixed/cgroup.procs 
2916
3042
2527
2781


Signed-off-by: Jie Liu <jeff.liu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

---
 kernel/cgroup.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f24f724..2b5b477 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -47,7 +47,6 @@
 #include <linux/magic.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
-#include <linux/sort.h>
 #include <linux/kmod.h>
 #include <linux/module.h>
 #include <linux/delayacct.h>
@@ -3374,11 +3373,6 @@ after:
 	return dest;
 }
 
-static int cmppid(const void *a, const void *b)
-{
-	return *(pid_t *)a - *(pid_t *)b;
-}
-
 /*
  * find the appropriate pidlist for our purpose (given procs vs tasks)
  * returns with the lock on that pidlist already held, and takes care
@@ -3463,8 +3457,7 @@ static int pidlist_array_load(struct cgroup *cgrp, enum cgroup_filetype type,
 	}
 	cgroup_iter_end(cgrp, &it);
 	length = n;
-	/* now sort & (if procs) strip out duplicates */
-	sort(array, length, sizeof(pid_t), cmppid, NULL);
+	/* if procs, strip out duplicates */
 	if (type == CGROUP_FILE_PROCS)
 		length = pidlist_uniq(&array, length);
 	l = cgroup_pidlist_find(cgrp, type);
-- 
1.7.9.5