On 08/12/2020 01:37, Al Viro wrote:
> On Thu, Dec 03, 2020 at 02:30:46AM +0000, Pavel Begunkov wrote:
>> Handle task works and lock it earlier before it starts killing off
>> task's resources like mm. io_uring makes use of it a lot and it'd
>> nicer to have all added task_work finding tasks in a consistent state.
>>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>> ---
>>
>> Would it be correct? I clearly don't know all the exit invariants, but
>> can't find any users relying on task_works in-between.
>
> You've just gotten rid of exit_task_work() anywhere after exit_files().
> And exit_mm() can trigger the final fput() just as easily as exit_files().
>
> IOW, you have just made the effect of final close on exit() completely
> asynchronous.
One more moment, after we've set PF_EXITING any task_work_run() would be
equivalent to exit_task_work()
static inline void exit_task_work(struct task_struct *task)
{
task_work_run();
}
It sounds from your words that this is not expected, is it? io_uring
may want (currently doesn't) to run works for cancellation purposes.
Shouldn't it be like below (not tested)? Also simplifies task_work_run().
CC Oleg
diff --git a/include/linux/task_work.h b/include/linux/task_work.h
index 0d848a1e9e62..6aae0e6c3a04 100644
--- a/include/linux/task_work.h
+++ b/include/linux/task_work.h
@@ -24,10 +24,6 @@ int task_work_add(struct task_struct *task, struct
callback_head *twork,
struct callback_head *task_work_cancel(struct task_struct *, task_work_func_t);
void task_work_run(void);
-
-static inline void exit_task_work(struct task_struct *task)
-{
- task_work_run();
-}
+void exit_task_work(struct task_struct *task);
#endif /* _LINUX_TASK_WORK_H */
diff --git a/kernel/task_work.c b/kernel/task_work.c
index 9cde961875c0..60715f3d91a0 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -107,26 +107,15 @@ task_work_cancel(struct task_struct *task,
task_work_func_t func)
void task_work_run(void)
{
struct task_struct *task = current;
- struct callback_head *work, *head, *next;
+ struct callback_head *work, *next;
for (;;) {
- /*
- * work->func() can do task_work_add(), do not set
- * work_exited unless the list is empty.
- */
do {
- head = NULL;
work = READ_ONCE(task->task_works);
- if (!work) {
- if (task->flags & PF_EXITING)
- head = &work_exited;
- else
- break;
- }
- } while (cmpxchg(&task->task_works, work, head) != work);
+ if (!work)
+ return;
+ } while (cmpxchg(&task->task_works, work, NULL) != work);
- if (!work)
- break;
/*
* Synchronize with task_work_cancel(). It can not remove
* the first entry == work, cmpxchg(task_works) must fail.
@@ -143,3 +132,17 @@ void task_work_run(void)
} while (work);
}
}
+
+void exit_task_work(struct task_struct *task)
+{
+ WARN_ON(task != current);
+ WARN_ON(!(task->flags & PF_EXITING));
+ WARN_ON(READ_ONCE(task->task_works) == &work_exited);
+
+ /*
+ * work->func() can do task_work_add(), do not set
+ * work_exited unless the list is empty.
+ */
+ while (cmpxchg(&task->task_works, NULL, &work_exited) != NULL)
+ task_work_run();
+}
--
Pavel Begunkov
