[PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Denys Vlasenko]

POSIX and SUS say that setpriority(PRIO_PROCESS) should affect
all threads in the process:

SYNOPSIS
       #include <sys/resource.h>
       int getpriority(int which, id_t who);
       int setpriority(int which, id_t who, int value);
DESCRIPTION
       ...
       The  nice value set with setpriority() shall be applied to
       the process. If the process is multi-threaded, the nice value
       shall affect all system scope threads in the process.

Currently, this is not the case. While PRIO_PGRP and PRIO_USER
do set priority to the selected group of processes, PRIO_PROCESS
sets priority only for the thread with tid == who.

This mostly goes unnoticed because single-threaded processes have
tid == pid.

However, in multi-threaded processes

setpriority(PRIO_PROCESS, getpid(), value)

sets new priority only for the thread with tid == pid, leaving
all other threads unaffected. This is wrong.

Attached patch changes setpriority(PRIO_PROCESS) to set priority
for all threads with selected pid. getpriority is changed accordingly,
to return the (numerical) max of all threads' priority.

In order to allow priority of individual threads to be manipulated,
patch adds PRIO_THREAD which acts on single thread, always.

Since there may be programs which use the fact that

setpriority(PRIO_PROCESS, tid, value)

prior to this patch was setting priority for selected thread,
this behavior is retained in case when tid != pid.

IOW: with PRIO_PROCESS, if pid specifies a thread group leader,
all threads' prios are set. Otherwise, only selected thread's priority
is set. (Alternative can be to just fail with ESRCH).

getpriority() is acting similarly.

Patch is run tested. I will post test program etc as a reply.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
--
vda



diff --git a/include/linux/resource.h b/include/linux/resource.h
index aaa423a..f292690 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -51,6 +51,7 @@ struct rlimit {
 #define	PRIO_PROCESS	0
 #define	PRIO_PGRP	1
 #define	PRIO_USER	2
+#define	PRIO_THREAD	3
 
 /*
  * Limit the stack by to some sane default: root can always
diff --git a/kernel/sys.c b/kernel/sys.c
index 038a7bc..d339c1a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	int error = -EINVAL;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		goto out;
 
 	/* normalize: avoid signed division (rounding problems) */
@@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 			if (p)
 				error = set_one_prio(p, niceval, error);
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->pid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					error = set_one_prio(p, niceval, error);
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);
@@ -206,14 +219,14 @@ asmlinkage long sys_getpriority(int which, int
who)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	long niceval, retval = -ESRCH;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		return -EINVAL;
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -224,6 +237,21 @@ asmlinkage long sys_getpriority(int which, int who)
 					retval = niceval;
 			}
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->pid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					niceval = 20 - task_nice(p);
+					if (niceval > retval)
+						retval = niceval;
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

On Mon, 2008-09-01 at 16:12 +0200, Denys Vlasenko wrote:
> Patch is run tested. I will post test program etc as a reply.

Test program is attached.
Build with "gcc -o prio_vda3 prio_vda3.c -lpthread".
Output:

start test program
CHILD1: TID:3269 getpriority:0
CHILD1: PID:3269 getpriority:0
setpriority(PRIO_PROCESS, 3269, -10)
THREAD CHILD: TID:3270 getpriority:-2
THREAD CHILD: PID:3269 getpriority:-10
THREAD CHILD: TID:3271 getpriority:-4
THREAD CHILD: PID:3269 getpriority:-10
THREAD CHILD: TID:3272 getpriority:-6
THREAD CHILD: PID:3269 getpriority:-10
THREAD CHILD: TID:3273 getpriority:-8
THREAD CHILD: PID:3269 getpriority:-10
CHILD2: TID:3269 getpriority:-10
CHILD2: PID:3269 getpriority:-10
Joined!
Joined!
Joined!
Joined!

log of instrumented kernel:

 (no threads created yet, only one tid exists)
 getpriority:
PRIO_PROCESS: pid 3269 tgid 3269: niceval=20
PRIO_PROCESS: returning retval=20
PRIO_PROCESS: pid 3269 tgid 3269: niceval=20
PRIO_PROCESS: returning retval=20
 (5 threads are created)
 setpriority(PRIO_PROCESS, pid, -10):
setting prio of pid 3269 tgid 3269 to -10
setting prio of pid 3270 tgid 3269 to -10
setting prio of pid 3271 tgid 3269 to -10
setting prio of pid 3272 tgid 3269 to -10
setting prio of pid 3273 tgid 3269 to -10
 setpriority(3 /*PRIO_THREAD*/, syscall(SYS_gettid), -1);
setting prio of pid 3270 tgid 3269 to -1
 setpriority(PRIO_PROCESS, syscall(SYS_gettid), -2);
setting prio of pid 3270 tgid 3269 to -2

Above you see how setpriority(PRIO_PROCESS) can set prio for all threads
or only for one.

  prio = getpriority(PRIO_PROCESS, tid);
PRIO_PROCESS: pid 3270 tgid 3269: niceval=22
PRIO_PROCESS: returning retval=22
  prio = getpriority(PRIO_PROCESS, pid);
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: pid 3270 tgid 3269: niceval=22
PRIO_PROCESS: pid 3271 tgid 3269: niceval=30
PRIO_PROCESS: pid 3272 tgid 3269: niceval=30
PRIO_PROCESS: pid 3273 tgid 3269: niceval=30
PRIO_PROCESS: returning retval=30

