Re: [ISSUE] The time cost of IOSQE_IO_LINK

[Oleg Nesterov <oleg@redhat.com>]

On 02/18, Peter Zijlstra wrote:
>
> But this has me wondering about task_work_run(), as it is it will
> unconditionally take pi_lock,

because spin_unlock_wait() was removed ;) task_work_run() doesn't
really need to take pi_lock at all. 

> would not something like this make sense?

I think yes, but see below.

> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -93,16 +93,20 @@ void task_work_run(void)
>  	struct callback_head *work, *head, *next;
>
>  	for (;;) {
> +		work = READ_ONCE(task->task_work);
> +		if (!work)
> +			break

This is wrong if PF_EXITING is set, in this case we must set
task->task_works = work_exited.

> +
>  		/*
>  		 * work->func() can do task_work_add(), do not set
>  		 * work_exited unless the list is empty.
>  		 */
>  		raw_spin_lock_irq(&task->pi_lock);
>  		do {
> -			work = READ_ONCE(task->task_works);
> -			head = !work && (task->flags & PF_EXITING) ?
> -				&work_exited : NULL;
> -		} while (cmpxchg(&task->task_works, work, head) != work);
> +			head = NULL;
> +			if (unlikely(!work && (task->flags & PF_EXITING)))
> +				head = &work_exited;
> +		} while (!try_cmpxchg(&task->task_works, &work, head));
>  		raw_spin_unlock_irq(&task->pi_lock);
>
>  		if (!work)

otherwise I think this is correct, but how about the patch below?
Then this code can be changed to use try_cmpxchg().

Oleg.

--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -97,17 +97,24 @@ void task_work_run(void)
 		 * work->func() can do task_work_add(), do not set
 		 * work_exited unless the list is empty.
 		 */
-		raw_spin_lock_irq(&task->pi_lock);
 		do {
+			head = NULL;
 			work = READ_ONCE(task->task_works);
-			head = !work && (task->flags & PF_EXITING) ?
-				&work_exited : NULL;
+			if (!work) {
+				if (task->flags & PF_EXITING)
+					head = &work_exited;
+				else
+					break;
+			}
 		} while (cmpxchg(&task->task_works, work, head) != work);
-		raw_spin_unlock_irq(&task->pi_lock);
 
 		if (!work)
 			break;
 
+		// Synchronize with task_work_cancel()
+		raw_spin_lock_irq(&task->pi_lock);
+		raw_spin_unlock_irq(&task->pi_lock);
+
 		do {
 			next = work->next;
 			work->func(work);