All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pids: introduce find_get_task_by_vpid helper
@ 2017-10-26 13:07 ` Mike Rapoport
  0 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2017-10-26 13:07 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Darren Hart,
	Oleg Nesterov, Balbir Singh, linux-mm, lkml, Mike Rapoport

There are several functions that do find_task_by_vpid() followed by
get_task_struct(). We can use a helper function instead.

Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
---
 include/linux/sched.h  |  5 +++++
 kernel/futex.c         |  7 +------
 kernel/pid.c           | 13 +++++++++++++
 kernel/ptrace.c        |  6 +-----
 kernel/taskstats.c     |  6 +-----
 mm/process_vm_access.c |  6 +-----
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 26a7df4e558c..4c3af5255fcf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1484,6 +1484,11 @@ static inline struct thread_info *task_thread_info(struct task_struct *task)
 extern struct task_struct *find_task_by_vpid(pid_t nr);
 extern struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns);
 
+/*
+ * find a task by its virtual pid and get the task struct
+ */
+extern struct task_struct *find_get_task_by_vpid(pid_t nr);
+
 extern int wake_up_state(struct task_struct *tsk, unsigned int state);
 extern int wake_up_process(struct task_struct *tsk);
 extern void wake_up_new_task(struct task_struct *tsk);
diff --git a/kernel/futex.c b/kernel/futex.c
index 0518a0bfc746..6446aa9f2288 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
 {
 	struct task_struct *p;
 
-	rcu_read_lock();
-	p = find_task_by_vpid(pid);
-	if (p)
-		get_task_struct(p);
-
-	rcu_read_unlock();
+	p = find_get_task_by_vpid(pid);
 
 	return p;
 }
diff --git a/kernel/pid.c b/kernel/pid.c
index 020dedbdf066..ead086b0ef8e 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -462,6 +462,19 @@ struct task_struct *find_task_by_vpid(pid_t vnr)
 	return find_task_by_pid_ns(vnr, task_active_pid_ns(current));
 }
 
+struct task_struct *find_get_task_by_vpid(pid_t nr)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = find_task_by_vpid(nr);
+	if (task)
+		get_task_struct(task);
+	rcu_read_unlock();
+
+	return task;
+}
+
 struct pid *get_task_pid(struct task_struct *task, enum pid_type type)
 {
 	struct pid *pid;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 84b1367935e4..91efc97674ce 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
 {
 	struct task_struct *child;
 
-	rcu_read_lock();
-	child = find_task_by_vpid(pid);
-	if (child)
-		get_task_struct(child);
-	rcu_read_unlock();
+	child = find_get_task_by_vpid(pid);
 
 	if (!child)
 		return ERR_PTR(-ESRCH);
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 4559e914452b..4e62a4a8fa91 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -194,11 +194,7 @@ static int fill_stats_for_pid(pid_t pid, struct taskstats *stats)
 {
 	struct task_struct *tsk;
 
-	rcu_read_lock();
-	tsk = find_task_by_vpid(pid);
-	if (tsk)
-		get_task_struct(tsk);
-	rcu_read_unlock();
+	tsk = find_get_task_by_vpid(pid);
 	if (!tsk)
 		return -ESRCH;
 	fill_stats(current_user_ns(), task_active_pid_ns(current), tsk, stats);
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index 8973cd231ece..16424b9ae424 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -197,11 +197,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
 	}
 
 	/* Get process information */
-	rcu_read_lock();
-	task = find_task_by_vpid(pid);
-	if (task)
-		get_task_struct(task);
-	rcu_read_unlock();
+	task = find_get_task_by_vpid(pid);
 	if (!task) {
 		rc = -ESRCH;
 		goto free_proc_pages;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] pids: introduce find_get_task_by_vpid helper
@ 2017-10-26 13:07 ` Mike Rapoport
  0 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2017-10-26 13:07 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Darren Hart,
	Oleg Nesterov, Balbir Singh, linux-mm, lkml, Mike Rapoport

There are several functions that do find_task_by_vpid() followed by
get_task_struct(). We can use a helper function instead.

Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
---
 include/linux/sched.h  |  5 +++++
 kernel/futex.c         |  7 +------
 kernel/pid.c           | 13 +++++++++++++
 kernel/ptrace.c        |  6 +-----
 kernel/taskstats.c     |  6 +-----
 mm/process_vm_access.c |  6 +-----
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 26a7df4e558c..4c3af5255fcf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1484,6 +1484,11 @@ static inline struct thread_info *task_thread_info(struct task_struct *task)
 extern struct task_struct *find_task_by_vpid(pid_t nr);
 extern struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns);
 