Same for getpriority.

The rest is analogous:

setting prio of pid 3271 tgid 3269 to -3
setting prio of pid 3271 tgid 3269 to -4
PRIO_PROCESS: pid 3271 tgid 3269: niceval=24
PRIO_PROCESS: returning retval=24
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: pid 3271 tgid 3269: niceval=24
PRIO_PROCESS: pid 3272 tgid 3269: niceval=30
PRIO_PROCESS: pid 3273 tgid 3269: niceval=30
PRIO_PROCESS: returning retval=30
setting prio of pid 3272 tgid 3269 to -5
setting prio of pid 3272 tgid 3269 to -6
PRIO_PROCESS: pid 3272 tgid 3269: niceval=26
PRIO_PROCESS: returning retval=26
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: pid 3272 tgid 3269: niceval=26
PRIO_PROCESS: pid 3273 tgid 3269: niceval=30
PRIO_PROCESS: returning retval=30
setting prio of pid 3273 tgid 3269 to -7
setting prio of pid 3273 tgid 3269 to -8
PRIO_PROCESS: pid 3273 tgid 3269: niceval=28
PRIO_PROCESS: returning retval=28
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: pid 3273 tgid 3269: niceval=28
PRIO_PROCESS: returning retval=30
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: returning retval=30
PRIO_PROCESS: pid 3269 tgid 3269: niceval=30
PRIO_PROCESS: returning retval=30

Patch with instrumentation is attached too.
--
vda



[-- Attachment #2: prio_vda3.c --]
[-- Type: text/x-csrc, Size: 1632 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <errno.h>
#include <signal.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/time.h>
#include <sys/wait.h>
#include <sys/resource.h>
#include <memory.h>

#define THREAD_NUM 5

static void printprio(const char *pfx)
{
	int prio;
	pid_t tid = syscall(SYS_gettid);
	pid_t pid = getpid();
	prio = getpriority(PRIO_PROCESS, tid);
	printf("%sTID:%d getpriority:%d\n", pfx, tid, prio);
	prio = getpriority(PRIO_PROCESS, pid);
	printf("%sPID:%d getpriority:%d\n", pfx, pid, prio);
}

int threadno;

static void* threadfunc(void *dummy)
{
	sleep(1); /* wait for parent to setpriority */
	setpriority(3 /*PRIO_THREAD*/, syscall(SYS_gettid), -(++threadno));
	setpriority(PRIO_PROCESS, syscall(SYS_gettid), -(++threadno));
	printprio("THREAD CHILD: ");
	pthread_exit(0);
}

int main()
{
	pid_t pid = fork();
	if (pid) {
		/* parent */
		int status;
		printf("start test program\n");
		sleep(1); /* wait for threads to start in child */
		printf("setpriority(PRIO_PROCESS, %d, -10)\n", pid);
		if (setpriority(PRIO_PROCESS, pid, -10) < 0) {
			perror("setpriority PRIO_PROCESS");
			exit(1);
		}
		wait(&status);
	} else { 
		/* child */
		int i;
		pthread_t thread_id[THREAD_NUM];

		printprio("CHILD1: ");
		for (i = 1; i < THREAD_NUM; i++) {
			usleep(30*1000);
			pthread_create(&thread_id[i], NULL, &threadfunc, NULL);
		}

		usleep(30*1000);
		sleep(1); /* wait for parent to setpriority */
		printprio("CHILD2: ");

		for (i = 1; i < THREAD_NUM; i++) {
			pthread_join(thread_id[i], NULL);
			printf("Joined!\n");
		}
	}
	return 0;
}

[-- Attachment #3: setprio_POSIX_instrumented.diff --]
[-- Type: text/x-patch, Size: 1672 bytes --]

--- a/kernel/sys.c.orig	2008-09-01 12:50:22.000000000 +0200
+++ b/kernel/sys.c	2008-09-01 15:11:45.000000000 +0200
@@ -162,15 +162,22 @@
 			else
 				p = current;
 			if (p)
+{printk("setting prio of pid %d tgid %d to %d\n", p->pid, p->tgid, niceval);
 				error = set_one_prio(p, niceval, error);
+}
 			break;
 		case PRIO_PROCESS:
 			if (who)
 				pid = find_vpid(who);
-			else
+			else {
 				pid = task_pid(current);
+				who = current->pid;
+			}
 			do_each_pid_thread(pid, PIDTYPE_PID, p) {
-				error = set_one_prio(p, niceval, error);
+				if (who == p->pid || who == p->tgid) {
+printk("setting prio of pid %d tgid %d to %d\n", p->pid, p->tgid, niceval);
+					error = set_one_prio(p, niceval, error);
+				}
 			} while_each_pid_thread(pid, PIDTYPE_PID, p);
 			break;
 		case PRIO_PGRP:
@@ -231,18 +238,25 @@
 				niceval = 20 - task_nice(p);
 				if (niceval > retval)
 					retval = niceval;
+printk("PRIO_THREAD: pid %d tgid %d: niceval=%ld retval=%ld\n", p->pid, p->tgid, niceval, retval);
 			}
 			break;
 		case PRIO_PROCESS:
 			if (who)
 				pid = find_vpid(who);
-			else
+			else {
 				pid = task_pid(current);
+				who = current->pid;
+			}
 			do_each_pid_thread(pid, PIDTYPE_PID, p) {
-				niceval = 20 - task_nice(p);
-				if (niceval > retval)
-					retval = niceval;
+				if (who == p->pid || who == p->tgid) {
+					niceval = 20 - task_nice(p);
+					if (niceval > retval)
+						retval = niceval;
+printk("PRIO_PROCESS: pid %d tgid %d: niceval=%ld\n", p->pid, p->tgid, niceval);
+				}
 			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+printk("PRIO_PROCESS: returning retval=%ld\n", retval);
 			break;
 		case PRIO_PGRP:
 			if (who)

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Denys Vlasenko]

On Mon, 2008-09-01 at 16:12 +0200, Denys Vlasenko wrote:
> Patch is run tested. I will post test program etc as a reply.

Looks like Evolution word-wrapped the patch. Let me try again.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
--
vda


diff --git a/include/linux/resource.h b/include/linux/resource.h
index aaa423a..f292690 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -51,6 +51,7 @@ struct rlimit {
 #define	PRIO_PROCESS	0
 #define	PRIO_PGRP	1
 #define	PRIO_USER	2
+#define	PRIO_THREAD	3
 
 /*
  * Limit the stack by to some sane default: root can always
diff --git a/kernel/sys.c b/kernel/sys.c
index 038a7bc..d339c1a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	int error = -EINVAL;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		goto out;
 
 	/* normalize: avoid signed division (rounding problems) */
@@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
 			if (p)
 				error = set_one_prio(p, niceval, error);
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->pid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					error = set_one_prio(p, niceval, error);
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);
@@ -206,14 +219,14 @@ asmlinkage long sys_getpriority(int which, int who)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	long niceval, retval = -ESRCH;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		return -EINVAL;
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -224,6 +237,21 @@ asmlinkage long sys_getpriority(int which, int who)
 					retval = niceval;
 			}
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->pid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					niceval = 20 - task_nice(p);
+					if (niceval > retval)
+						retval = niceval;
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Peter Zijlstra]

