task_work_queue() with 2 users
Subsystems (7)
v3 cover task_work_queue() with 2 users
Hello.
Compared to v2
1/3: make it (partly) useable without TIF_NOTIFY_RESUME
2/3: address the comments from David
3/3: New. Looks trivial, but I guess needs more testing.
And of course, needs the review from Thomas.
Oleg.
include/linux/interrupt.h | 4 -- include/linux/key.h | 6 +-- include/linux/sched.h | 12 ++---- include/linux/task_work.h | 32 ++++++++++++++++ include/linux/tracehook.h | 14 ++++++-- kernel/Makefile | 2 +- kernel/exit.c | 5 ++- kernel/fork.c | 1 + kernel/irq/manage.c | 69 +++++++++++++++++------------------ kernel/task_work.c | 81 ++++++++++++++++++++++++++++++++++++++++++ security/keys/internal.h | 3 +- security/keys/keyctl.c | 76 ++++++++++++++++++-------------------- security/keys/process_keys.c | 25 ++++--------- 13 files changed, 213 insertions(+), 117 deletions(-)
PATCH v3 1/3 task_work_queue: add generic process-context callbacks
Provide a simple mechanism that allows running code in the (nonatomic) context of the arbitrary task.
The caller does task_work_queue(task, task_work) and this task executes task_work->func() either from do_notify_resume() or from do_exit(). The callback can rely on PF_EXITING to detect the latter case.
"struct task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.
This allows us to kill the ->replacement_session_keyring hack, and potentially this can have more users.
Performance-wise, this adds 2 "unlikely(!hlist_empty())" checks into tracehook_notify_resume() and do_exit(). But at the same time we can remove the "replacement_session_keyring != NULL" checks from arch/*/signal.c and exit_creds().
Note: task_work_queue/task_work_run abuses ->pi_lock. This is only because this lock is already used by lookup_pi_state() to synchronize with do_exit() setting PF_EXITING. Fortunately the scope of this lock in task_work.c is really tiny, and the code is unlikely anyway.
v3:
- task_work_queue() gets the new arg, "bool notify" to conditionalize set_notify_resume(), this makes it useable for kthreads and task_work_queue(notify => false) can work without TIF_NOTIFY_RESUME.
- don't add the dummy "ifndef TIF_NOTIFY_RESUME" inlines, just add the simple check in task_work_queue().
Todo:
- move clear_thread_flag(TIF_NOTIFY_RESUME) from arch/ to tracehook_notify_resume()
- rename tracehook_notify_resume() and move it into linux/task_work.h
- m68k and xtensa don't have TIF_NOTIFY_RESUME and thus task_work_queue(notify => true) fails with -ENOTSUPP.
However, ->replacement_session_keyring equally needs this logic, task_work_queue() is not worse.
--- include/linux/sched.h | 2 + include/linux/task_work.h | 32 ++++++++++++++++++ include/linux/tracehook.h | 14 ++++++-- kernel/Makefile | 2 +- kernel/exit.c | 5 ++- kernel/fork.c | 1 + kernel/task_work.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 132 insertions(+), 5 deletions(-) create mode 100644 include/linux/task_work.h create mode 100644 kernel/task_work.c
include/linux/sched.h +2 -0
@@ -1445,6 +1445,8 @@ struct task_struct {int (*notifier)(void *priv);void *notifier_data;sigset_t *notifier_mask; +struct hlist_head task_works; +struct audit_context *audit_context;#ifdef CONFIG_AUDITSYSCALL uid_t loginuid;
include/linux/task_work.h new file +32 -0
@@ -0,0 +1,32 @@ +#ifndef _LINUX_TASK_WORK_H +#define _LINUX_TASK_WORK_H 1 + +#include <linux/sched.h> + +struct task_work ; +typedef void (*task_work_func_t )(struct task_work *); + +struct task_work { +struct hlist_node hlist; +task_work_func_t func; +void *data; +}; + +static inline void +init_task_work (struct task_work *twork,task_work_func_t func,void *data) +{ + twork->func = func; + twork->data = data; +} + +int task_work_queue (struct task_struct *task,struct task_work *twork,bool ); +struct task_work *task_work_cancel (struct task_struct *,task_work_func_t ); +void task_work_run (struct task_struct *task); + +static inline void exit_task_work (struct task_struct *task) +{ +if (unlikely (!hlist_empty (&task->task_works))) +task_work_run (task); +} + +#endif /* _LINUX_TASK_WORK_H */
include/linux/tracehook.h +11 -3
@@ -46,7 +46,7 @@#ifndef _LINUX_TRACEHOOK_H#define _LINUX_TRACEHOOK_H 1 -#include <linux/sched.h> +#include <linux/task_work.h> #include <linux/ptrace.h> #include <linux/security.h> struct linux_binprm ;
@@ -153,7 +153,6 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,ptrace_notify (SIGTRAP ); } -#ifdef TIF_NOTIFY_RESUME /** * set_notify_resume - cause tracehook_notify_resume() to be called * @task: task that will call tracehook_notify_resume()
@@ -165,8 +164,10 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,*/ static inline void set_notify_resume (struct task_struct *task) { +#ifdef TIF_NOTIFY_RESUME if (!test_and_set_tsk_thread_flag (task,TIF_NOTIFY_RESUME ))kick_process (task); +#endif }/**
@@ -184,7 +185,14 @@ static inline void set_notify_resume(struct task_struct *task)*/ static inline void tracehook_notify_resume (struct pt_regs *regs) { +/* +* The caller just cleared TIF_NOTIFY_RESUME. This barrier +* pairs with task_work_queue()->set_notify_resume() after +* hlist_add_head(task->task_works); +*/ +smp_mb__after_clear_bit (); +if (unlikely (!hlist_empty (¤t->task_works))) +task_work_run (current); } -#endif /* TIF_NOTIFY_RESUME */ #endif /* <linux/tracehook.h> */
kernel/Makefile +1 -1
@@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o printk.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o cred.o \ - async.o range.o groups.o + async.o range.o groups.o task_work.o ifdef CONFIG_FUNCTION_TRACER # Do not trace debug files and internal ftrace files
kernel/exit.c +4 -1
@@ -946,11 +946,14 @@ void do_exit(long code)exit_signals (tsk);/* sets PF_EXITING */ /* * tsk->flags are checked in the futex code to protect against -* an exiting task cleaning up the robust pi futexes. +* an exiting task cleaning up the robust pi futexes, and in +* task_work_queue() to avoid the race with exit_task_work(). */ smp_mb ();raw_spin_unlock_wait (&tsk->pi_lock); +exit_task_work (tsk); +exit_irq_thread ();if (unlikely (in_atomic ()))
kernel/fork.c +1 -0
@@ -1380,6 +1380,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,*/ p->group_leader = p;INIT_LIST_HEAD (&p->thread_group); +INIT_HLIST_HEAD (&p->task_works);/* Now that the task is set up, run cgroup callbacks if * necessary. We need to run them before the task is visible
kernel/task_work.c new file +81 -0
81 lines not on this page · raw message
Signature · 1 line
1.5.5.1
Andrew Morton
maintainerRe: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks
<pays some attention this time>
Quoted 10 lines · On Thu, 12 Apr 2012 23:12:08 +0200 Oleg Nesterov <[email protected]> wrote:
Provide a simple mechanism that allows running code in the (nonatomic) context of the arbitrary task.
The caller does
task_work_queue(task, task_work)and this task executestask_work->func()either fromdo_notify_resume()or fromdo_exit(). The callback can rely onPF_EXITINGto detect the latter case."struct
task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.
So if the task-work was kmalloced, it is the callback function's responsibility to kfree it? task_work_run() appears to handle that OK, without any use-after-free.
Quoted 2 lines
This allows us to kill the ->replacement_session_keyring hack, and potentially this can have more users.
I'm not seeing anything in the changelogs which explains what was wrong with ->replacement_session_keyring, so the motivation for this patchset eludes me.
The immediate reaction is "what's wrong with include/linux/notifier.h". Presumably this has been discussed on some mailing list somewhere ;)
I was wondering if taskstats_exit() could use this. But the call happens from the wrong place. And therein lies a problem with the
proposal: the callback site will be in the wrong place for many potential users. I suspect this new code won't be as useful as we hope
- perhaps we should stick with the current scheme of open-coded callouts to subsystem-specific sites from within do_exit().
include/linux/task_work.h line 4
Quoted 10 lines
...
include/linux/task_work.h new file +4
-0@@ -0,0 +1,32 @@ +#ifndef _LINUX_TASK_WORK_H +#define _LINUX_TASK_WORK_H 1 + +#include <linux/sched.h>
Could just forward-declare struct task_struct, afaict.
This header doesn't include sufficient prerequisite headers, so everything will deservedly explode if you make that change.
include/linux/tracehook.h line 49
Quoted 11 lines
...
include/linux/tracehook.h +1
-1@@ -46,7 +46,7 @@#ifndef _LINUX_TRACEHOOK_H#define _LINUX_TRACEHOOK_H 1 -#include <linux/sched.h> +#include <linux/task_work.h>
ick. So tracehook.h gains a secret dependency upon task_work.h including sched.h.
kernel/task_work.c line 1
Quoted 7 lines
...
kernel/task_work.c new file +1
-0@@ -0,0 +1,81 @@ +#include <linux/tracehook.h>
This file also doesn't come close to including the headers which it requires. This practice is fragile and frequently results in sad little patches months later. Often to fix the alpha build, for unknown reasons.
Having to spell out all the includes is rather a pain. Inevitably some are missed and some remain after they are no longer needed. otoh if we don't do this, things tend to break when unexpected configs are used. There's no nice solution, alas.
kernel/task_work.c line 4
Quoted 2 lines
kernel/task_work.c new file +2
-0+int +task_work_queue (struct task_struct *task,struct task_work *twork,bool notify)
hm, I see. The "queue" in "task_work_queue" is "the action of queueing", not "a queue". Noun-vs-verb. Perhaps task_work_add() would be clearer.
Quoted 4 lines
kernel/task_work.c new file +4
-0+{ +unsigned long flags; +int err = -ESRCH ; +
kernel/task_work.c line 18
Quoted 10 lines
kernel/task_work.c new file +10
-0+#ifndef TIF_NOTIFY_RESUME +if (notify) +return -ENOTSUPP ; +#endif +/* +* We must not insert the new work if the task has already passed +* exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait() +* and check PF_EXITING under pi_lock. +*/ +raw_spin_lock_irqsave (&task->pi_lock, flags);
It's unobvious (to this little reader) why the code uses raw_ locking. The raw_spin_unlock_wait() thing prevents us from using spin_lock_irqsave()? Add a nice comment?
Quoted 16 lines
kernel/task_work.c new file +16
-0+if (likely (!(task->flags &PF_EXITING ))) { +hlist_add_head (&twork->hlist, &task->task_works); + err =0 ; + } +raw_spin_unlock_irqrestore (&task->pi_lock, flags); + +/* test_and_set_bit() implies mb(), see tracehook_notify_resume(). */ +if (likely (!err) && notify) +set_notify_resume (task); +return err; +} + +struct task_work * +task_work_cancel (struct task_struct *task,task_work_func_t func) +{ +unsigned long flags;
kernel/task_work.c line 44
Quoted 10 lines
kernel/task_work.c new file +10
-0+struct task_work *twork; +struct hlist_node *pos; + +raw_spin_lock_irqsave (&task->pi_lock, flags); +hlist_for_each_entry (twork, pos, &task->task_works, hlist) { +if (twork->func == func) { +hlist_del (&twork->hlist); + goto found; + } + }
I trust we won't be giving userspace a way of controlling the queue length!
Quoted 10 lines
kernel/task_work.c new file +10
-0+ twork =NULL ; + found: +raw_spin_unlock_irqrestore (&task->pi_lock, flags); + +return twork; +} + +void task_work_run (struct task_struct *task) +{ +struct hlist_head task_works;
kernel/task_work.c line 64
Quoted 10 lines
kernel/task_work.c new file +10
-0+struct hlist_node *pos; + +raw_spin_lock_irq (&task->pi_lock); +hlist_move_list (&task->task_works, &task_works); +raw_spin_unlock_irq (&task->pi_lock); + +if (unlikely (hlist_empty (&task_works))) +return ; +/* +* We use hlist to save the space in task_struct, but we want fifo.
hm. How does the reader of this code work out why the author wanted fifo? Perhaps because keyctl_session_to_parent() or exit_irq_thread() needed this.
Quoted 17 lines
kernel/task_work.c new file +17
-0+* Find the last entry, the list should be short, then process them +* in reverse order. +*/ +for (pos = task_works.first; pos->next; pos = pos->next) + ; + +for (;;) { +struct hlist_node **pprev = pos->pprev; + struct task_work *twork = container_of(pos, struct task_work, + hlist); + twork->func (twork); + +if (pprev == &task_works.first) +break ; + pos = container_of(pprev, struct hlist_node, next); + } +}
Re: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks
Quoted 2 lines · Andrew Morton
<pays some attention this time>
Thanks!
Quoted 11 lines
Quoted 8 lines · On Thu, 12 Apr 2012 23:12:08 +0200 Oleg Nesterov <[email protected]> wrote:
Provide a simple mechanism that allows running code in the (nonatomic) context of the arbitrary task.
The caller does
task_work_queue(task, task_work)and this task executestask_work->func()either fromdo_notify_resume()or fromdo_exit(). The callback can rely onPF_EXITINGto detect the latter case.
Quoted 5 lines
Quoted 2 lines
"struct
task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.So if the task-work was kmalloced, it is the callback function's responsibility to kfree it?
Yes,
Quoted 2 lines
task_work_run()appears to handle that OK,
without any use-after-free.
I hope ;)
Quoted 6 lines
Quoted 2 lines
This allows us to kill the ->replacement_session_keyring hack, and potentially this can have more users.
I'm not seeing anything in the changelogs which explains what was wrong with ->replacement_session_keyring, so the motivation for this patchset eludes me.
->replacement_session_keyring in fact implements the notifier too, just it is very limited. task_work simply tries to generalize and cleanup the logic we already have. And at the same cost.
Besides, the current code is racy. And it is not convenient to fix all users in arch/.
Quoted 2 lines
The immediate reaction is "what's wrong with
include/linux/notifier.h". Presumably this has been discussed on some mailing list somewhere ;)
Ah, please no. Imho we do not want _notifier_head in task_struct, plus I'd certainly like to avoid the new locks in do_exit()->exit_task_work() path.
Quoted 1 line
I was wondering if
taskstats_exit()could use this.
Unlikely, I think. taskstats is "global", every task should report if there are listeners.
Quoted 6 lines
But the call
happens from the wrong place. And therein lies a problem with the
proposal: the callback site will be in the wrong place for many potential users. I suspect this new code won't be as useful as we hope
- perhaps we should stick with the current scheme of open-coded callouts to subsystem-specific sites from withindo_exit().
May be we can add more task_work_run's later, but right now I do not think this is needed. And I do not expect it will find a lot of new users.
Quoted 11 lines
include/linux/tracehook.h line 49
Quoted 8 lines
include/linux/tracehook.h +1
-1@@ -46,7 +46,7 @@#ifndef _LINUX_TRACEHOOK_H#define _LINUX_TRACEHOOK_H 1 -#include <linux/sched.h> +#include <linux/task_work.h> ick. So
tracehook.hgains a secret dependency upontask_work.hincludingsched.h.
Hmm. I thought that it is alway fine to remove the unnecessary include. I can put it back, but we are going to kill tracehook.h altogether.
Quoted 9 lines
kernel/task_work.c line 1
Quoted 4 lines
kernel/task_work.c new file +1
-0@@ -0,0 +1,81 @@ +#include <linux/tracehook.h> This file also doesn't come close to including the headers which it requires. This practice is fragile and frequently results in sad little patches months later. Often to fix the alpha build, for unknown reasons.
OK, I'll add "include linix/task_work.h".
Quoted 5 lines
kernel/task_work.c line 4
Quoted 2 lines
kernel/task_work.c new file +2
-0+int +task_work_queue (struct task_struct *task,struct task_work *twork,bool notify)hm, I see. The "queue" in "
task_work_queue" is "the action of queueing", not "a queue". Noun-vs-verb.
Cough. Peter, do you read this? I stole the naming and the first line of the changelog from kernel/irq_work.c!
Quoted 2 lines
Perhaps
task_work_add()would
be clearer.
OK, I'll rename.
Quoted 9 lines
Quoted 9 lines
kernel/task_work.c new file +9
-0+unsigned long flags; +int err = -ESRCH ; + +#ifndef TIF_NOTIFY_RESUME +if (notify) +return -ENOTSUPP ; +#endif +/* +* We must not insert the new work if the task has already passed
Quoted 8 lines
kernel/task_work.c line 18
Quoted 4 lines
kernel/task_work.c new file +4
-0+* exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait() +* and check PF_EXITING under pi_lock. +*/ +raw_spin_lock_irqsave (&task->pi_lock, flags);It's unobvious (to this little reader) why the code uses
raw_locking. Theraw_spin_unlock_wait()thing prevents us from usingspin_lock_irqsave()? Add a nice comment?
Well, but ->pi_lock is raw_spinlock_t ?
Quoted 9 lines
Quoted 9 lines
kernel/task_work.c new file +9
-0+struct task_work * +task_work_cancel (struct task_struct *task,task_work_func_t func) +{ +unsigned long flags; +struct task_work *twork; +struct hlist_node *pos; + +raw_spin_lock_irqsave (&task->pi_lock, flags); +hlist_for_each_entry (twork, pos, &task->task_works, hlist) {
Quoted 8 lines
kernel/task_work.c line 44
Quoted 5 lines
kernel/task_work.c new file +5
-0+if (twork->func == func) { +hlist_del (&twork->hlist); + goto found; + } + }I trust we won't be giving userspace a way of controlling the queue length!
Exactly. From 2/3:
Note that we do task_work_cancel() before task_work_queue() to ensure that only one work can be pending at any time. This is important, we must not allow user-space to abuse the parent's ->task_works list.
Quoted 8 lines
Quoted 8 lines
kernel/task_work.c new file +8
-0+void task_work_run (struct task_struct *task) +{ +struct hlist_head task_works; +struct hlist_node *pos; + +raw_spin_lock_irq (&task->pi_lock); +hlist_move_list (&task->task_works, &task_works); +raw_spin_unlock_irq (&task->pi_lock);
Quoted 8 lines
kernel/task_work.c line 64
Quoted 5 lines
kernel/task_work.c new file +5
-0+ +if (unlikely (hlist_empty (&task_works))) +return ; +/* +* We use hlist to save the space in task_struct, but we want fifo. hm. How does the reader of this code work out why the author wanted fifo? Perhaps because
keyctl_session_to_parent()orexit_irq_thread()needed this.
Argh.
No, keyctl_session_to_parent() and exit_irq_thread() do not care, they can't add more than one work.
Just I think that fifo makes more sense in general. Just for example, suppose that keyctl_session_to_parent() didn't cancel the pending request(s). In this case fifo would be needed for correctness.
Oleg.
Peter Zijlstra
maintainerRe: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks
Quoted 5 lines · Oleg Nesterov
Quoted 2 lines
hm, I see. The "queue" in "
task_work_queue" is "the action of queueing", not "a queue". Noun-vs-verb.Cough. Peter, do you read this? I stole the naming and the first line of the changelog from
kernel/irq_work.c!
Am now.. right, so that was modeled after queue_work(), although I now realize I reversed the words and made it irq_work_queue() instead of queue_irq_work().
I'd prefer to keep the queue thing as per workqueue and irq_work etc..
PATCH v3 2/3 cred: change keyctl_session_to_parent() to use task_work_queue()
In reply to Oleg NesterovChange keyctl_session_to_parent() to use task_work_queue() and move key_replace_session_keyring() logic into task_work->func().
Note that we do task_work_cancel() before task_work_queue() to ensure that only one work can be pending at any time. This is important, we must not allow user-space to abuse the parent's ->task_works list.
The callback, replace_session_keyring(), checks PF_EXITING. I guess this is not really needed but looks better.
As a side effect, this fixes the (unlikely) race. The callers of key_replace_session_keyring() and keyctl_session_to_parent() lack the necessary barriers, the parent can miss the request.
Now we can remove task_struct->replacement_session_keyring and related code.
--- include/linux/key.h | 6 +-- security/keys/internal.h | 3 +- security/keys/keyctl.c | 76 ++++++++++++++++++++---------------------- security/keys/process_keys.c | 25 ++++---------- 4 files changed, 47 insertions(+), 63 deletions(-)
include/linux/key.h +2 -4
@@ -33,6 +33,8 @@ typedef uint32_t key_perm_t;struct key ; +#define key_replace_session_keyring () do { } while (0) +#ifdef CONFIG_KEYS #undef KEY_DEBUGGING
@@ -302,9 +304,6 @@ static inline bool key_is_instantiated(const struct key *key)#ifdef CONFIG_SYSCTL extern ctl_table key_sysctls[];#endif - -extern void key_replace_session_keyring (void ); -/* * the userspace interface */
@@ -327,7 +326,6 @@ extern void key_init(void);#define key_fsuid_changed (t) do { } while(0)#define key_fsgid_changed (t) do { } while(0)#define key_init () do { } while(0) -#define key_replace_session_keyring () do { } while(0)#endif /* CONFIG_KEYS */ #endif /* __KERNEL__ */
security/keys/internal.h +2 -1
@@ -12,7 +12,7 @@#ifndef _INTERNAL_H#define _INTERNAL_H -#include <linux/sched.h> +#include <linux/task_work.h> #include <linux/key-type.h> #ifdef __KDEBUG
@@ -148,6 +148,7 @@ extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,#define KEY_LOOKUP_FOR_UNLINK 0x04extern long join_session_keyring (const char *name); +extern void key_change_session_keyring (struct task_work *twork);extern struct work_struct key_gc_work;extern unsigned key_gc_delay;
security/keys/keyctl.c +36 -40
136 lines not on this page · raw message
security/keys/process_keys.c +7 -18
@@ -828,27 +828,17 @@ error:return ret; } -/* -* Replace a process's session keyring on behalf of one of its children when -* the target process is about to resume userspace execution. -*/ -void key_replace_session_keyring (void ) +void key_change_session_keyring (struct task_work *twork) { -const struct cred *old; -struct cred *new; +const struct cred *old =current_cred (); +struct cred *new = twork->data; -if (!current->replacement_session_keyring) -return ; - -write_lock_irq (&tasklist_lock); - new = current->replacement_session_keyring; - current->replacement_session_keyring =NULL ; -write_unlock_irq (&tasklist_lock); - -if (!new) +kfree (twork); +if (unlikely (current->flags &PF_EXITING )) { +put_cred (new);return ; + } - old =current_cred (); new-> uid = old-> uid; new-> euid = old-> euid; new-> suid = old-> suid;
@@ -873,6 +863,5 @@ void key_replace_session_keyring(void) new->tgcred->process_keyring =key_get (old->tgcred->process_keyring);security_transfer_creds (new, old); -commit_creds (new); }
Signature · 1 line
1.5.5.1
PATCH v3 3/3 genirq: reimplement exit_irq_thread() hook via task_work_queue()
In reply to Oleg Nesterovexit_irq_thread() and task->irq_thread are needed to handle the unexpected (and unlikely) exit of irq-thread.
We can use task_work instead and make this all private to kernel/irq/manage.c, simplification plus micro-optimization.
1. rename exit_irq_thread() to irq_thread_dtor(), make it static, and move it up before irq_thread().
2. change irq_thread() to do task_queue_work(irq_thread_dtor) at the start and task_work_cancel() before return.
tracehook_notify_resume() can never play with kthreads, only do_exit()->exit_task_work() can call the callback and this is what we want.
3. remove task_struct->irq_thread and the special hook in do_exit().
--- include/linux/interrupt.h | 4 -- include/linux/sched.h | 10 +----- kernel/exit.c | 2 - kernel/irq/manage.c | 69 +++++++++++++++++++++----------------------- 4 files changed, 35 insertions(+), 50 deletions(-)
include/linux/interrupt.h +0 -4
@@ -142,8 +142,6 @@ request_any_context_irq(unsigned int irq, irq_handler_t handler,extern int __must_check request_percpu_irq (unsigned int irq,irq_handler_t handler,const char *devname,void __percpu *percpu_dev_id); - -extern void exit_irq_thread (void );#else extern int __must_check
@@ -177,8 +175,6 @@ request_percpu_irq(unsigned int irq, irq_handler_t handler, {return request_irq (irq, handler,0 , devname, percpu_dev_id); } - -static inline void exit_irq_thread (void ) { }#endif extern void free_irq (unsigned int ,void *);
include/linux/sched.h +2 -8
@@ -1346,11 +1346,6 @@ struct task_struct { unsigned sched_reset_on_fork:1; unsigned sched_contributes_to_load:1; -#ifdef CONFIG_GENERIC_HARDIRQS -/* IRQ handler threads */ - unsigned irq_thread:1; -#endif -pid_t pid;pid_t tgid;
@@ -1358,10 +1353,9 @@ struct task_struct {/* Canary value for the -fstack-protector gcc feature */ unsigned long stack_canary;#endif - -/* +/* * pointers to (original) parent process, youngest child, younger sibling, -* older sibling, respectively. (p->father can be replaced with +* older sibling, respectively. (p->father can be replaced with * p->real_parent->pid) */ struct task_struct __rcu *real_parent;/* real parent process */
kernel/exit.c +0 -2
@@ -954,8 +954,6 @@ void do_exit(long code)exit_task_work (tsk); -exit_irq_thread (); -if (unlikely (in_atomic ())) printk(KERN_INFO "note: %s[%d] exited with preempt_count %d\n", current->comm, task_pid_nr(current),
kernel/irq/manage.c +33 -36
102 lines not on this page · raw message
Signature · 1 line
1.5.5.1
Re: [PATCH v3 3/3] genirq: reimplement exit_irq_thread() hook via task_work_queue()
Quoted 19 lines · Oleg Nesterov
exit_irq_thread()andtask->irq_threadare needed to handle the unexpected (and unlikely) exit of irq-thread.We can use
task_workinstead and make this all private tokernel/irq/manage.c, simplification plus micro-optimization.1. rename
exit_irq_thread()toirq_thread_dtor(), make it static, and move it up beforeirq_thread().2. change
irq_thread()to dotask_queue_work(irq_thread_dtor)at the start andtask_work_cancel()before return.
tracehook_notify_resume()can never play with kthreads, only do_exit()->exit_task_work() can call the callback and this is what we want.3. remove
task_struct->irq_threadand the special hook indo_exit().
Quoted 7 lines
Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/interrupt.h | 4 -- include/linux/sched.h | 10 +----- kernel/exit.c | 2 - kernel/irq/manage.c | 69 +++++++++++++++++++++----------------------- 4 files changed, 35 insertions(+), 50 deletions(-)
Nice cleanup.