On Tue, Feb 18, 2020 at 04:50:18PM +0100, Oleg Nesterov wrote:
> As Peter pointed out, task_work() can avoid ->pi_lock and cmpxchg()
> if task->task_works == NULL && !PF_EXITING.
>
> And in fact the only reason why task_work_run() needs ->pi_lock is
> the possible race with task_work_cancel(), we can optimize this code
> and make the locking more clear.
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
Still playing with my try_cmpxchg() patches, how does the below look on
top?
---
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -27,14 +27,13 @@ static struct callback_head work_exited;
int
task_work_add(struct task_struct *task, struct callback_head *work, bool notify)
{
- struct callback_head *head;
+ struct callback_head *head = READ_ONCE(tsk->task_works);
do {
- head = READ_ONCE(task->task_works);
if (unlikely(head == &work_exited))
return -ESRCH;
work->next = head;
- } while (cmpxchg(&task->task_works, head, work) != head);
+ } while (!try_cmpxchg(&task->task_works, &head, work))
if (notify)
set_notify_resume(task);
@@ -90,26 +89,24 @@ task_work_cancel(struct task_struct *tas
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);
+ work = READ_ONCE(task->task_works);
+ if (!work) {
+ if (!(task->flags & PF_EXITING))
+ return;
+
+ /*
+ * work->func() can do task_work_add(), do not set
+ * work_exited unless the list is empty.
+ */
+ if (try_cmpxchg(&task->task_works, &work, &work_exited))
+ return;
+ }
+
+ work = xchg(&task->task_works, NULL);
- if (!work)
- break;
/*
* Synchronize with task_work_cancel(). It can not remove
* the first entry == work, cmpxchg(task_works) must fail.