On Mon, 2008-09-01 at 16:42 +0200, Denys Vlasenko wrote:
> On Mon, 2008-09-01 at 16:12 +0200, Denys Vlasenko wrote:
> > Patch is run tested. I will post test program etc as a reply.
> 
> Looks like Evolution word-wrapped the patch. Let me try again.

Patch looks simple enough, although a few comments below.
Also, I guess the glibc people (Ulrich added to CC) might have an
opinion.

> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> --
> vda
> 
> 
> diff --git a/include/linux/resource.h b/include/linux/resource.h
> index aaa423a..f292690 100644
> --- a/include/linux/resource.h
> +++ b/include/linux/resource.h
> @@ -51,6 +51,7 @@ struct rlimit {
>  #define	PRIO_PROCESS	0
>  #define	PRIO_PGRP	1
>  #define	PRIO_USER	2
> +#define	PRIO_THREAD	3
>  
>  /*
>   * Limit the stack by to some sane default: root can always
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 038a7bc..d339c1a 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
>  	struct task_struct *g, *p;
>  	struct user_struct *user;
>  	int error = -EINVAL;
> -	struct pid *pgrp;
> +	struct pid *pgrp, *pid;
>  
> -	if (which > PRIO_USER || which < PRIO_PROCESS)
> +	if (which > PRIO_THREAD || which < PRIO_PROCESS)
>  		goto out;
>  
>  	/* normalize: avoid signed division (rounding problems) */
> @@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
>  
>  	read_lock(&tasklist_lock);
>  	switch (which) {
> -		case PRIO_PROCESS:
> +		case PRIO_THREAD:
>  			if (who)
>  				p = find_task_by_vpid(who);
>  			else
> @@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
>  			if (p)
>  				error = set_one_prio(p, niceval, error);
>  			break;
> +		case PRIO_PROCESS:
> +			if (who)
> +				pid = find_vpid(who);
> +			else {
> +				pid = task_pid(current);
> +				who = current->pid;
> +			}
> +			do_each_pid_thread(pid, PIDTYPE_PID, p) {
> +				if (who == p->pid || who == p->tgid) {
> +					error = set_one_prio(p, niceval, error);
> +				}
> +			} while_each_pid_thread(pid, PIDTYPE_PID, p);

I worry about destroying the return value here, support one thread
fails, but the next succeeds, should we still report failure?

> +			break;
>  		case PRIO_PGRP:
>  			if (who)
>  				pgrp = find_vpid(who);
> @@ -206,14 +219,14 @@ asmlinkage long sys_getpriority(int which, int who)
>  	struct task_struct *g, *p;
>  	struct user_struct *user;
>  	long niceval, retval = -ESRCH;
> -	struct pid *pgrp;
> +	struct pid *pgrp, *pid;
>  
> -	if (which > PRIO_USER || which < PRIO_PROCESS)
> +	if (which > PRIO_THREAD || which < PRIO_PROCESS)
>  		return -EINVAL;
>  
>  	read_lock(&tasklist_lock);
>  	switch (which) {
> -		case PRIO_PROCESS:
> +		case PRIO_THREAD:
>  			if (who)
>  				p = find_task_by_vpid(who);
>  			else
> @@ -224,6 +237,21 @@ asmlinkage long sys_getpriority(int which, int who)
>  					retval = niceval;
>  			}
>  			break;
> +		case PRIO_PROCESS:
> +			if (who)
> +				pid = find_vpid(who);
> +			else {
> +				pid = task_pid(current);
> +				who = current->pid;
> +			}
> +			do_each_pid_thread(pid, PIDTYPE_PID, p) {
> +				if (who == p->pid || who == p->tgid) {
> +					niceval = 20 - task_nice(p);
> +					if (niceval > retval)
> +						retval = niceval;
> +				}
> +			} while_each_pid_thread(pid, PIDTYPE_PID, p);

So we basically return the highest prio amongst the threads?

> +			break;
>  		case PRIO_PGRP:
>  			if (who)
>  				pgrp = find_vpid(who);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Peter Zijlstra]

On Mon, 2008-09-01 at 17:08 +0200, Peter Zijlstra wrote:
> On Mon, 2008-09-01 at 16:42 +0200, Denys Vlasenko wrote:
> > On Mon, 2008-09-01 at 16:12 +0200, Denys Vlasenko wrote:
> > > Patch is run tested. I will post test program etc as a reply.
> > 
> > Looks like Evolution word-wrapped the patch. Let me try again.
> 
> Patch looks simple enough, although a few comments below.
> Also, I guess the glibc people (Ulrich added to CC) might have an
> opinion.
> 
> > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > --

> > +		case PRIO_PROCESS:
> > +			if (who)
> > +				pid = find_vpid(who);
> > +			else {
> > +				pid = task_pid(current);
> > +				who = current->pid;
> > +			}
> > +			do_each_pid_thread(pid, PIDTYPE_PID, p) {
> > +				if (who == p->pid || who == p->tgid) {
> > +					error = set_one_prio(p, niceval, error);
> > +				}
> > +			} while_each_pid_thread(pid, PIDTYPE_PID, p);
> 
> I worry about destroying the return value here, support one thread
> fails, but the next succeeds, should we still report failure?

Ok - got fooled by this funny set_one_prio() function. It passes the old
error value and maintains it if no new error occurs (except for -ESRCH,
but I guess people know wth they're doing).

So I'll retract my concern.

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Denys Vlasenko]

On Mon, 2008-09-01 at 17:08 +0200, Peter Zijlstra wrote:
> On Mon, 2008-09-01 at 16:42 +0200, Denys Vlasenko wrote:
> > On Mon, 2008-09-01 at 16:12 +0200, Denys Vlasenko wrote:
> > > Patch is run tested. I will post test program etc as a reply.
> > 
> > Looks like Evolution word-wrapped the patch. Let me try again.
> 
> Patch looks simple enough, although a few comments below.
> Also, I guess the glibc people (Ulrich added to CC) might have an
> opinion.
> 
> > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> > --
> > vda
> > 
> > 
> > diff --git a/include/linux/resource.h b/include/linux/resource.h
> > index aaa423a..f292690 100644
> > --- a/include/linux/resource.h
> > +++ b/include/linux/resource.h
> > @@ -51,6 +51,7 @@ struct rlimit {
> >  #define	PRIO_PROCESS	0
> >  #define	PRIO_PGRP	1
> >  #define	PRIO_USER	2
> > +#define	PRIO_THREAD	3
> >  
> >  /*
> >   * Limit the stack by to some sane default: root can always
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 038a7bc..d339c1a 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
> >  	struct task_struct *g, *p;
> >  	struct user_struct *user;
> >  	int error = -EINVAL;
> > -	struct pid *pgrp;
> > +	struct pid *pgrp, *pid;
> >  
> > -	if (which > PRIO_USER || which < PRIO_PROCESS)
> > +	if (which > PRIO_THREAD || which < PRIO_PROCESS)
> >  		goto out;
> >  
> >  	/* normalize: avoid signed division (rounding problems) */
> > @@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
> >  
> >  	read_lock(&tasklist_lock);
> >  	switch (which) {
> > -		case PRIO_PROCESS:
> > +		case PRIO_THREAD:
> >  			if (who)
> >  				p = find_task_by_vpid(who);
> >  			else
> > @@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who, int niceval)
> >  			if (p)
> >  				error = set_one_prio(p, niceval, error);
> >  			break;
> > +		case PRIO_PROCESS:
> > +			if (who)
> > +				pid = find_vpid(who);
> > +			else {
> > +				pid = task_pid(current);
> > +				who = current->pid;
> > +			}
> > +			do_each_pid_thread(pid, PIDTYPE_PID, p) {
> > +				if (who == p->pid || who == p->tgid) {
> > +					error = set_one_prio(p, niceval, error);
> > +				}
> > +			} while_each_pid_thread(pid, PIDTYPE_PID, p);
> 
> I worry about destroying the return value here, support one thread
> fails, but the next succeeds, should we still report failure?

Hmm. I think we should fail only if they all failed.
I don't feel strongly either way. Ulrich what do you prefer?

> > +		case PRIO_PROCESS:
> > +			if (who)
> > +				pid = find_vpid(who);
> > +			else {
> > +				pid = task_pid(current);
> > +				who = current->pid;
> > +			}
> > +			do_each_pid_thread(pid, PIDTYPE_PID, p) {
> > +				if (who == p->pid || who == p->tgid) {
> > +					niceval = 20 - task_nice(p);
> > +					if (niceval > retval)
> > +						retval = niceval;
> > +				}
> > +			} while_each_pid_thread(pid, PIDTYPE_PID, p);
> 
> So we basically return the highest prio amongst the threads?

Yes. This is analogous to what happens with PRIO_USER etc,
no surprises here.
--
vda

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Peter Zijlstra wrote:
> Also, I guess the glibc people (Ulrich added to CC) might have an
> opinion.

Looks good.

I just wish we could get something similar for setuid and friends.
Perhaps after David cred patches?

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAki8LVUACgkQ2ijCOnn/RHTWYQCePQKadvZWkC8ZoH+DOHd/N+et
KwAAoKxASCCigXbO67/rSwggRIkemPKw
=MixL
-----END PGP SIGNATURE-----

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Michael Kerrisk]

Denys,

On 9/1/08, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> POSIX and SUS say that setpriority(PRIO_PROCESS) should affect
>  all threads in the process:
>
>  SYNOPSIS
>        #include <sys/resource.h>
>        int getpriority(int which, id_t who);
>        int setpriority(int which, id_t who, int value);
>  DESCRIPTION
>        ...
>        The  nice value set with setpriority() shall be applied to
>        the process. If the process is multi-threaded, the nice value
>        shall affect all system scope threads in the process.
>
>  Currently, this is not the case. While PRIO_PGRP and PRIO_USER
>  do set priority to the selected group of processes, PRIO_PROCESS
>  sets priority only for the thread with tid == who.
>
>  This mostly goes unnoticed because single-threaded processes have
>  tid == pid.
>
>  However, in multi-threaded processes
>
>  setpriority(PRIO_PROCESS, getpid(), value)
>
>  sets new priority only for the thread with tid == pid, leaving
>  all other threads unaffected. This is wrong.
>
>  Attached patch changes setpriority(PRIO_PROCESS) to set priority
>  for all threads with selected pid. getpriority is changed accordingly,
>  to return the (numerical) max of all threads' priority.
>
>  In order to allow priority of individual threads to be manipulated,
>  patch adds PRIO_THREAD which acts on single thread, always.
>
>  Since there may be programs which use the fact that
>
>  setpriority(PRIO_PROCESS, tid, value)
>
>  prior to this patch was setting priority for selected thread,
>  this behavior is retained in case when tid != pid.
>
>  IOW: with PRIO_PROCESS, if pid specifies a thread group leader,
>  all threads' prios are set. Otherwise, only selected thread's priority
>  is set. (Alternative can be to just fail with ESRCH).
>
>  getpriority() is acting similarly.
>
>  Patch is run tested. I will post test program etc as a reply.

Tested-by: Michael Kerrisk <mtk.manpages@gmail.com>

Please do CC me on API changes, so that they might get documented in
the man pages.

Thanks for this work.  (I'm not sure whether or not it's a response to
my bug report, http://bugzilla.kernel.org/show_bug.cgi?id=6258 )

I tested your patch.  Most things seem to work as I would expect, but
there is one strangeness.

I would expect
setpriority(PRIO_PROCESS, getpid())
and
setpriority(PRIO_PROCESS, 0)
to have the same affect (because: which == PRIO_PRCESS, who == 0
conventionally means "the calling process").

But they do not: the latter call only changes the priority of the
calling thread.  Is this intended?

My test program is at the end of this mail.  Here are two tests
demonstrating the behavior in each of the above cases:

$ sudo ./thread_nice_test p p
root's password:
main(): TID = 6574, PID = 6574
main(): initial nice value is 0

Thread 1: TID = 6578
Thread 2: TID = 6579
Loop 0
    process nice value is 0
    main(): nice value is 0
    Thread 1: nice value is 0
    Thread 2: nice value is 0
Thread 1: setpriority() succeeded (0, 6574)
Loop 1
    process nice value is -10
    main(): nice value is -10
    Thread 1: nice value is -10
    Thread 2: nice value is -10
Loop 2
    process nice value is -10
    main(): nice value is -10
    Thread 1: nice value is -10
    Thread 2: nice value is -10


$ sudo ./thread_nice_test p 0
main(): TID = 6590, PID = 6590
main(): initial nice value is 0

Thread 1: TID = 6594
Thread 2: TID = 6595
Loop 0
    process nice value is 0
    main(): nice value is 0
    Thread 1: nice value is 0
    Thread 2: nice value is 0
Thread 1: setpriority() succeeded (0, 0)
Loop 1
    process nice value is -10
    main(): nice value is 0
    Thread 1: nice value is -10
    Thread 2: nice value is 0
Loop 2
    process nice value is -10
    main(): nice value is 0
    Thread 1: nice value is -10
    Thread 2: nice value is 0

=====

/* thread_nice_test.c

   Copyright 2008, Linux Foundation, written by Michael Kerrisk
        <mtk.manpages@gmail.com>
   Licensed under GNU GPL version 2 or later

   Creates two subthreads.
   The first thread does a setpriority() with arguments
   based on command-line arguments.
   The main() program loops, using getpriority() to monitor
   priority of all threads, and the process as a whole.
*/
#define _GNU_SOURCE
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <limits.h>
#include <string.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>

#define PRIO_THREAD 3

#define errExit(msg)            { perror(msg); exit(EXIT_FAILURE); }

#define errExitEN(en, msg)      { errno = en; perror(msg); \
                                  exit(EXIT_FAILURE); }

#define fatal(msg)              { fprintf(stderr, "%s\n", msg); \
                                  exit(EXIT_FAILURE); }

