Re: [PATCH] task_work_run: don't take ->pi_lock unconditionally

[Peter Zijlstra <peterz@infradead.org>]

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@redhat.com>
> ---

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.