core: Convert thread iteration to use for_each[_process]_thread APIs, 1st pile
Subsystems (6)
cover core: Convert thread iteration to use for_each[_process]_thread APIs, 1st pile
Hi,
This is the first pile of a longer series that convert deprecated and RCU-unsafe thread group iterators (do_each_thread, while_each_thread) to use for_each_thread/for_each_process_thread RCU-safe iterators that have been introduced by Oleg.
for_each[_process]_thread() is already upstream so these patches don't depend on any pending preparatory work.
So ideally it would be nice if maintainers cherry-pick the patches corresponding to their own subsystem.
Thanks,
Frederic
---
Frederic Weisbecker (5):
sched: Convert thread_group_cputime() to use for_each_thread()
tracepoint: Convert process iteration to use for_each_process_thread()
hung_task: Convert process iteration to use for_each_process_thread()
procfs: Convert process iteration to use for_each_thread()
sched: Convert tasks iteration to use for_each_process_thread()
fs/proc/array.c | 7 ++++--- fs/proc/base.c | 4 ++-- kernel/hung_task.c | 8 ++++---- kernel/sched/core.c | 13 ++++++------- kernel/sched/cputime.c | 13 ++++++------- kernel/tracepoint.c | 12 ++++++------ 6 files changed, 28 insertions(+), 29 deletions(-)
PATCH 1/5 sched: Convert thread_group_cputime() to use for_each_thread()
do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the cputime code accounting. The ultimate goal is to remove the struct task_struct::thread_group field and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.
It also makes thread_group_cputime() eventually RCU-safe.
--- kernel/sched/cputime.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/sched/cputime.c +6 -7
@@ -281,11 +281,11 @@ static __always_inline bool steal_account_process_tick(void)/* * Accumulate raw cputime values of dead tasks (sig->[us]time) and live -* tasks (sum on group iteration) belonging to @tsk's group. +* tasks (sum on group iteration) belonging to @p's group. */ -void thread_group_cputime (struct task_struct *tsk,struct task_cputime *times) +void thread_group_cputime (struct task_struct *p,struct task_cputime *times) { -struct signal_struct *sig = tsk->signal; +struct signal_struct *sig = p->signal;cputime_t utime, stime;struct task_struct *t;
@@ -295,16 +295,15 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)rcu_read_lock ();/* make sure we can trust tsk->thread_group list */ -if (!likely (pid_alive (tsk))) +if (!likely (pid_alive (p))) goto out; - t = tsk; - do { +for_each_thread (p, t) {task_cputime (t, &utime, &stime); times->utime += utime; times->stime += stime; times->sum_exec_runtime +=task_sched_runtime (t); - } while_each_thread(tsk, t); + } out:rcu_read_unlock (); }
Signature · 1 line
1.8.3.1
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
kernel/sched/cputime.c void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) · line 298
Quoted 7 lines · Frederic Weisbecker
kernel/sched/cputime.c +1
-1@@ -295,16 +295,15 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)rcu_read_lock ();/* make sure we can trust tsk->thread_group list */ -if (!likely (pid_alive (tsk))) +if (!likely (pid_alive (p)))
You can simply remove this check now. for_each_thread() is always safe.
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
In reply to Frederic WeisbeckerQuoted 9 lines · Frederic Weisbecker
do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the cputime code accounting. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.It also makes
thread_group_cputime()eventually RCU-safe.
this fails to explain how the current code is broken.
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Quoted 8 lines
Quoted 6 lines
Lets convert the callers in the cputime code accounting. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.It also makes
thread_group_cputime()eventually RCU-safe.this fails to explain how the current code is broken.
while_each_thread(g, t) will loop forever if g exits and removes itself from ->thread_group. This can happen even if it is the group leader, de_thread() can do this.
Another problem is that it is used wrongly very often, even if while_each_thread() was fine people forget to check pid_alive() to ensure it didn't exit even before we take rcu_read_lock().
for_each_thread(p, t) is always safe. Unless p's task_struct can't go away, of course.
But there is a difference. Ignoring the bug above
p = g;
do {
printk("pid=%d\n", p->pid);
} while_each_thread(g, p);always prints at least one pid.
for_each_thread(g, p)
printk("pid=%d\n", p->pid);can print nothing if g has already exited.
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 33 lines · Oleg Nesterov
Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Quoted 8 lines
Quoted 6 lines
Lets convert the callers in the cputime code accounting. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.It also makes
thread_group_cputime()eventually RCU-safe.this fails to explain how the current code is broken.
while_each_thread(g, t)will loop forever if g exits and removes itself from ->thread_group. This can happen even if it is the group leader,de_thread()can do this.Another problem is that it is used wrongly very often, even if
while_each_thread()was fine people forget to checkpid_alive()to ensure it didn't exit even before we takercu_read_lock().
for_each_thread(p, t)is always safe. Unless p'stask_structcan't go away, of course.But there is a difference. Ignoring the bug above
p = g; do { printk("pid=%d\n", p->pid); } while_each_thread(g, p);
Quoted 6 lines
always prints at least one pid.
for_each_thread(g, p) printk("pid=%d\n", p->pid);can print nothing if g has already exited.
Cute,.. and all that lives in sched.h and I wasn't even Cc'ed :-(
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 2 lines · Peter Zijlstra
Cute,.. and all that lives in
sched.hand I wasn't even Cc'ed :-(
Sorry ;) I didn't think you can be interested. I'll cc you in future, this still needs more work.
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 6 lines · Oleg Nesterov
Quoted 2 lines · Peter Zijlstra
Cute,.. and all that lives in
sched.hand I wasn't even Cc'ed :-(Sorry ;) I didn't think you can be interested. I'll cc you in future, this still needs more work.
In general anything locking and/or task related, and this is both, is 'interesting'.
Anyway, I'll look over the stuff tomorrow.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 1 line · Peter Zijlstra
Anyway, I'll look over the stuff tomorrow.
That patch was missing useful clues..
Also, I shot __for_each_thread in the head, it seemed useless.
--- include/linux/sched.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
include/linux/sched.h +6 -5
@@ -2377,6 +2377,9 @@ extern bool current_is_single_threaded(void);/* * Careful: do_each_thread/while_each_thread is a double loop so * 'break' will not work as expected - use goto instead. +* +* Deprecated; use for_each_thread() instead; this has serious issues with +* g != current and RCU. */ #define do_each_thread (g, t) \ for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do
@@ -2384,11 +2387,8 @@ extern bool current_is_single_threaded(void);#define while_each_thread (g, t) \ while ((t = next_thread(t)) != g) -#define __for_each_thread (signal, t) \ - list_for_each_entry_rcu(t, &(signal)->thread_head, thread_node) -#define for_each_thread (p, t) \ - __for_each_thread((p)->signal, t) + list_for_each_entry_rcu(t, &(p)->signal->thread_head, thread_node)/* Careful: this is a double loop, 'break' won't work as expected. */ #define for_each_process_thread (p, t) \
@@ -2421,7 +2421,8 @@ bool same_thread_group(struct task_struct *p1, struct task_struct *p2)return p1->signal == p2->signal; } -static inline struct task_struct *next_thread (const struct task_struct *p) +static inline __deprecated +struct task_struct *next_thread (const struct task_struct *p) {return list_entry_rcu (p->thread_group.next, struct task_struct, thread_group);
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
include/linux/sched.h extern bool current_is_single_threaded(void); · line 2383
Quoted 12 lines · Peter Zijlstra
On Wed, Apr 09, 2014 at 09:49:57PM +0200, Peter Zijlstra wrote:
include/linux/sched.h +3
-0@@ -2377,6 +2377,9 @@ extern bool current_is_single_threaded(void);/* * Careful: do_each_thread/while_each_thread is a double loop so * 'break' will not work as expected - use goto instead. +* +* Deprecated; use for_each_thread() instead; this has serious issues with +* g != current and RCU. */
Yes, thanks.
include/linux/sched.h extern bool current_is_single_threaded(void); · line 2391
Quoted 12 lines
include/linux/sched.h +1
-4#define do_each_thread (g, t) \ for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do@@ -2384,11 +2387,8 @@ extern bool current_is_single_threaded(void);#define while_each_thread (g, t) \ while ((t = next_thread(t)) != g) -#define __for_each_thread (signal, t) \ - list_for_each_entry_rcu(t, &(signal)->thread_head, thread_node) -#define for_each_thread (p, t) \ - __for_each_thread((p)->signal, t) + list_for_each_entry_rcu(t, &(p)->signal->thread_head, thread_node)
Why? __for_each_thread(signal) can generate a better code, if we do care.
In fact, ignoring the bad "signal" name, __for_each_thread(signal, t) even looks better. "signal" represents the whole thread group.
But I won't argue. Besides, this reminds me about CONST_CAST() and making task_struct->signal "const". This can improve the code generation too.
include/linux/sched.h bool same_thread_group(struct task_struct *p1, struct task_struct *p2) · line 2426
Quoted 3 lines
include/linux/sched.h +2
-0+static inline __deprecated +struct task_struct *next_thread (const struct task_struct *p) {
Not sure... But probably fine too.
I already killed some users of next_thread(). This reminds me about next_tid(), probably it should be converted too.
As for, say, __exit_signal() it really needs next_thread(). We can fix it instead of deprecating, or we can add another one with another name.
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 6 lines · Oleg Nesterov
Quoted 6 lines
#define
do_each_thread(g, t)\include/linux/sched.h +0
-0for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do@@ -2384,11 +2387,8 @@ extern bool current_is_single_threaded(void);#define while_each_thread (g, t) \ while ((t = next_thread(t)) != g)
Quoted 8 lines
include/linux/sched.h extern bool current_is_single_threaded(void); · line 2391
Quoted 6 lines
include/linux/sched.h +1
-4-#define __for_each_thread (signal, t) \ - list_for_each_entry_rcu(t, &(signal)->thread_head, thread_node) -#define for_each_thread (p, t) \ - __for_each_thread((p)->signal, t) + list_for_each_entry_rcu(t, &(p)->signal->thread_head, thread_node)Why?
__for_each_thread(signal)can generate a better code, if we do care.
Well, there were no users and it wasn't mentioned anywhere.
Quoted 5 lines
In fact, ignoring the bad "signal" name,
__for_each_thread(signal, t)even looks better. "signal" represents the whole thread group.But I won't argue. Besides, this reminds me about
CONST_CAST()and makingtask_struct->signal"const". This can improve the code generation too.
Yeah, I always disliked how we mixed up the signal handling and the thread group stuff. We should probably rename the lot.
Quoted 11 lines
include/linux/sched.h bool same_thread_group(struct task_struct *p1, struct task_struct *p2) · line 2426
Quoted 3 lines
include/linux/sched.h +2
-0+static inline __deprecated +struct task_struct *next_thread (const struct task_struct *p) {Not sure... But probably fine too.
I already killed some users of
next_thread(). This reminds me aboutnext_tid(), probably it should be converted too.As for, say,
__exit_signal()it really needsnext_thread(). We can fix it instead of deprecating, or we can add another one with another name.
Well, your Changelog said that next_thread() was faulty too; if __exit_signal() is the only site where it is correct we can open-code it there. If there's more we should probably create a new function and audit all current sites.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 5 lines · Peter Zijlstra
Quoted 5 lines
include/linux/sched.h bool same_thread_group(struct task_struct *p1, struct task_struct *p2) · line 2426
Quoted 3 lines
include/linux/sched.h +2
-0+static inline __deprecated +struct task_struct *next_thread (const struct task_struct *p) {Not sure... But probably fine too.
Also, since its used in while_each_thread() each usage of that will also generate an appropriate warn.
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
In reply to Peter ZijlstraQuoted 9 lines · Peter Zijlstra
Quoted 6 lines · Oleg Nesterov
include/linux/sched.h bool same_thread_group(struct task_struct *p1, struct task_struct *p2) · line 2426
Quoted 3 lines
include/linux/sched.h +2
-0+static inline __deprecated +struct task_struct *next_thread (const struct task_struct *p) {Not sure... But probably fine too.
Quoted 7 lines
Quoted 5 lines
I already killed some users of
next_thread(). This reminds me aboutnext_tid(), probably it should be converted too.As for, say,
__exit_signal()it really needsnext_thread(). We can fix it instead of deprecating, or we can add another one with another name.Well, your Changelog said that
next_thread()was faulty too;
If lockless. Sure.
Quoted 4 lines
if
__exit_signal()is the only site where it is correct we can open-code it there. If there's more we should probably create a new function and audit all current sites.
So far I can see only 2 users, __exit_signal() and complete_signal().
May be next_tid(), but in fact it needs next_thread_or_zero(). And this connects to check_hung_task(). What they actually (OK, perhaps) need is for_each_thread_continue, like list_for_each_entry_continue_rcu.
We will see. I didn't finish this all because I was distracted (and lazy of course ;).
My plan (== TODO) was:
- check all current users, carefully convert those who need a special attention (because the semantics differs a bit)
- the same for thread_group_empty()
- reimplement next_thread/while_each_thread via ->thread_head
- kill task_struct->thread_group
- all other users can be converted later
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
Quoted 1 line · Oleg Nesterov
So far I can see only 2 users,
__exit_signal()andcomplete_signal().
If that's it we could just open-code them and be done.
Quoted 3 lines
May be
next_tid(), but in fact it needsnext_thread_or_zero(). And this connects tocheck_hung_task(). What they actually (OK, perhaps) need isfor_each_thread_continue, likelist_for_each_entry_continue_rcu.
That could work I suppose; brain has left the building already..
Quoted 2 lines
We will see. I didn't finish this all because I was distracted (and lazy of course ;).
:-)
Peter Zijlstra
maintainerRe: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
In reply to Peter ZijlstraQuoted 6 lines · Peter Zijlstra
Quoted 1 line · Peter Zijlstra
Anyway, I'll look over the stuff tomorrow.
That patch was missing useful clues..
Also, I shot
__for_each_threadin the head, it seemed useless.
Hmm, do we really want to have for_each_process_thread() ? Having people explicitly write:
for_each_process(p) {
for_each_thread(p, t) {
}}
isn't that much more effort and its a whole lot more explicit on the double loop thing.
Re: [PATCH 1/5] sched: Convert thread_group_cputime() to use for_each_thread()
In reply to Oleg NesterovQuoted 6 lines · Oleg Nesterov
Quoted 2 lines · Peter Zijlstra
Cute,.. and all that lives in
sched.hand I wasn't even Cc'ed :-(Sorry ;) I didn't think you can be interested. I'll cc you in future, this still needs more work.
Please generally Cc: the folks listed in:
scripts/get_maintainer.pl -f include/linux/sched.h
If maintainers are truly not interested in changes to a file they are listed to maintain then they'll get it unlisted in the MAINTAINERS patterns.
Thanks,
Ingo
PATCH 2/5 tracepoint: Convert process iteration to use for_each_process_thread()
In reply to Frederic Weisbeckerdo_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the tracepoint code. The ultimate goal is to remove the struct task_struct::thread_group field and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.
--- kernel/tracepoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/tracepoint.c +6 -6
@@ -561,15 +561,15 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {/* Skip kernel threads. */ if (t->mm)set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); + }read_unlock_irqrestore (&tasklist_lock, flags); } sys_tracepoint_refcount++;
@@ -578,14 +578,14 @@ void syscall_regfunc(void)void syscall_unregfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t; sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); + }read_unlock_irqrestore (&tasklist_lock, flags); } }
Signature · 1 line
1.8.3.1
Mathieu Desnoyers
reviewerRe: [PATCH 2/5] tracepoint: Convert process iteration to use for_each_process_thread()
----- Original Message -----
Quoted 31 lines
From: "Frederic Weisbecker" <[email protected]>
To: "LKML" <[email protected]>
Cc: "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Ingo Molnar" <[email protected]>, "Mathieu Desnoyers" <[email protected]>, "Oleg Nesterov" <[email protected]>, "Steven Rostedt" <[email protected]>
Sent: Wednesday, April 9, 2014 12:11:19 PM
Subject: [PATCH 2/5] tracepoint: Convert process iteration to usefor_each_process_thread()do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the tracepoint code. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.Cc Andrew Morton <[email protected]>Cc Ingo Molnar <[email protected]>Cc Mathieu Desnoyers <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Steven Rostedt <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>--- kernel/tracepoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)kernel/tracepoint.c +0
-0
kernel/tracepoint.c static int sys_tracepoint_refcount; · line 568
Quoted 10 lines
kernel/tracepoint.c +2
-2@@ -561,15 +561,15 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {
What are the locking rules for for_each_process_thread() ?
Is it required to hold RCU read-side lock ? (it's not the case here)
Is tasklist_lock read-side lock sufficient ?
A quick glance at those for_each iterator defines in sched.h was not helpful in finding this information.
Thanks,
Mathieu
Quoted 30 lines
kernel/tracepoint.c +4
-5/* Skip kernel threads. */ if (t->mm)set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); + }read_unlock_irqrestore (&tasklist_lock, flags); } sys_tracepoint_refcount++;@@ -578,14 +578,14 @@ void syscall_regfunc(void)void syscall_unregfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t; sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); + }read_unlock_irqrestore (&tasklist_lock, flags); } } --1.8.3.1
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH 2/5] tracepoint: Convert process iteration to use for_each_process_thread()
Quoted 45 lines · Mathieu Desnoyers
----- Original Message -----
Quoted 31 lines
From: "Frederic Weisbecker" <[email protected]>
To: "LKML" <[email protected]>
Cc: "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Ingo Molnar" <[email protected]>, "Mathieu Desnoyers" <[email protected]>, "Oleg Nesterov" <[email protected]>, "Steven Rostedt" <[email protected]>
Sent: Wednesday, April 9, 2014 12:11:19 PM
Subject: [PATCH 2/5] tracepoint: Convert process iteration to usefor_each_process_thread()do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the tracepoint code. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.Cc Andrew Morton <[email protected]>Cc Ingo Molnar <[email protected]>Cc Mathieu Desnoyers <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Steven Rostedt <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>--- kernel/tracepoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)kernel/tracepoint.c +0
-0kernel/tracepoint.c static int sys_tracepoint_refcount; · line 568
Quoted 10 lines
kernel/tracepoint.c +2
-2@@ -561,15 +561,15 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {What are the locking rules for
for_each_process_thread()?
Quoted 3 lines
Is it required to hold RCU read-side lock ? (it's not the case here)
Is
tasklist_lockread-side lock sufficient ?
It's the same requirements than do_each_thread while_each_thread: tasklist_lock or RCU. Except that it's really RCU-safe. while_each_thread isn't really safe due to issues with concurrent exec/de_thread()
Then it depends on your requirement, if you can tolerate concurrent adds and removals or not. Here tasklist_lock seems required or we may miss some tasks' syscall traces.
Quoted 3 lines
A quick glance at those
for_eachiterator defines insched.hwas not helpful in finding this information.
Agreed, I'm going to add comments to precise that.
Steven Rostedt
maintainerRe: [PATCH 2/5] tracepoint: Convert process iteration to use for_each_process_thread()
In reply to Mathieu DesnoyersQuoted 45 lines · On Wed, 9 Apr 2014 16:28:35 +0000 (UTC) Mathieu Desnoyers…
----- Original Message -----
Quoted 31 lines
From: "Frederic Weisbecker" <[email protected]>
To: "LKML" <[email protected]>
Cc: "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Ingo Molnar" <[email protected]>, "Mathieu Desnoyers" <[email protected]>, "Oleg Nesterov" <[email protected]>, "Steven Rostedt" <[email protected]>
Sent: Wednesday, April 9, 2014 12:11:19 PM
Subject: [PATCH 2/5] tracepoint: Convert process iteration to usefor_each_process_thread()do_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the tracepoint code. The ultimate goal is to remove the struct
task_struct::thread_groupfield and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.Cc Andrew Morton <[email protected]>Cc Ingo Molnar <[email protected]>Cc Mathieu Desnoyers <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Steven Rostedt <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>--- kernel/tracepoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)kernel/tracepoint.c +0
-0kernel/tracepoint.c static int sys_tracepoint_refcount; · line 568
Quoted 10 lines
kernel/tracepoint.c +2
-2@@ -561,15 +561,15 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) {unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t;if (!sys_tracepoint_refcount) {read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +for_each_process_thread (p, t) {What are the locking rules for
for_each_process_thread()?
Quoted 3 lines
Is it required to hold RCU read-side lock ? (it's not the case here)
Is
tasklist_lockread-side lock sufficient ?
The tasklist_lock is all that is needed. Nothing about the list will change when that's held. Ideally we would like to start converting things to rcu, where it's OK if the list actually changes while you are iterating.
But if you require the list to stay the same while you read all the tasks, then you need the bigger hammer (tasklist_lock).
Also, this is just about iterating the tasklist list. If something within the loop requires rcu, then that's a different story.
-- Steve
Quoted 4 lines
A quick glance at those
for_eachiterator defines insched.hwas not helpful in finding this information.
cover Was: Convert process iteration to use for_each_process_thread()
Quoted 2 lines · Steven Rostedt
The
tasklist_lockis all that is needed.
Yes, but not _irq. Plus this code is buggy ;)
Steven, I sent these patches twice (at least!). This time I didn't bother to recheck this code, but the patches still apply so perhaps you can take a look?
Last attempt, I promise ;)
Oleg.
PATCH RESEND 1/2 tracing: syscall_*regfunc() can race with copy_process()
syscall_regfunc() and syscall_unregfunc() should set/clear TIF_SYSCALL_TRACEPOINT system-wide, but do_each_thread() can race with copy_process() and miss the new child which was not added to init_task.tasks list yet.
Change copy_process() to update the child's TIF_SYSCALL_TRACEPOINT under tasklist.
While at it,
- remove _irqsafe from syscall_regfunc/syscall_unregfunc, read_lock(tasklist) doesn't need to disable irqs.
- change syscall_unregfunc() to check PF_KTHREAD to skip the kernel threads, ->mm != NULL is the common mistake.
Note: probably this check should be simply removed, needs another patch.
--- include/trace/syscall.h | 15 +++++++++++++++ kernel/fork.c | 2 ++ kernel/tracepoint.c | 12 +++++------- 3 files changed, 22 insertions(+), 7 deletions(-)
include/trace/syscall.h +15 -0
@@ -4,6 +4,7 @@#include <linux/tracepoint.h> #include <linux/unistd.h> #include <linux/ftrace_event.h> +#include <linux/thread_info.h> #include <asm/ptrace.h>
@@ -31,4 +32,18 @@ struct syscall_metadata { struct ftrace_event_call *exit_event; }; +#ifdef CONFIG_TRACEPOINTS +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +if (test_thread_flag (TIF_SYSCALL_TRACEPOINT )) +set_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +else +clear_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +} +#else +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +} +#endif +#endif /* _TRACE_SYSCALL_H */
kernel/fork.c +2 -0
@@ -1472,7 +1472,9 @@ static struct task_struct *copy_process(unsigned long clone_flags, total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +proc_fork_connector (p);cgroup_post_fork (p);if (clone_flags &CLONE_THREAD )
kernel/tracepoint.c +5 -7
@@ -732,33 +732,31 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) { -unsigned long flags;struct task_struct *g, *t;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {/* Skip kernel threads. */ -if (t->mm) +if (!(t->flags &PF_KTHREAD ))set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); } sys_tracepoint_refcount++; }void syscall_unregfunc (void ) { -unsigned long flags;struct task_struct *g, *t; sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); } }#endif
Signature · 1 line
1.5.5.1
Steven Rostedt
maintainerRe: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
Quoted 13 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()andsyscall_unregfunc()should set/clearTIF_SYSCALL_TRACEPOINTsystem-wide, butdo_each_thread()can race withcopy_process()and miss the new child which was not added to init_task.tasks list yet.Change
copy_process()to update the child'sTIF_SYSCALL_TRACEPOINTunder tasklist.While at it,
- remove
_irqsafefrom syscall_regfunc/syscall_unregfunc,read_lock(tasklist)doesn't need to disable irqs.
Quoted 7 lines
- change
syscall_unregfunc()to checkPF_KTHREADto skip the kernel threads, ->mm != NULL is the common mistake.Note: probably this check should be simply removed, needs another patch.
Signed-off-by Oleg Nesterov <[email protected]>
BTW, how important is this fix? Something we should aim for the current merge window? stable? Or can it wait till 3.16?
-- Steve
Re: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
Quoted 20 lines · Steven Rostedt
Quoted 16 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()andsyscall_unregfunc()should set/clearTIF_SYSCALL_TRACEPOINTsystem-wide, butdo_each_thread()can race withcopy_process()and miss the new child which was not added to init_task.tasks list yet.Change
copy_process()to update the child'sTIF_SYSCALL_TRACEPOINTunder tasklist.While at it,
- remove
_irqsafefrom syscall_regfunc/syscall_unregfunc,read_lock(tasklist)doesn't need to disable irqs.- change
syscall_unregfunc()to checkPF_KTHREADto skip the kernel threads, ->mm != NULL is the common mistake.
Quoted 7 lines
Quoted 4 lines
Note: probably this check should be simply removed, needs another patch.
Signed-off-by Oleg Nesterov <[email protected]>BTW, how important is this fix? Something we should aim for the current merge window? stable? Or can it wait till 3.16?
I think this all can wait till 3.16. The problem is really minor.
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
In reply to Oleg NesterovQuoted 62 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()andsyscall_unregfunc()should set/clearTIF_SYSCALL_TRACEPOINTsystem-wide, butdo_each_thread()can race withcopy_process()and miss the new child which was not added to init_task.tasks list yet.Change
copy_process()to update the child'sTIF_SYSCALL_TRACEPOINTunder tasklist.While at it,
- remove
_irqsafefrom syscall_regfunc/syscall_unregfunc,read_lock(tasklist)doesn't need to disable irqs.- change
syscall_unregfunc()to checkPF_KTHREADto skip the kernel threads, ->mm != NULL is the common mistake.Note: probably this check should be simply removed, needs another patch.
Signed-off-by Oleg Nesterov <[email protected]>--- include/trace/syscall.h | 15 +++++++++++++++ kernel/fork.c | 2 ++ kernel/tracepoint.c | 12 +++++------- 3 files changed, 22 insertions(+), 7 deletions(-)include/trace/syscall.h +15
-0@@ -4,6 +4,7 @@#include <linux/tracepoint.h> #include <linux/unistd.h> #include <linux/ftrace_event.h> +#include <linux/thread_info.h> #include <asm/ptrace.h> @@ -31,4 +32,18 @@ struct syscall_metadata { struct ftrace_event_call *exit_event; }; +#ifdef CONFIG_TRACEPOINTS +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +if (test_thread_flag (TIF_SYSCALL_TRACEPOINT )) +set_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +else +clear_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +} +#else +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +} +#endif +#endif /* _TRACE_SYSCALL_H */ kernel/fork.c +0
-0
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1477
Quoted 6 lines
kernel/fork.c +2
-0@@ -1472,7 +1472,9 @@ static struct task_struct *copy_process(unsigned long clone_flags, total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +
So this looks to be a fix that probably should go to stable?
Quoted 33 lines
kernel/fork.c +0
-0proc_fork_connector (p);cgroup_post_fork (p);if (clone_flags &CLONE_THREAD )kernel/tracepoint.c +3
-5@@ -732,33 +732,31 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) { -unsigned long flags;struct task_struct *g, *t;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {/* Skip kernel threads. */ -if (t->mm) +if (!(t->flags &PF_KTHREAD ))set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); } sys_tracepoint_refcount++; }void syscall_unregfunc (void ) { -unsigned long flags;struct task_struct *g, *t;
kernel/tracepoint.c static int sys_tracepoint_refcount; · line 759
Quoted 10 lines
kernel/tracepoint.c +2
-2sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock);
These changes should be in a separate patch, as they can wait till 3.16.
-- Steve
Quoted 3 lines
kernel/tracepoint.c +0
-0} }#endif
Re: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
kernel/fork.c +0
-0@@ -1472,7 +1472,9 @@ static struct task_struct *copy_process(unsigned long clone_flags,
Quoted 7 lines
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1477
Quoted 5 lines
kernel/fork.c +2
-0total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +So this looks to be a fix that probably should go to stable?
Not sure, this is up to you.
Quoted 27 lines
Quoted 27 lines
kernel/fork.c +3
-5@@ -732,33 +732,31 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) { -unsigned long flags;struct task_struct *g, *t;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {/* Skip kernel threads. */ -if (t->mm) +if (!(t->flags &PF_KTHREAD ))set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); } sys_tracepoint_refcount++; }void syscall_unregfunc (void ) { -unsigned long flags;struct task_struct *g, *t;
Quoted 11 lines
kernel/fork.c static int sys_tracepoint_refcount; · line 759
Quoted 9 lines
kernel/fork.c +2
-2sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock);These changes should be in a separate patch, as they can wait till 3.16.
OK, I'll split this change.
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
Quoted 17 lines · On Thu, 10 Apr 2014 15:34:35 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
kernel/fork.c +0
-0@@ -1472,7 +1472,9 @@ static struct task_struct *copy_process(unsigned long clone_flags,Quoted 7 lines
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1477
Quoted 5 lines
kernel/fork.c +2
-0total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +So this looks to be a fix that probably should go to stable?
Quoted 1 line
Not sure, this is up to you.
I'll have to think about this one, whether or not it should go to stable.
Quoted 40 lines
Quoted 28 lines
Quoted 28 lines
kernel/fork.c +3
-5@@ -732,33 +732,31 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) { -unsigned long flags;struct task_struct *g, *t;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {/* Skip kernel threads. */ -if (t->mm) +if (!(t->flags &PF_KTHREAD ))set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); } sys_tracepoint_refcount++; }void syscall_unregfunc (void ) { -unsigned long flags;struct task_struct *g, *t;Quoted 11 lines
kernel/fork.c static int sys_tracepoint_refcount; · line 759
Quoted 9 lines
kernel/fork.c +2
-2sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock); do_each_thread(g, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock);These changes should be in a separate patch, as they can wait till 3.16.
Quoted 1 line
OK, I'll split this change.
Are you going to send a new series?
-- Steve
Re: [PATCH RESEND 1/2] tracing: syscall_*regfunc() can race with copy_process()
Quoted 25 lines · Steven Rostedt
Quoted 17 lines · On Thu, 10 Apr 2014 15:34:35 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Wed, 9 Apr 2014 19:05:42 +0200 Oleg Nesterov <[email protected]> wrote:
kernel/fork.c +0
-0@@ -1472,7 +1472,9 @@ static struct task_struct *copy_process(unsigned long clone_flags,Quoted 7 lines
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1477
Quoted 5 lines
kernel/fork.c +2
-0total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +So this looks to be a fix that probably should go to stable?
Quoted 1 line
Not sure, this is up to you.
I'll have to think about this one, whether or not it should go to stable.
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
These changes should be in a separate patch, as they can wait till 3.16.
OK, I'll split this change.
Are you going to send a new series?
Yes, will do. I will split 1/2, and I need to update the changelog in 2/2.
Oleg.
v2 cover tracing: syscall_*regfunc() fixes
Quoted 7 lines · Oleg Nesterov
Quoted 2 lines · Steven Rostedt
Are you going to send a new series?
Yes, will do. I will split 1/2, and I need to update the changelog in 2/2.
Please see the patches.
Frederic! I am sorry, I decided to steal your patch to simplify the merging and avoid too many changes in the trivial syscall_*regfunc's. In case you do not object, could you add your sob into 2/3 ? Otherwise I can wait for your patch, then send v3 on top of it.
Hmm. This is off-topic, but CLONE_KERNEL should die, and kernel_init() should be spawned without CLONE_SIGHAND. I'll send another patch.
Oleg.
PATCH v2 1/3 tracing: fix syscall_*regfunc() vs copy_process() race
syscall_regfunc() and syscall_unregfunc() should set/clear TIF_SYSCALL_TRACEPOINT system-wide, but do_each_thread() can race with copy_process() and miss the new child which was not added to the process/thread lists yet.
Change copy_process() to update the child's TIF_SYSCALL_TRACEPOINT under tasklist.
--- include/trace/syscall.h | 15 +++++++++++++++ kernel/fork.c | 2 ++ 2 files changed, 17 insertions(+), 0 deletions(-)
include/trace/syscall.h +15 -0
@@ -4,6 +4,7 @@#include <linux/tracepoint.h> #include <linux/unistd.h> #include <linux/ftrace_event.h> +#include <linux/thread_info.h> #include <asm/ptrace.h>
@@ -32,4 +33,18 @@ struct syscall_metadata { struct ftrace_event_call *exit_event; }; +#ifdef CONFIG_TRACEPOINTS +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +if (test_thread_flag (TIF_SYSCALL_TRACEPOINT )) +set_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +else +clear_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +} +#else +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +} +#endif +#endif /* _TRACE_SYSCALL_H */
kernel/fork.c +2 -0
@@ -1484,7 +1484,9 @@ static struct task_struct *copy_process(unsigned long clone_flags, total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +proc_fork_connector (p);cgroup_post_fork (p);if (clone_flags &CLONE_THREAD )
Signature · 1 line
1.5.5.1
Re: [PATCH v2 1/3] tracing: fix syscall_*regfunc() vs copy_process() race
Quoted 9 lines · Oleg Nesterov
syscall_regfunc()andsyscall_unregfunc()should set/clearTIF_SYSCALL_TRACEPOINTsystem-wide, butdo_each_thread()can race withcopy_process()and miss the new child which was not added to the process/thread lists yet.Change
copy_process()to update the child'sTIF_SYSCALL_TRACEPOINTunder tasklist.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 54 lines
--- include/trace/syscall.h | 15 +++++++++++++++ kernel/fork.c | 2 ++ 2 files changed, 17 insertions(+), 0 deletions(-)include/trace/syscall.h +15
-0@@ -4,6 +4,7 @@#include <linux/tracepoint.h> #include <linux/unistd.h> #include <linux/ftrace_event.h> +#include <linux/thread_info.h> #include <asm/ptrace.h> @@ -32,4 +33,18 @@ struct syscall_metadata { struct ftrace_event_call *exit_event; }; +#ifdef CONFIG_TRACEPOINTS +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +if (test_thread_flag (TIF_SYSCALL_TRACEPOINT )) +set_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +else +clear_tsk_thread_flag (p,TIF_SYSCALL_TRACEPOINT ); +} +#else +static inline void syscall_tracepoint_update (struct task_struct *p) +{ +} +#endif +#endif /* _TRACE_SYSCALL_H */ kernel/fork.c +2
-0@@ -1484,7 +1484,9 @@ static struct task_struct *copy_process(unsigned long clone_flags, total_forks++;spin_unlock (¤t->sighand->siglock); +syscall_tracepoint_update (p);write_unlock_irq (&tasklist_lock); +proc_fork_connector (p);cgroup_post_fork (p);if (clone_flags &CLONE_THREAD )Signature · 1 line
1.5.5.1
PATCH v2 2/3 tracing: change syscall_*regfunc() to check PF_KTHREAD and use for_each_process_thread()
In reply to Oleg Nesterov1. Remove _irqsafe from syscall_regfunc/syscall_unregfunc, read_lock(tasklist) doesn't need to disable irqs.
2. Change this code to avoid the deprecated do_each_thread() and use for_each_process_thread() (stolen from the patch from Frederic).
3. Change syscall_regfunc() to check PF_KTHREAD to skip the kernel threads, ->mm != NULL is the common mistake.
Note: probably this check should be simply removed, needs another patch.
--- kernel/tracepoint.c | 24 +++++++++++------------- 1 files changed, 11 insertions(+), 13 deletions(-)
kernel/tracepoint.c +11 -13
@@ -742,33 +742,31 @@ static int sys_tracepoint_refcount;void syscall_regfunc (void ) { -unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +read_lock (&tasklist_lock); +for_each_process_thread (p, t) {/* Skip kernel threads. */ -if (t->mm) +if (!(t->flags &PF_KTHREAD ))set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); + } +read_unlock (&tasklist_lock); } sys_tracepoint_refcount++; }void syscall_unregfunc (void ) { -unsigned long flags; -struct task_struct *g, *t; +struct task_struct *p, *t; sys_tracepoint_refcount--;if (!sys_tracepoint_refcount) { -read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, t) { +read_lock (&tasklist_lock); +for_each_process_thread (p, t) {clear_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); - }while_each_thread (g, t); -read_unlock_irqrestore (&tasklist_lock, flags); + } +read_unlock (&tasklist_lock); } }#endif
Signature · 1 line
1.5.5.1
PATCH v2 3/3 tracing: syscall_regfunc() should not skip kernel threads
In reply to Oleg Nesterovsyscall_regfunc() ignores the kernel threads because "it has no effect", see cc3b13c1 "Don't trace kernel thread syscalls" which added this check.
However, this means that a user-space task spawned by call_usermodehelper() will run without TIF_SYSCALL_TRACEPOINT if sys_tracepoint_refcount != 0.
Remove this check. The unnecessary report from ret_from_fork path mentioned by cc3b13c1 is no longer possible, see See commit fb45550d76bb5 "make sure that kernel_thread() callbacks call do_exit() themselves".
A kernel_thread() callback can only return and take the int_ret_from_sys_call path after do_execve() succeeds, otherwise the kernel will crash. But in this case it is no longer a kernel thread and thus is needs TIF_SYSCALL_TRACEPOINT.
--- kernel/tracepoint.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)
kernel/tracepoint.c +1 -3
@@ -747,9 +747,7 @@ void syscall_regfunc(void)if (!sys_tracepoint_refcount) {read_lock (&tasklist_lock);for_each_process_thread (p, t) { -/* Skip kernel threads. */ -if (!(t->flags &PF_KTHREAD )) -set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); +set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }read_unlock (&tasklist_lock); }
Signature · 1 line
1.5.5.1
Re: [PATCH v2 0/3] tracing: syscall_*regfunc() fixes
In reply to Oleg NesterovQuoted 6 lines · Oleg Nesterov
Quoted 5 lines · Oleg Nesterov
Quoted 2 lines · Steven Rostedt
Are you going to send a new series?
Quoted 8 lines
Quoted 2 lines
Yes, will do. I will split 1/2, and I need to update the changelog in 2/2.
Please see the patches.
Frederic! I am sorry, I decided to steal your patch to simplify the merging and avoid too many changes in the trivial syscall_*regfunc's. In case you do not object, could you add your sob into 2/3 ?
Sure, feel free to add it!
Thanks!
Quoted 10 lines
Otherwise
I can wait for your patch, then send v3 on top of it.Hmm. This is off-topic, but
CLONE_KERNELshould die, andkernel_init()should be spawned withoutCLONE_SIGHAND. I'll send another patch.Oleg.
Steven Rostedt
maintainerRe: [PATCH v2 0/3] tracing: syscall_*regfunc() fixes
In reply to Oleg NesterovQuoted 11 lines · On Sun, 13 Apr 2014 20:58:28 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 7 lines · Oleg Nesterov
Quoted 2 lines · Steven Rostedt
Are you going to send a new series?
Yes, will do. I will split 1/2, and I need to update the changelog in 2/2.
Please see the patches.
I'm pulling these in now and will start tests, wont be till tomorrow till I can push them.
If you start a new patch series, either make them a new thread or at least a reply from the 0/x of the previous version. Having a v2 series starting at the bottom of the 1/x thread will get lost.
-- Steve
Re: [PATCH v2 0/3] tracing: syscall_*regfunc() fixes
Quoted 10 lines · Steven Rostedt
Quoted 6 lines · On Sun, 13 Apr 2014 20:58:28 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 5 lines · Oleg Nesterov
Quoted 2 lines · Steven Rostedt
Are you going to send a new series?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Yes, will do. I will split 1/2, and I need to update the changelog in 2/2.
Please see the patches.
I'm pulling these in now and will start tests, wont be till tomorrow till I can push them.
Great, thanks ;)
Quoted 3 lines
If you start a new patch series, either make them a new thread or at least a reply from the 0/x of the previous version. Having a v2 series starting at the bottom of the 1/x thread will get lost.
Ah, OK, sorry about that.
Oleg.
PATCH RESEND 2/2 tracing: syscall_regfunc() should not skip kernel threads
In reply to Oleg Nesterovsyscall_regfunc() ignores the kernel thread because "it has no effect", see cc3b13c1 "Don't trace kernel thread syscalls".
However, this means that a user-space task spawned by call_usermodehelper() won't report the system calls if kernel_execve() is called when sys_tracepoint_refcount != 0.
Remove this check. Hopefully the unnecessary report from ret_from_fork path mentioned by cc3b13c1 is fine. In fact "this is the only case" is not true. Say, kernel_execve() itself does "int 80" on X86_32. Hopefully fine too.
--- kernel/tracepoint.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)
kernel/tracepoint.c +1 -3
@@ -737,9 +737,7 @@ void syscall_regfunc(void)if (!sys_tracepoint_refcount) {read_lock (&tasklist_lock); do_each_thread(g, t) { -/* Skip kernel threads. */ -if (!(t->flags &PF_KTHREAD )) -set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); +set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t);read_unlock (&tasklist_lock); }
Signature · 1 line
1.5.5.1
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 6 lines · On Wed, 9 Apr 2014 19:06:16 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()ignores the kernel thread because "it has no effect", seecc3b13c1"Don't trace kernel thread syscalls".However, this means that a user-space task spawned by
call_usermodehelper()won't report the system calls ifkernel_execve()is called whensys_tracepoint_refcount!= 0.
What about doing the set there? That is, we could add a check in the call_userspacehelper() just before it does the do_execve, that if sys_tracepoint_refcount is set, we set the TIF flag.
-- Steve
Quoted 26 lines
Remove this check. Hopefully the unnecessary report from
ret_from_forkpath mentioned bycc3b13c1is fine. In fact "this is the only case" is not true. Say,kernel_execve()itself does "int 80" onX86_32. Hopefully fine too.Signed-off-by Oleg Nesterov <[email protected]>--- kernel/tracepoint.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)kernel/tracepoint.c +1
-3@@ -737,9 +737,7 @@ void syscall_regfunc(void)if (!sys_tracepoint_refcount) {read_lock (&tasklist_lock); do_each_thread(g, t) { -/* Skip kernel threads. */ -if (!(t->flags &PF_KTHREAD )) -set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); +set_tsk_thread_flag (t,TIF_SYSCALL_TRACEPOINT ); }while_each_thread (g, t);read_unlock (&tasklist_lock); }
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Wed, 9 Apr 2014 19:06:16 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()ignores the kernel thread because "it has no effect", seecc3b13c1"Don't trace kernel thread syscalls".
Quoted 7 lines
Quoted 3 lines
However, this means that a user-space task spawned by
call_usermodehelper()won't report the system calls ifkernel_execve()is called whensys_tracepoint_refcount!= 0.What about doing the set there? That is, we could add a check in the
call_userspacehelper()just before it does thedo_execve, that ifsys_tracepoint_refcountis set, we set the TIF flag.
But for what?
And if we do this, ____call_usermodehelper() needs write_lock_irq(tasklist) to serialize with syscall_*regfunc().
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 12 lines · On Thu, 10 Apr 2014 15:38:55 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 11 lines · Steven Rostedt
Quoted 6 lines · On Wed, 9 Apr 2014 19:06:16 +0200 Oleg Nesterov <[email protected]> wrote:
syscall_regfunc()ignores the kernel thread because "it has no effect", seecc3b13c1"Don't trace kernel thread syscalls".However, this means that a user-space task spawned by
call_usermodehelper()won't report the system calls ifkernel_execve()is called whensys_tracepoint_refcount!= 0.
Quoted 5 lines
Quoted 3 lines
What about doing the set there? That is, we could add a check in the
call_userspacehelper()just before it does thedo_execve, that ifsys_tracepoint_refcountis set, we set the TIF flag.But for what?
Isn't call_usermodehelper() the reason you added this?
Quoted 3 lines
And if we do this,
____call_usermodehelper()needswrite_lock_irq(tasklist)to serialize with syscall_*regfunc().
You mean for the slight race between checking if its set and when the tracepoint is actually activated?
I don't think we really care about that race. I mean, the tracepoint is activated usually by humans, and if they enabled it just as a usermode helper is activated, and those are really fast to run, do we even care if it is missed?
Now, if tracing is on and we need to set the flag, that should take the task list lock to make sure that we don't miss clearing it. Missing the set isn't a big deal, but missing the clearing of the flag is.
void tracepoint_check_syscalls(void)
{
if (!sys_tracepoint_refcount)
return;
read_lock(&tasklist_lock);
/* Make sure it wasn't cleared since taking the lock */
if (sys_tracepoint_refcount)
set_tsk_thread_flag(current, TIF_SYSCALL_TRACEPOINT);
read_unlock(&tasklist_lock);
}Hmm, probably need to use another lock than tasklist_lock as the updating of sys_tracepoint_refcount is not done under it. But as this is all local to tracepoint.c we could easily add something to do the job
s/read_lock(&tasklist_lock)/spin_lock(&tracepoint_sys_lock)/
-- Steve
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Thu, 10 Apr 2014 15:38:55 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 4 lines
Quoted 3 lines
However, this means that a user-space task spawned by
call_usermodehelper()won't report the system calls ifkernel_execve()is called whensys_tracepoint_refcount!= 0.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
What about doing the set there? That is, we could add a check in the
call_userspacehelper()just before it does thedo_execve, that ifsys_tracepoint_refcountis set, we set the TIF flag.But for what?
Isn't
call_usermodehelper()the reason you added this?
Sure. I meant, why complicate ____call_usermodehelper() and keep the unnecessary complication (PF_KTHREAD check( in syscall_*regfunc() ?
Quoted 5 lines
Quoted 2 lines
And if we do this,
____call_usermodehelper()needswrite_lock_irq(tasklist)to serialize with syscall_*regfunc().You mean for the slight race between checking if its set and when the tracepoint is actually activated?
Or deactivated.
Quoted 1 line
I don't think we really care about that race.
OK, I won't argue. I agree, the problem is minor, but in this case imho it is better to do nothing than add the racy fix.
Quoted 4 lines
I mean, the tracepoint is
activated usually by humans, and if they enabled it just as a usermode helper is activated, and those are really fast to run, do we even care if it is missed?
A user space task spawned by call_usermodehelper() can do everything, it can run forever.
Quoted 5 lines
Now, if tracing is on and we need to set the flag, that should take the task list lock to make sure that we don't miss clearing it. Missing the set isn't a big deal, but missing the clearing of the flag is.
void
tracepoint_check_syscalls(void)
Quoted 10 lines
{
if (!sys_tracepoint_refcount)
return;
read_lock(&tasklist_lock);
/* Make sure it wasn't cleared since taking the lock */
if (sys_tracepoint_refcount)
set_tsk_thread_flag(current, TIF_SYSCALL_TRACEPOINT);
read_unlock(&tasklist_lock);
}And how this can help to avoid the race? We need write_lock_irq().
Perhaps I missed something... and I simply do not understand why do you want to do this.
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 7 lines · On Thu, 10 Apr 2014 16:46:55 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 4 lines
I mean, the tracepoint is
activated usually by humans, and if they enabled it just as a usermode helper is activated, and those are really fast to run, do we even care if it is missed?A user space task spawned by
call_usermodehelper()can do everything, it can run forever.
Sounds nasty ;-)
Quoted 10 lines
Quoted 10 lines
Now, if tracing is on and we need to set the flag, that should take the task list lock to make sure that we don't miss clearing it. Missing the set isn't a big deal, but missing the clearing of the flag is.
void
tracepoint_check_syscalls(void){ if (!sys_tracepoint_refcount) return;
Quoted 8 lines
Quoted 6 lines
read_lock(&tasklist_lock); /* Make sure it wasn't cleared since taking the lock */ if (sys_tracepoint_refcount) set_tsk_thread_flag(current, TIF_SYSCALL_TRACEPOINT); read_unlock(&tasklist_lock); }And how this can help to avoid the race? We need
write_lock_irq().
But you chopped off the last part. Where I replaced tasklist_lock with a tracepoint specific lock that would synchronize sys_tracepoint_refcount with the setting of the flags.
Quoted 3 lines
Perhaps I missed something... and I simply do not understand why do you want to do this.
Because I'm being an ass ;-)
The real reason I'm doing this debate is more to find out exactly what the problems are. A learning exercise if you will. I just don't want to add a regression, as Hendrik (which I just Cc'd) added the commit for a reason. Perhaps you are correct that we should just go back to the way things were.
Hendrik, we are debating about removing cc3b13c11c567c69a6356be98d0c03ff11541d5c as it stops call_usermodehelper tasks from tracing their syscalls.
If Hendrik has no problems with this, neither do I.
-- Steve
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 16 lines · Steven Rostedt
Quoted 12 lines · On Thu, 10 Apr 2014 16:46:55 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 11 lines
void
tracepoint_check_syscalls(void){ if (!sys_tracepoint_refcount) return; read_lock(&tasklist_lock); /* Make sure it wasn't cleared since taking the lock */ if (sys_tracepoint_refcount) set_tsk_thread_flag(current, TIF_SYSCALL_TRACEPOINT); read_unlock(&tasklist_lock); }
Quoted 5 lines
Quoted 1 line
And how this can help to avoid the race? We need
write_lock_irq().But you chopped off the last part. Where I replaced
tasklist_lockwith a tracepoint specific lock that would synchronizesys_tracepoint_refcountwith the setting of the flags.
Yes sure, if we add another lock everything is fine.
Quoted 4 lines
Quoted 2 lines
Perhaps I missed something... and I simply do not understand why do you want to do this.
Because I'm being an ass ;-)
Nothing new, I always knew this ;)
Quoted 5 lines
The real reason I'm doing this debate is more to find out exactly what the problems are. A learning exercise if you will. I just don't want to add a regression, as Hendrik (which I just Cc'd) added the commit for a reason. Perhaps you are correct that we should just go back to the way things were.
Sure, this should be verified. Besides, the changelog is very old. It says "kernel_execve() itself does "int 80" on X86_32.", this is no longer true.
Quoted 5 lines
Hendrik, we are debating about removing
cc3b13c11c567c69a6356be98d0c03ff11541d5cas it stopscall_usermodehelpertasks from tracing their syscalls.If Hendrik has no problems with this, neither do I.
OK.
cc3b13c11c567 mentions ret_from_fork, today copy_thread(PF_KTHREAD) uses ret_from_kernel_thread on 32bit, and still ret_from_fork on 64 bit but in the last case it checks PF_KTHREAD... I am wondering why they both (ret_from_kernel_thread and "1: " label in ret_from_fork) can't simply call do_exit() at the end?
And, since they do not, every kernel_thread's function (fn argument of kernel_thread) must call do_exit itself?
Looks a bit strange, I guess I missed something obvious.
Oleg.
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 9 lines · Oleg Nesterov
Quoted 6 lines · Steven Rostedt
Hendrik, we are debating about removing
cc3b13c11c567c69a6356be98d0c03ff11541d5cas it stopscall_usermodehelpertasks from tracing their syscalls.If Hendrik has no problems with this, neither do I.
Quoted 7 lines
OK.
cc3b13c11c567mentionsret_from_fork, todaycopy_thread(PF_KTHREAD)usesret_from_kernel_threadon 32bit, and stillret_from_forkon 64 bit but in the last case it checksPF_KTHREAD... I am wondering why they both (ret_from_kernel_threadand "1: " label inret_from_fork) can't simply calldo_exit()at the end?
probably because we need to change all architectures...
Quoted 2 lines
And, since they do not, every kernel_thread's function (fn argument of
kernel_thread) must calldo_exititself?
Hmm yes. See fb45550d76bb5 "make sure that kernel_thread() callbacks call do_exit() themselves".
Quoted 1 line
Looks a bit strange, I guess I missed something obvious.
And I forgot to mention, given that the kernel_thread() callback should call do_exit() itself, then this part of cc3b13c11c567c69a63
one case when a kernel thread can reach the
usual syscall exit tracing path: when we create a kernel thread, the child comes to ret_from_fork
is no longer relevant? A PF_KTHREAD child should never return from the callback and thus it should never do "jmp syscall_exit" ?
Or I am totally confused?
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 10 lines · On Thu, 10 Apr 2014 20:14:17 +0200 Oleg Nesterov <[email protected]> wrote:
And I forgot to mention, given that the
kernel_thread()callback should calldo_exit()itself, then this part ofcc3b13c11c567c69a63one case when a kernel thread can reach the
usual syscall exit tracing path: when we create a kernel thread, the child comes toret_from_forkis no longer relevant? A
PF_KTHREADchild should never return from the callback and thus it should never do "jmpsyscall_exit" ?
Are you sure. On set up of the kthread, create_kthread() calls kernel_thread() with "kthread()" as its first parameter.
kernel_thread() then calls do_fork() passing the "kthread" function as the stack_start parameter, which if you follow where that goes, it gets to copy_thread() in process_[63][42].c which assigns sp (the function) to the bx register for the PF_KTHREAD case. But more importantly, it sets up the stack to have ip pointing to ret_from_kernel_thread (32 bit version).
The jmp syscall_exit when it goes to return to "userspace" will in actuality return to ret_from_kernel_thread (32 bit). Which this does:
call *PT_EBX(%esp)
which calls your handler. But then again, this calls syscall_exit when done, which probably will never be hit as kthread() calls do_exit() itself. Perhaps if something goes wrong, syscall_exit can handle any faults that can happen?
For 64 bit, the check for kernel thread is in ret_from_fork itself. which does the call *%rbx, but again, if it fails, it then calls int_ret_from_sys_call, which it may also handle faults.
Looks like kernel threads on 32bit call syscall exit at least once, to get to ret_from_kernel_thread. Not sure why it does that. Perhaps this could be another 32bit clean up to make it more like x86_64.
-- Steve
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 12 lines · Steven Rostedt
Quoted 7 lines · On Thu, 10 Apr 2014 20:14:17 +0200 Oleg Nesterov <[email protected]> wrote:
And I forgot to mention, given that the
kernel_thread()callback should calldo_exit()itself, then this part ofcc3b13c11c567c69a63one case when a kernel thread can reach the
usual syscall exit tracing path: when we create a kernel thread, the child comes toret_from_fork
Quoted 5 lines
Quoted 3 lines
is no longer relevant? A
PF_KTHREADchild should never return from the callback and thus it should never do "jmpsyscall_exit" ?Are you sure.
Not.
Quoted 15 lines
On set up of the kthread,
create_kthread()callskernel_thread()with "kthread()" as its first parameter.
kernel_thread()then callsdo_fork()passing the "kthread" function as thestack_startparameter, which if you follow where that goes, it gets tocopy_thread()in process_[63][42].c which assigns sp (the function) to the bx register for thePF_KTHREADcase. But more importantly, it sets up the stack to have ip pointing toret_from_kernel_thread(32 bit version).The jmp
syscall_exitwhen it goes to return to "userspace" will in actuality return toret_from_kernel_thread(32 bit). Which this does:call *PT_EBX(%esp)
Quoted 8 lines
which calls your handler. But then again, this calls
syscall_exitwhen done, which probably will never be hit askthread()callsdo_exit()itself. Perhaps if something goes wrong,syscall_exitcan handle any faults that can happen?For 64 bit, the check for kernel thread is in
ret_from_forkitself. which does the call *%rbx, but again, if it fails, it then callsint_ret_from_sys_call, which it may also handle faults.
See my previous email.
I _think_ that the kernel thread can only return from "call *%rbx" if it is no longer a kernel thread, iow, do_execve() was called.
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 2 lines · On Thu, 10 Apr 2014 21:38:20 +0200 Oleg Nesterov <[email protected]> wrote:
I _think_ that the kernel thread can only return from "call *%rbx" if it is no longer a kernel thread, iow,
do_execve()was called.
Ah right. But only in special cases.
Actually, it only returns if the function in kernel_thread() returns, and in the case of ____call_usermodehelper, it does an exec and returns on success. But if it fails, it needs to call do_exit().
Thus, it's not sufficient to just say "only if do_execve is called" but to say, if the handler that is called is only allowed to return iff it did a execve first and succeeded.
-- Steve
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 4 lines · Steven Rostedt
On Thu, 10 Apr 2014 21:38:20 +0200
Oleg Nesterov <[email protected]> wrote:
Quoted 8 lines
Quoted 2 lines
I _think_ that the kernel thread can only return from "call *%rbx" if it is no longer a kernel thread, iow,
do_execve()was called.Ah right. But only in special cases.
Actually, it only returns if the function in
kernel_thread()returns, and in the case of____call_usermodehelper, it does an exec and returns on success. But if it fails, it needs to calldo_exit().
Yes. If it simply returns the kernel will crash, without start_thread/etc "int_ret_from_sys_call" can do nothing good.
So we can conclude that "there is only one case when a kernel thread can reach the usual syscall exit tracing path:" from cc3b13c11c567c69 is no longer valid and we can kille the PF_KTHREAD check in regfuncs.
Quoted 3 lines
Thus, it's not sufficient to just say "only if
do_execveis called" but to say, if the handler that is called is only allowed to return iff it did a execve first and succeeded.
I do remember, I already told you are pedant!
Oleg.
Steven Rostedt
maintainerRe: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
Quoted 1 line · On Fri, 11 Apr 2014 14:03:47 +0200 Oleg Nesterov <[email protected]> wrote:
I do remember, I already told you are pedant!
Point taken ;-)
I'll pull your patches into my 3.16 queue.
Thanks!
-- Steve
Re: [PATCH RESEND 2/2] tracing: syscall_regfunc() should not skip kernel threads
In reply to Oleg NesterovQuoted 13 lines · Oleg Nesterov
Quoted 11 lines · Oleg Nesterov
Quoted 6 lines · Steven Rostedt
Hendrik, we are debating about removing
cc3b13c11c567c69a6356be98d0c03ff11541d5cas it stopscall_usermodehelpertasks from tracing their syscalls.If Hendrik has no problems with this, neither do I.
OK.
Quoted 7 lines
Quoted 5 lines
cc3b13c11c567mentionsret_from_fork, todaycopy_thread(PF_KTHREAD)usesret_from_kernel_threadon 32bit, and stillret_from_forkon 64 bit but in the last case it checksPF_KTHREAD... I am wondering why they both (ret_from_kernel_threadand "1: " label inret_from_fork) can't simply calldo_exit()at the end?probably because we need to change all architectures...
Heh. And because "call *%rbx" can actually return if this kernel thread execs ;)
But in this case we want TIF_SYSCALL_TRACING, so
Quoted 11 lines
Quoted 2 lines
And, since they do not, every kernel_thread's function (fn argument of
kernel_thread) must calldo_exititself?Hmm yes. See
fb45550d76bb5"make sure thatkernel_thread()callbacks calldo_exit()themselves".Quoted 1 line
Looks a bit strange, I guess I missed something obvious.
And I forgot to mention, given that the
kernel_thread()callback should calldo_exit()itself, then this part ofcc3b13c11c567c69a63
Quoted 6 lines
one case when a kernel thread can reach the
usual syscall exit tracing path: when we create a kernel thread, the child comes toret_from_forkis no longer relevant? A
PF_KTHREADchild should never return from the callback and thus it should never do "jmpsyscall_exit" ?
this is still true, I guess.
Oleg.
Steven Rostedt
maintainerRe: [PATCH 0/2] Was: Convert process iteration to use for_each_process_thread()
In reply to Oleg NesterovQuoted 11 lines · On Wed, 9 Apr 2014 19:05:05 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 2 lines · Steven Rostedt
The
tasklist_lockis all that is needed.Yes, but not
_irq. Plus this code is buggy ;)Steven, I sent these patches twice (at least!). This time I didn't bother to recheck this code, but the patches still apply so perhaps you can take a look?
Last attempt, I promise ;)
Ah OK, and I actually have time to look at them now :-)
-- Steve
PATCH 3/5 hung_task: Convert process iteration to use for_each_process_thread()
In reply to Frederic Weisbeckerdo_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the hung task detector code. The ultimate goal is to remove the struct task_struct::thread_group field and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.
It also makes the hung task threads iteration eventually RCU safe.
--- kernel/hung_task.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/hung_task.c +4 -4
@@ -157,7 +157,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout) {int max_count = sysctl_hung_task_check_count;int batch_count =HUNG_TASK_BATCHING ; -struct task_struct *g, *t; +struct task_struct *p, *t;/* * If the system crashed already then all bets are off,
@@ -167,18 +167,18 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (p, t) {if (!max_count--) goto unlock;if (!--batch_count) { batch_count =HUNG_TASK_BATCHING ; -if (!rcu_lock_break (g, t)) +if (!rcu_lock_break (p, t)) goto unlock; }/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ if (t->state ==TASK_UNINTERRUPTIBLE )check_hung_task (t, timeout); - }while_each_thread (g, t); + } unlock:rcu_read_unlock (); }
Signature · 1 line
1.8.3.1
Re: [PATCH 3/5] hung_task: Convert process iteration to use for_each_process_thread()
kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 175
Quoted 13 lines · Frederic Weisbecker
kernel/hung_task.c +2
-2@@ -167,18 +167,18 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (p, t) {if (!max_count--) goto unlock;if (!--batch_count) { batch_count =HUNG_TASK_BATCHING ; -if (!rcu_lock_break (g, t)) +if (!rcu_lock_break (p, t))
Not sure. And in any case this is suboptimal. Frederic, I'll write another email tomorrow. In fact I already have the patch which changes check_hung_uninterruptible_tasks() somewhere.
Oleg.
PATCH 4/5 procfs: Convert process iteration to use for_each_thread()
In reply to Frederic Weisbeckerdo_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in procfs. The ultimate goal is to remove the struct task_struct::thread_group field and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.
--- fs/proc/array.c | 7 ++++--- fs/proc/base.c | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-)
fs/proc/array.c +4 -3
@@ -439,12 +439,13 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,/* add up live thread stats at the group level */ if (whole) { -struct task_struct *t = task; - do { +struct task_struct *t; + +for_each_thread (task, t) { min_flt += t->min_flt; maj_flt += t->maj_flt; gtime +=task_gtime (t); - } while_each_thread(task, t); + } min_flt += sig->min_flt; maj_flt += sig->maj_flt;
fs/proc/base.c +2 -2
@@ -2437,10 +2437,10 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole) }if (whole &&lock_task_sighand (task, &flags)) { -struct task_struct *t = task; +struct task_struct *t;task_io_accounting_add (&acct, &task->signal->ioac); - while_each_thread(task, t) +for_each_thread (task, t)task_io_accounting_add (&acct, &t->ioac);unlock_task_sighand (task, &flags);
Signature · 1 line
1.8.3.1
PATCH 5/5 sched: Convert tasks iteration to use for_each_process_thread()
In reply to Frederic Weisbeckerdo_each_thread/while_each_thread iterators are deprecated by for_each_thread/for_each_process_thread() APIs.
Lets convert the callers in the sched subsystem. The ultimate goal is to remove the struct task_struct::thread_group field and the corresponding do_each_thread/while_each_thread iterators that are RCU unsafe.
--- kernel/sched/core.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/sched/core.c +6 -7
@@ -4389,7 +4389,7 @@ void show_state_filter(unsigned long state_filter) " task PC stack pid father\n");#endif rcu_read_lock (); - do_each_thread(g, p) { +for_each_process_thread (g, p) {/* * reset the NMI-timeout, listing all files on a slow * console might take a lot of time:
@@ -4397,8 +4397,7 @@ void show_state_filter(unsigned long state_filter)touch_nmi_watchdog ();if (!state_filter || (p->state & state_filter))sched_show_task (p); - }while_each_thread (g, p); - + }touch_all_softlockup_watchdogs ();#ifdef CONFIG_SCHED_DEBUG
@@ -6997,7 +6996,7 @@ void normalize_rt_tasks(void)struct rq *rq;read_lock_irqsave (&tasklist_lock, flags); - do_each_thread(g, p) { +for_each_process_thread (g, p) {/* * Only normalize user tasks: */
@@ -7028,7 +7027,7 @@ void normalize_rt_tasks(void)__task_rq_unlock (rq);raw_spin_unlock (&p->pi_lock); - }while_each_thread (g, p); + }read_unlock_irqrestore (&tasklist_lock, flags); }
@@ -7217,10 +7216,10 @@ static inline int tg_has_rt_tasks(struct task_group *tg) {struct task_struct *g, *p; - do_each_thread(g, p) { +for_each_process_thread (g, p) {if (rt_task (p) &&task_rq (p)->rt.tg == tg)return 1 ; - }while_each_thread (g, p); + }return 0 ; }
Signature · 1 line
1.8.3.1