#define NEW_NICE -10

static pid_t
gettid(void)
{
    return syscall(SYS_gettid);
}

static pid_t t1_tid, t2_tid, main_tid;
static char cwhich, cwho;

static void *
tf1_nice(void *x)
{
    int which, who;

    t1_tid = gettid();
    printf("Thread 1: TID = %ld\n", (long) gettid());

    sleep(1);

    which = (cwhich == 'p') ? PRIO_PROCESS :
            (cwhich == 't') ? PRIO_THREAD :
            9999;       /* Invalid value */
    who =   (cwho == '0') ? 0 :
            (cwho == 't') ? gettid() :
            (cwho == 'p') ? getpid() :
            (cwho == 'm') ? main_tid :
            -1;                 /* Invalid value */
    if (setpriority(which, who, NEW_NICE) == -1)
        errExit("setpriority");

    printf("Thread 1: setpriority() succeeded (%d, %d)\n", which, who);

    sleep(10);
    printf("Thread 1 terminating\n");
    return NULL;
}

static void *
tf2_nice(void *x)
{
    t2_tid = gettid();
    printf("Thread 2: TID = %ld\n", (long) gettid());

    sleep (10);

    printf("Thread 2 terminating\n");
    return NULL;
}


int
main(int argc, char *argv[])
{
    pthread_t t1, t2;
    int s;
    int nval;
    int j;

    if (argc != 3) {
        fprintf(stderr, "Usage: %s <which> <who>\n", argv[0]);
        fprintf(stderr, "which is\n");
        fprintf(stderr, "    p   PRIO_PROCESS\n");
        fprintf(stderr, "    t   PRIO_THREAD\n");
        fprintf(stderr, "who is:\n");
        fprintf(stderr, "    0   0\n");
        fprintf(stderr, "    t   gettid()\n");
        fprintf(stderr, "    p   getpid()\n");
        fprintf(stderr, "    m   TID of main()\n");
        exit(EXIT_FAILURE);
    }

    main_tid = gettid();
    cwhich = argv[1][0];
    cwho = argv[2][0];

    printf("main(): TID = %ld, PID = %ld\n",
            (long) gettid(), (long) getpid());

    errno = 0;
    nval = getpriority(PRIO_PROCESS, gettid());
    if (nval == -1 && errno != 0)
        errExit("getpriority");
    printf("main(): initial nice value is %d\n", nval);


    printf("\n");

    s = pthread_create(&t1, NULL, tf1_nice, (void *) 1);
    if (s != 0)
        errExitEN(s, "pthread_create");

    s = pthread_create(&t2, NULL, tf2_nice, (void *) 2);
    if (s != 0)
        errExitEN(s, "pthread_create");

    usleep(200000);  /* Allow time for subthreads to set global vars */

    for (j = 0; j < 3; j++) {
        printf("Loop %d\n", j);
        errno = 0;
        nval = getpriority(PRIO_PROCESS, gettid());
        if (nval == -1 && errno != 0)
            errExit("getpriority");
        printf("    process nice value is %d\n", nval);

        errno = 0;
        nval = getpriority(PRIO_THREAD, gettid());
        if (nval == -1 && errno != 0)
            errExit("getpriority");
        printf("    main(): nice value is %d\n", nval);

        errno = 0;
        nval = getpriority(PRIO_THREAD, t1_tid);
        if (nval == -1 && errno != 0)
            errExit("getpriority");
        printf("    Thread 1: nice value is %d\n", nval);

        errno = 0;
        nval = getpriority(PRIO_THREAD, t2_tid);
        if (nval == -1 && errno != 0)
            errExit("getpriority");
        printf("    Thread 2: nice value is %d\n", nval);

        sleep(2);
    }

    exit(EXIT_SUCCESS);
} /* main */

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Chris Friesen]