+/*
+ * find a task by its virtual pid and get the task struct
+ */
+extern struct task_struct *find_get_task_by_vpid(pid_t nr);
+
 extern int wake_up_state(struct task_struct *tsk, unsigned int state);
 extern int wake_up_process(struct task_struct *tsk);
 extern void wake_up_new_task(struct task_struct *tsk);
diff --git a/kernel/futex.c b/kernel/futex.c
index 0518a0bfc746..6446aa9f2288 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
 {
 	struct task_struct *p;
 
-	rcu_read_lock();
-	p = find_task_by_vpid(pid);
-	if (p)
-		get_task_struct(p);
-
-	rcu_read_unlock();
+	p = find_get_task_by_vpid(pid);
 
 	return p;
 }
diff --git a/kernel/pid.c b/kernel/pid.c
index 020dedbdf066..ead086b0ef8e 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -462,6 +462,19 @@ struct task_struct *find_task_by_vpid(pid_t vnr)
 	return find_task_by_pid_ns(vnr, task_active_pid_ns(current));
 }
 
+struct task_struct *find_get_task_by_vpid(pid_t nr)
+{
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = find_task_by_vpid(nr);
+	if (task)
+		get_task_struct(task);
+	rcu_read_unlock();
+
+	return task;
+}
+
 struct pid *get_task_pid(struct task_struct *task, enum pid_type type)
 {
 	struct pid *pid;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 84b1367935e4..91efc97674ce 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
 {
 	struct task_struct *child;
 
-	rcu_read_lock();
-	child = find_task_by_vpid(pid);
-	if (child)
-		get_task_struct(child);
-	rcu_read_unlock();
+	child = find_get_task_by_vpid(pid);
 
 	if (!child)
 		return ERR_PTR(-ESRCH);
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 4559e914452b..4e62a4a8fa91 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -194,11 +194,7 @@ static int fill_stats_for_pid(pid_t pid, struct taskstats *stats)
 {
 	struct task_struct *tsk;
 
-	rcu_read_lock();
-	tsk = find_task_by_vpid(pid);
-	if (tsk)
-		get_task_struct(tsk);
-	rcu_read_unlock();
+	tsk = find_get_task_by_vpid(pid);
 	if (!tsk)
 		return -ESRCH;
 	fill_stats(current_user_ns(), task_active_pid_ns(current), tsk, stats);
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index 8973cd231ece..16424b9ae424 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -197,11 +197,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
 	}
 
 	/* Get process information */
-	rcu_read_lock();
-	task = find_task_by_vpid(pid);
-	if (task)
-		get_task_struct(task);
-	rcu_read_unlock();
+	task = find_get_task_by_vpid(pid);
 	if (!task) {
 		rc = -ESRCH;
 		goto free_proc_pages;
-- 
2.7.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] pids: introduce find_get_task_by_vpid helper
  2017-10-26 13:07 ` Mike Rapoport
@ 2017-10-26 13:58   ` Oleg Nesterov
  -1 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2017-10-26 13:58 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Andrew Morton, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Darren Hart, Balbir Singh, linux-mm, lkml

On 10/26, Mike Rapoport wrote:
>
> There are several functions that do find_task_by_vpid() followed by
> get_task_struct(). We can use a helper function instead.

Yes, agreed, I was going to do this many times.

> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
>  {
>  	struct task_struct *p;
>  
> -	rcu_read_lock();
> -	p = find_task_by_vpid(pid);
> -	if (p)
> -		get_task_struct(p);
> -
> -	rcu_read_unlock();
> +	p = find_get_task_by_vpid(pid);
>  
>  	return p;

OK, but then I think you should remove futex_find_get_task() and convert
it callers to use the new helper.

> @@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
>  {
>  	struct task_struct *child;
>  
> -	rcu_read_lock();
> -	child = find_task_by_vpid(pid);
> -	if (child)
> -		get_task_struct(child);
> -	rcu_read_unlock();
> +	child = find_get_task_by_vpid(pid);
>  
>  	if (!child)
>  		return ERR_PTR(-ESRCH);

The same. ptrace_get_task_struct() should die imo.

Oleg.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] pids: introduce find_get_task_by_vpid helper
@ 2017-10-26 13:58   ` Oleg Nesterov
  0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2017-10-26 13:58 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Andrew Morton, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Darren Hart, Balbir Singh, linux-mm, lkml

On 10/26, Mike Rapoport wrote:
>
> There are several functions that do find_task_by_vpid() followed by
> get_task_struct(). We can use a helper function instead.

Yes, agreed, I was going to do this many times.

> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
>  {
>  	struct task_struct *p;
>  
> -	rcu_read_lock();
> -	p = find_task_by_vpid(pid);
> -	if (p)
> -		get_task_struct(p);
> -
> -	rcu_read_unlock();
> +	p = find_get_task_by_vpid(pid);
>  
>  	return p;

OK, but then I think you should remove futex_find_get_task() and convert
it callers to use the new helper.

> @@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
>  {
>  	struct task_struct *child;
>  
> -	rcu_read_lock();
> -	child = find_task_by_vpid(pid);
> -	if (child)
> -		get_task_struct(child);
> -	rcu_read_unlock();
> +	child = find_get_task_by_vpid(pid);
>  
>  	if (!child)
>  		return ERR_PTR(-ESRCH);

The same. ptrace_get_task_struct() should die imo.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] pids: introduce find_get_task_by_vpid helper
  2017-10-26 13:58   ` Oleg Nesterov
@ 2017-10-26 19:54     ` Mike Rapoport
  -1 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2017-10-26 19:54 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Darren Hart, Balbir Singh, linux-mm, lkml

On Thu, Oct 26, 2017 at 03:58:25PM +0200, Oleg Nesterov wrote:
> On 10/26, Mike Rapoport wrote:
> >
> > There are several functions that do find_task_by_vpid() followed by
> > get_task_struct(). We can use a helper function instead.
> 
> Yes, agreed, I was going to do this many times.
> 
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
> >  {
> >  	struct task_struct *p;
> >  
> > -	rcu_read_lock();
> > -	p = find_task_by_vpid(pid);
> > -	if (p)
> > -		get_task_struct(p);
> > -
> > -	rcu_read_unlock();
> > +	p = find_get_task_by_vpid(pid);
> >  
> >  	return p;
> 
> OK, but then I think you should remove futex_find_get_task() and convert
> it callers to use the new helper.

Agree. My cocci script was too simple :)
 
> > @@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
> >  {
> >  	struct task_struct *child;
> >  
> > -	rcu_read_lock();
> > -	child = find_task_by_vpid(pid);
> > -	if (child)
> > -		get_task_struct(child);
> > -	rcu_read_unlock();
> > +	child = find_get_task_by_vpid(pid);
> >  
> >  	if (!child)
> >  		return ERR_PTR(-ESRCH);
> 
> The same. ptrace_get_task_struct() should die imo.

Yeah, we could just return -ESRCH in ptrace_get_task_struct users instead
of all the ERR_PTR and PTR_ERR conversions...

> Oleg.
> 

-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] pids: introduce find_get_task_by_vpid helper
@ 2017-10-26 19:54     ` Mike Rapoport
  0 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2017-10-26 19:54 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Darren Hart, Balbir Singh, linux-mm, lkml

On Thu, Oct 26, 2017 at 03:58:25PM +0200, Oleg Nesterov wrote:
> On 10/26, Mike Rapoport wrote:
> >
> > There are several functions that do find_task_by_vpid() followed by
> > get_task_struct(). We can use a helper function instead.
> 
> Yes, agreed, I was going to do this many times.
> 
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -870,12 +870,7 @@ static struct task_struct *futex_find_get_task(pid_t pid)
> >  {
> >  	struct task_struct *p;
> >  
> > -	rcu_read_lock();
> > -	p = find_task_by_vpid(pid);
> > -	if (p)
> > -		get_task_struct(p);
> > -
> > -	rcu_read_unlock();
> > +	p = find_get_task_by_vpid(pid);
> >  
> >  	return p;
> 
> OK, but then I think you should remove futex_find_get_task() and convert
> it callers to use the new helper.

Agree. My cocci script was too simple :)
 
> > @@ -1103,11 +1103,7 @@ static struct task_struct *ptrace_get_task_struct(pid_t pid)
> >  {
> >  	struct task_struct *child;
> >  
> > -	rcu_read_lock();
> > -	child = find_task_by_vpid(pid);
> > -	if (child)
> > -		get_task_struct(child);
> > -	rcu_read_unlock();
> > +	child = find_get_task_by_vpid(pid);
> >  
> >  	if (!child)
> >  		return ERR_PTR(-ESRCH);
> 
> The same. ptrace_get_task_struct() should die imo.

Yeah, we could just return -ESRCH in ptrace_get_task_struct users instead
of all the ERR_PTR and PTR_ERR conversions...

> Oleg.
> 

-- 
Sincerely yours,
Mike.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-26 19:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-26 13:07 [PATCH] pids: introduce find_get_task_by_vpid helper Mike Rapoport
2017-10-26 13:07 ` Mike Rapoport
2017-10-26 13:58 ` Oleg Nesterov
2017-10-26 13:58   ` Oleg Nesterov
2017-10-26 19:54   ` Mike Rapoport
2017-10-26 19:54     ` Mike Rapoport

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.