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

Oleg Nesterov
2020-02-20T17:22:32Z

On 02/20, Peter Zijlstra wrote:
>
> Still playing with my try_cmpxchg() patches, how does the below look on
> top?

I too made a simple patch right after you sent "[PATCH] asm-generic/atomic:
Add try_cmpxchg() fallbacks", see below.

But I am fine with your version, with one exception

> 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;

You can't remove the "if (!work)" check, cancel_task_work() can remove
a single callback between READ_ONCE() and xchg().

Oleg.

--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -27,14 +27,11 @@ static struct callback_head work_exited; /* all we need is ->next == NULL */
int
task_work_add(struct task_struct *task, struct callback_head *work, bool notify)
{
- struct callback_head *head;
-
+ work->next = READ_ONCE(task->task_works);
do {
- head = READ_ONCE(task->task_works);
- if (unlikely(head == &work_exited))
+ if (unlikely(work->next == &work_exited))
return -ESRCH;
- work->next = head;
- } while (cmpxchg(&task->task_works, head, work) != head);
+ } while (!try_cmpxchg(&task->task_works, &work->next, work));

if (notify)
set_notify_resume(task);
@@ -68,10 +65,10 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
* we raced with task_work_run(), *pprev == NULL/exited.
*/
raw_spin_lock_irqsave(&task->pi_lock, flags);
- while ((work = READ_ONCE(*pprev))) {
+ for (work = READ_ONCE(*pprev); work; ) {
if (work->func != func)
pprev = &work->next;
- else if (cmpxchg(pprev, work, work->next) == work)
+ else if (try_cmpxchg(pprev, &work, work->next))
break;
}
raw_spin_unlock_irqrestore(&task->pi_lock, flags);
@@ -97,16 +94,16 @@ void task_work_run(void)
* work->func() can do task_work_add(), do not set
* work_exited unless the list is empty.
*/
+ work = READ_ONCE(task->task_works);
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);
+ } while (!try_cmpxchg(&task->task_works, &work, head));

if (!work)
break;