Michael Kerrisk wrote:

> On 9/1/08, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>> Since there may be programs which use the fact that
>>
>> setpriority(PRIO_PROCESS, tid, value)
>>
>> prior to this patch was setting priority for selected thread,
>> this behavior is retained in case when tid != pid.
>>
>> IOW: with PRIO_PROCESS, if pid specifies a thread group leader,
>> all threads' prios are set. Otherwise, only selected thread's priority
>> is set. (Alternative can be to just fail with ESRCH).


> I would expect
> setpriority(PRIO_PROCESS, getpid())
> and
> setpriority(PRIO_PROCESS, 0)
> to have the same affect (because: which == PRIO_PRCESS, who == 0
> conventionally means "the calling process").
> 
> But they do not: the latter call only changes the priority of the
> calling thread.  Is this intended?

The patch interprets 0 as the current pid rather than the current tgid.

It's up for discussion whether we should preserve old behaviour when 
specifying 0, or use a new and arguably more logical behaviour but 
possibly break old apps.

Chris

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Michael Kerrisk]

Chris,

On Tue, Sep 9, 2008 at 6:42 PM, Chris Friesen <cfriesen@nortel.com> wrote:
> Michael Kerrisk wrote:
>
>> On 9/1/08, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>>
>>> Since there may be programs which use the fact that
>>>
>>> setpriority(PRIO_PROCESS, tid, value)
>>>
>>> prior to this patch was setting priority for selected thread,
>>> this behavior is retained in case when tid != pid.
>>>
>>> IOW: with PRIO_PROCESS, if pid specifies a thread group leader,
>>> all threads' prios are set. Otherwise, only selected thread's priority
>>> is set. (Alternative can be to just fail with ESRCH).
>
>
>> I would expect
>> setpriority(PRIO_PROCESS, getpid())
>> and
>> setpriority(PRIO_PROCESS, 0)
>> to have the same affect (because: which == PRIO_PRCESS, who == 0
>> conventionally means "the calling process").
>>
>> But they do not: the latter call only changes the priority of the
>> calling thread.  Is this intended?
>
> The patch interprets 0 as the current pid rather than the current tgid.
> It's up for discussion whether we should preserve old behaviour when
> specifying 0, or use a new and arguably more logical behaviour but possibly
> break old apps.

