[PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Stanislaw Gruszka]

tasklist_lock is not needed to protect find_task_by_vpid() nor
thread_group_leader() nor same_thread_group() , use rcu_read_lock() instead.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/posix-cpu-timers.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index bece7c0..1d9cdf3 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
 	if (pid == 0)
 		return 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	p = find_task_by_vpid(pid);
 	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
 		   same_thread_group(p, current) : thread_group_leader(p))) {
 		error = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_lock();
 
 	return error;
 }
@@ -398,7 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	new_timer->it.cpu.incr.sched = 0;
 	new_timer->it.cpu.expires.sched = 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
 		if (pid == 0) {
 			p = current;
@@ -422,7 +422,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	} else {
 		ret = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return ret;
 }
-- 
1.6.0.6

Re: [PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Peter Zijlstra]

On Fri, 2009-06-12 at 08:49 +0200, Stanislaw Gruszka wrote:
> tasklist_lock is not needed to protect find_task_by_vpid() nor
> thread_group_leader() nor same_thread_group() , use rcu_read_lock() instead.

Aside from not being strictly needed, one can sometimes require a
tasklist_lock reader to ensure nothing changes, however since the code
is racy in that regard anyway, this looks good.

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  kernel/posix-cpu-timers.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index bece7c0..1d9cdf3 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
>  	if (pid == 0)
>  		return 0;
>  
> -	read_lock(&tasklist_lock);
> +	rcu_read_lock();
>  	p = find_task_by_vpid(pid);
>  	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
>  		   same_thread_group(p, current) : thread_group_leader(p))) {
>  		error = -EINVAL;
>  	}
> -	read_unlock(&tasklist_lock);
> +	rcu_read_lock();
>  
>  	return error;
>  }
> @@ -398,7 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
>  	new_timer->it.cpu.incr.sched = 0;
>  	new_timer->it.cpu.expires.sched = 0;
>  
> -	read_lock(&tasklist_lock);
> +	rcu_read_lock();
>  	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
>  		if (pid == 0) {
>  			p = current;
> @@ -422,7 +422,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
>  	} else {
>  		ret = -EINVAL;
>  	}
> -	read_unlock(&tasklist_lock);
> +	rcu_read_unlock();
>  
>  	return ret;
>  }

Re: [PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Richard Kennedy]

Stanislaw Gruszka wrote:
> tasklist_lock is not needed to protect find_task_by_vpid() nor
> thread_group_leader() nor same_thread_group() , use rcu_read_lock() instead.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  kernel/posix-cpu-timers.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index bece7c0..1d9cdf3 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
>  	if (pid == 0)
>  		return 0;
>  
> -	read_lock(&tasklist_lock);
> +	rcu_read_lock();
>  	p = find_task_by_vpid(pid);
>  	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
>  		   same_thread_group(p, current) : thread_group_leader(p))) {
>  		error = -EINVAL;
>  	}
> -	read_unlock(&tasklist_lock);
> +	rcu_read_lock();
>  
Did you really intend to replace an unlock with a lock?

regards
Richard

Re: [PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Stanislaw Gruszka]

On Fri, 12 Jun 2009 08:48:24 +0100
Richard Kennedy <richard@rsk.demon.co.uk> wrote:

> > -	read_unlock(&tasklist_lock);
> > +	rcu_read_lock();
> >  
> Did you really intend to replace an unlock with a lock?

Intention was quite different. 

Thanks
Stanislaw 

[PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Stanislaw Gruszka]

tasklist_lock is not needed to protect find_task_by_vpid() nor
thread_group_leader() nor same_thread_group() , use rcu_read_lock() instead.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/posix-cpu-timers.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index bece7c0..1fadd2f 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
 	if (pid == 0)
 		return 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	p = find_task_by_vpid(pid);
 	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
 		   same_thread_group(p, current) : thread_group_leader(p))) {
 		error = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return error;
 }
@@ -398,7 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	new_timer->it.cpu.incr.sched = 0;
 	new_timer->it.cpu.expires.sched = 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
 		if (pid == 0) {
 			p = current;
@@ -422,7 +422,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	} else {
 		ret = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return ret;
 }
-- 
1.5.5.6

Re: [PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Oleg Nesterov]

On 06/12, Stanislaw Gruszka wrote:
>
> tasklist_lock is not needed to protect find_task_by_vpid() nor
> thread_group_leader()

It does protect thread_group_leader(), unless we use current.
Please see below.

> @@ -398,7 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
>  	new_timer->it.cpu.incr.sched = 0;
>  	new_timer->it.cpu.expires.sched = 0;
>
> -	read_lock(&tasklist_lock);
> +	rcu_read_lock();
>  	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
>  		if (pid == 0) {
>  			p = current;
> @@ -422,7 +422,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
>  	} else {
>  		ret = -EINVAL;
>  	}
> -	read_unlock(&tasklist_lock);
> +	rcu_read_unlock();

Suppose that the non-main thread execs. de_thread() does

	attach_pid(tsk, PIDTYPE_PID, task_pid(leader));
	leader->group_leader = tsk;

under write_lock(tasklist). This means posix_cpu_timer_create() can return
-EINVAL if it is called in between, or if we found the group leader but
thread_group_leader() is called after "tsk->group_leader = tsk".

I think the patch is fine, but you should also replace thread_group_leader()
with has_group_leader_pid().



This reminds me. !CPUCLOCK_PERTHREAD timers survive after exec, but only
if ->group_leader execs. Not good.

Oleg.

Re: [PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Oleg Nesterov]

On 06/12, Oleg Nesterov wrote:
>
> This reminds me. !CPUCLOCK_PERTHREAD timers survive after exec, but only
> if ->group_leader execs. Not good.

Perhaps cpu_timer_list should use "struct pid *" instead of
"struct task_struct *", but this requires a lot of changes.

Oleg.

[PATCH 2/2] posix-cpu-timers: remove tasklist_lock where we can

[Stanislaw Gruszka]

tasklist_lock is not needed to protect find_task_by_vpid() nor
same_thread_group() and if we replace thread_group_leader() by
has_group_leader_pid() we can just use RCU lock to protect some process
timers/clocks checks.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/posix-cpu-timers.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index bece7c0..029b442 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
 	if (pid == 0)
 		return 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	p = find_task_by_vpid(pid);
 	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : thread_group_leader(p))) {
+		   same_thread_group(p, current) : has_group_leader_pid(p))) {
 		error = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return error;
 }
@@ -398,7 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	new_timer->it.cpu.incr.sched = 0;
 	new_timer->it.cpu.expires.sched = 0;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
 		if (pid == 0) {
 			p = current;
@@ -412,7 +412,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 			p = current->group_leader;
 		} else {
 			p = find_task_by_vpid(pid);
-			if (p && !thread_group_leader(p))
+			if (p && !has_group_leader_pid(p))
 				p = NULL;
 		}
 	}
@@ -422,7 +422,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	} else {
 		ret = -EINVAL;
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return ret;
 }
-- 
1.5.5.6