AFAICS, interpreting 0 as the TGID would be more consistent with those
parts of the interface that are specified by POSIX.1.


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Denys Vlasenko]

On Tue, 2008-09-09 at 17:45 +0200, Michael Kerrisk wrote:
> > Patch is run tested. I will post test program etc as a reply.
> 
> Tested-by: Michael Kerrisk <mtk.manpages@gmail.com>
> 
> Please do CC me on API changes, so that they might get documented in
> the man pages.
> 
> Thanks for this work.  (I'm not sure whether or not it's a response to
> my bug report, http://bugzilla.kernel.org/show_bug.cgi?id=6258 )

I worked on it because of this bug report:

https://bugzilla.redhat.com/show_bug.cgi?id=455251

> I tested your patch.  Most things seem to work as I would expect, but
> there is one strangeness.
> 
> I would expect
> setpriority(PRIO_PROCESS, getpid())
> and
> setpriority(PRIO_PROCESS, 0)
> to have the same affect (because: which == PRIO_PRCESS, who == 0
> conventionally means "the calling process").
> 
> But they do not: the latter call only changes the priority of the
> calling thread.  Is this intended?

No, it was not intended. I think this error is here:

+               case PRIO_PROCESS:
+                       if (who)
+                               pid = find_vpid(who);
+                       else {
+                               pid = task_pid(current);
+                               who = current->pid;
+                       }

I was confused. ->pid is TID, had to use ->tgid to get PID.
The fix: replace
who = current->pid;
with
who = current->tgid;
There are two places where you need to do it.

Updated patch is below.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
--
vda



diff --git a/include/linux/resource.h b/include/linux/resource.h
index aaa423a..f292690 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -51,6 +51,7 @@ struct rlimit {
 #define	PRIO_PROCESS	0
 #define	PRIO_PGRP	1
 #define	PRIO_USER	2
+#define	PRIO_THREAD	3
 
 /*
  * Limit the stack by to some sane default: root can always
diff --git a/kernel/sys.c b/kernel/sys.c
index 038a7bc..d339c1a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	int error = -EINVAL;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		goto out;
 
 	/* normalize: avoid signed division (rounding problems) */
@@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who,
int niceval)
 			if (p)
 				error = set_one_prio(p, niceval, error);
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->tgid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					error = set_one_prio(p, niceval, error);
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);
@@ -206,14 +219,14 @@ asmlinkage long sys_getpriority(int which, int
who)
 	struct task_struct *g, *p;
 	struct user_struct *user;
 	long niceval, retval = -ESRCH;
-	struct pid *pgrp;
+	struct pid *pgrp, *pid;
 
-	if (which > PRIO_USER || which < PRIO_PROCESS)
+	if (which > PRIO_THREAD || which < PRIO_PROCESS)
 		return -EINVAL;
 
 	read_lock(&tasklist_lock);
 	switch (which) {
-		case PRIO_PROCESS:
+		case PRIO_THREAD:
 			if (who)
 				p = find_task_by_vpid(who);
 			else
@@ -224,6 +237,21 @@ asmlinkage long sys_getpriority(int which, int who)
 					retval = niceval;
 			}
 			break;
+		case PRIO_PROCESS:
+			if (who)
+				pid = find_vpid(who);
+			else {
+				pid = task_pid(current);
+				who = current->tgid;
+			}
+			do_each_pid_thread(pid, PIDTYPE_PID, p) {
+				if (who == p->pid || who == p->tgid) {
+					niceval = 20 - task_nice(p);
+					if (niceval > retval)
+						retval = niceval;
+				}
+			} while_each_pid_thread(pid, PIDTYPE_PID, p);
+			break;
 		case PRIO_PGRP:
 			if (who)
 				pgrp = find_vpid(who);

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Michael Kerrisk]

Denys,

On 9/10/08, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On Tue, 2008-09-09 at 17:45 +0200, Michael Kerrisk wrote:
> > > Patch is run tested. I will post test program etc as a reply.
> >
> > Tested-by: Michael Kerrisk <mtk.manpages@gmail.com>
> >
> > Please do CC me on API changes, so that they might get documented in
> > the man pages.
> >
> > Thanks for this work.  (I'm not sure whether or not it's a response to
> > my bug report, http://bugzilla.kernel.org/show_bug.cgi?id=6258 )
>
> I worked on it because of this bug report:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=455251

(That report is inaccessible, although I have some kind of bugzilla
login there: "You are not authorized to access bug #455251.")

> > I tested your patch.  Most things seem to work as I would expect, but
> > there is one strangeness.
> >
> > I would expect
> > setpriority(PRIO_PROCESS, getpid())
> > and
> > setpriority(PRIO_PROCESS, 0)
> > to have the same affect (because: which == PRIO_PRCESS, who == 0
> > conventionally means "the calling process").
> >
> > But they do not: the latter call only changes the priority of the
> > calling thread.  Is this intended?
>
> No, it was not intended. I think this error is here:
>
> +               case PRIO_PROCESS:
> +                       if (who)
> +                               pid = find_vpid(who);
> +                       else {
> +                               pid = task_pid(current);
> +                               who = current->pid;
> +                       }
>
> I was confused. ->pid is TID, had to use ->tgid to get PID.

(Indeed as I read Chris Freiesen's mail I almost tripped up with the
same confusion.  The source code names of these fields can easily lead
to confusions like that.)

> The fix: replace
> who = current->pid;
> with
> who = current->tgid;
> There are two places where you need to do it.
>
> Updated patch is below.

Tested-by: Michael Kerrisk <mtk.manpages@gmail.com>

With your suggested change, things now work as I would expect.  Thanks!

Cheers,

Michael

> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> --
> vda
>
>
>
> diff --git a/include/linux/resource.h b/include/linux/resource.h
> index aaa423a..f292690 100644
> --- a/include/linux/resource.h
> +++ b/include/linux/resource.h
> @@ -51,6 +51,7 @@ struct rlimit {
>  #define        PRIO_PROCESS    0
>  #define        PRIO_PGRP       1
>  #define        PRIO_USER       2
> +#define        PRIO_THREAD     3
>
>  /*
>  * Limit the stack by to some sane default: root can always
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 038a7bc..d339c1a 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -142,9 +142,9 @@ asmlinkage long sys_setpriority(int which, int who,
> int niceval)
>        struct task_struct *g, *p;
>        struct user_struct *user;
>        int error = -EINVAL;
> -       struct pid *pgrp;
> +       struct pid *pgrp, *pid;
>
> -       if (which > PRIO_USER || which < PRIO_PROCESS)
> +       if (which > PRIO_THREAD || which < PRIO_PROCESS)
>                goto out;
>
>        /* normalize: avoid signed division (rounding problems) */
> @@ -156,7 +156,7 @@ asmlinkage long sys_setpriority(int which, int who,
> int niceval)
>
>        read_lock(&tasklist_lock);
>        switch (which) {
> -               case PRIO_PROCESS:
> +               case PRIO_THREAD:
>                        if (who)
>                                p = find_task_by_vpid(who);
>                        else
> @@ -164,6 +164,19 @@ asmlinkage long sys_setpriority(int which, int who,
> int niceval)
>                        if (p)
>                                error = set_one_prio(p, niceval, error);
>                        break;
> +               case PRIO_PROCESS:
> +                       if (who)
> +                               pid = find_vpid(who);
> +                       else {
> +                               pid = task_pid(current);
> +                               who = current->tgid;
> +                       }
> +                       do_each_pid_thread(pid, PIDTYPE_PID, p) {
> +                               if (who == p->pid || who == p->tgid) {
> +                                       error = set_one_prio(p, niceval, error);
> +                               }
> +                       } while_each_pid_thread(pid, PIDTYPE_PID, p);
> +                       break;
>                case PRIO_PGRP:
>                        if (who)
>                                pgrp = find_vpid(who);
> @@ -206,14 +219,14 @@ asmlinkage long sys_getpriority(int which, int
> who)
>        struct task_struct *g, *p;
>        struct user_struct *user;
>        long niceval, retval = -ESRCH;
> -       struct pid *pgrp;
> +       struct pid *pgrp, *pid;
>
> -       if (which > PRIO_USER || which < PRIO_PROCESS)
> +       if (which > PRIO_THREAD || which < PRIO_PROCESS)
>                return -EINVAL;
>
>        read_lock(&tasklist_lock);
>        switch (which) {
> -               case PRIO_PROCESS:
> +               case PRIO_THREAD:
>                        if (who)
>                                p = find_task_by_vpid(who);
>                        else
> @@ -224,6 +237,21 @@ asmlinkage long sys_getpriority(int which, int who)
>                                        retval = niceval;
>                        }
>                        break;
> +               case PRIO_PROCESS:
> +                       if (who)
> +                               pid = find_vpid(who);
> +                       else {
> +                               pid = task_pid(current);
> +                               who = current->tgid;
> +                       }
> +                       do_each_pid_thread(pid, PIDTYPE_PID, p) {
> +                               if (who == p->pid || who == p->tgid) {
> +                                       niceval = 20 - task_nice(p);
> +                                       if (niceval > retval)
> +                                               retval = niceval;
> +                               }
> +                       } while_each_pid_thread(pid, PIDTYPE_PID, p);
> +                       break;
>                case PRIO_PGRP:
>                        if (who)
>                                pgrp = find_vpid(who);
>
>
>
>


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [PATCH] make setpriority POSIX compliant; introduce PRIO_THREAD extension

[Christoph Hellwig]

On Mon, Sep 01, 2008 at 04:12:35PM +0200, Denys Vlasenko wrote:
> Attached patch changes setpriority(PRIO_PROCESS) to set priority
> for all threads with selected pid. getpriority is changed accordingly,
> to return the (numerical) max of all threads' priority.
> 
> In order to allow priority of individual threads to be manipulated,
> patch adds PRIO_THREAD which acts on single thread, always.

Bad idea, you silently change the existing interface, and programs that
used to work around the old Linux behvaiour silently break.  Just keep
PRIO_PROCESS as it was and add a new PRIO_TGROUP that does the Posix
functionality for the whole thread group.  Glibc can then implement
the library-PRIO_PROCESS as PRIO_TGROUP for newly linked applications
without breaking existing ones.