* [PATCH] task_work: avoid unneeded cmpxchg() in task_work_run()
@ 2012-10-08 9:18 Lai Jiangshan
2012-10-08 12:38 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2012-10-08 9:18 UTC (permalink / raw)
To: LKML, Peter Zijlstra, Oleg Nesterov, Al Viro, Ingo Molnar,
Eric Dumazet
We only require cmpxchg()&retry when task is exiting.
xchg() is enough in other cases like original code in ac3d0da8.
So we try our best to use xchg() and avoid competition&latency
from task_work_add().
Also remove the inner loop
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/task_work.c b/kernel/task_work.c
index 65bd3c9..82a42e7 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -56,14 +56,13 @@ void task_work_run(void)
* work->func() can do task_work_add(), do not set
* work_exited unless the list is empty.
*/
- do {
- work = ACCESS_ONCE(task->task_works);
- head = !work && (task->flags & PF_EXITING) ?
- &work_exited : NULL;
- } while (cmpxchg(&task->task_works, work, head) != work);
-
- if (!work)
+ if (!ACCESS_ONCE(task->task_works) ||
+ !(work = xchg(&task->task_works, NULL))) {
+ if ((task->flags & PF_EXITING) &&
+ cmpxchg(&task->task_works, NULL, &work_exited))
+ continue;
break;
+ }
/*
* Synchronize with task_work_cancel(). It can't remove
* the first entry == work, cmpxchg(task_works) should
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] task_work: avoid unneeded cmpxchg() in task_work_run()
2012-10-08 9:18 [PATCH] task_work: avoid unneeded cmpxchg() in task_work_run() Lai Jiangshan
@ 2012-10-08 12:38 ` Oleg Nesterov
2012-10-09 11:04 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2012-10-08 12:38 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: LKML, Peter Zijlstra, Al Viro, Ingo Molnar, Eric Dumazet
On 10/08, Lai Jiangshan wrote:
>
> We only require cmpxchg()&retry when task is exiting.
> xchg() is enough in other cases like original code in ac3d0da8.
Yes, we can probably do xchg/cmpxchg depending on NULL/work_exited.
Not sure it makes sense to complicate the code though. Is xchg()
really faster than cmpxchg?
> Also remove the inner loop
Yes, it is not really needed, only for readability.
"do while (!cmpxchg)" can be replaced with "if (!cmpxchg) continue".
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -56,14 +56,13 @@ void task_work_run(void)
> * work->func() can do task_work_add(), do not set
> * work_exited unless the list is empty.
> */
> - do {
> - work = ACCESS_ONCE(task->task_works);
> - head = !work && (task->flags & PF_EXITING) ?
> - &work_exited : NULL;
> - } while (cmpxchg(&task->task_works, work, head) != work);
> -
> - if (!work)
> + if (!ACCESS_ONCE(task->task_works) ||
ACCESS_ONCE() looks confusing. It is not needed with this patch.
> + !(work = xchg(&task->task_works, NULL))) {
> + if ((task->flags & PF_EXITING) &&
> + cmpxchg(&task->task_works, NULL, &work_exited))
> + continue;
> break;
> + }
I think the patch is correct.
But the code looks more complex, and the only advantage is that
non-exiting task does xchg() instead of cmpxchg(). Not sure this
worth the trouble, in this case task_work_run() will likey run
the callbacks (the caller checks ->task_works != NULL), I do not
think this can add any noticeable speedup.
But, as for correctness,
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] task_work: avoid unneeded cmpxchg() in task_work_run()
2012-10-08 12:38 ` Oleg Nesterov
@ 2012-10-09 11:04 ` Peter Zijlstra
2012-10-10 5:37 ` [PATCH V2] " Lai Jiangshan
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2012-10-09 11:04 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Lai Jiangshan, LKML, Al Viro, Ingo Molnar, Eric Dumazet
On Mon, 2012-10-08 at 14:38 +0200, Oleg Nesterov wrote:
> But the code looks more complex, and the only advantage is that
> non-exiting task does xchg() instead of cmpxchg(). Not sure this
> worth the trouble, in this case task_work_run() will likey run
> the callbacks (the caller checks ->task_works != NULL), I do not
> think this can add any noticeable speedup.
Yeah, I agree, the patch doesn't seem worth the trouble. It makes tricky
code unreadable at best.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] task_work: avoid unneeded cmpxchg() in task_work_run()
2012-10-09 11:04 ` Peter Zijlstra
@ 2012-10-10 5:37 ` Lai Jiangshan
2012-10-10 17:50 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2012-10-10 5:37 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Oleg Nesterov, LKML, Al Viro, Ingo Molnar, Eric Dumazet
On 10/09/2012 07:04 PM, Peter Zijlstra wrote:
> On Mon, 2012-10-08 at 14:38 +0200, Oleg Nesterov wrote:
>> But the code looks more complex, and the only advantage is that
>> non-exiting task does xchg() instead of cmpxchg(). Not sure this
>> worth the trouble, in this case task_work_run() will likey run
>> the callbacks (the caller checks ->task_works != NULL), I do not
>> think this can add any noticeable speedup.
>
> Yeah, I agree, the patch doesn't seem worth the trouble. It makes tricky
> code unreadable at best.
>
To gain better readability, we need to move work_exited things out
from task_work_run() too.
Thanks,
Lai
Subject: task_work: avoid unneeded cmpxchg() in task_work_run()
We only require cmpxchg()&retry when task is exiting.
xchg() is enough in other cases like original code in ac3d0da8.
So we use xchg() for task_work_run() and move the logic
of exit_task_work() out from task_work_run().
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/include/linux/task_work.h b/include/linux/task_work.h
index ca5a1cf..1e686a5 100644
--- a/include/linux/task_work.h
+++ b/include/linux/task_work.h
@@ -15,10 +15,6 @@ init_task_work(struct callback_head *twork, task_work_func_t func)
int task_work_add(struct task_struct *task, struct callback_head *twork, bool);
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 65bd3c9..87ef3b7 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -52,16 +52,7 @@ void task_work_run(void)
struct callback_head *work, *head, *next;
for (;;) {
- /*
- * work->func() can do task_work_add(), do not set
- * work_exited unless the list is empty.
- */
- do {
- work = ACCESS_ONCE(task->task_works);
- head = !work && (task->flags & PF_EXITING) ?
- &work_exited : NULL;
- } while (cmpxchg(&task->task_works, work, head) != work);
-
+ work = xchg(&task->task_works, NULL);
if (!work)
break;
/*
@@ -90,3 +81,17 @@ void task_work_run(void)
} while (work);
}
}
+
+void exit_task_work(struct task_struct *task)
+{
+ for (;;) {
+ /*
+ * work->func() can do task_work_add(), do not set
+ * work_exited unless the list is empty.
+ */
+ if (unlikely(task->task_works))
+ task_work_run();
+ if (cmpxchg(&task->task_works, NULL, &work_exited) == NULL)
+ break;
+ }
+}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] task_work: avoid unneeded cmpxchg() in task_work_run()
2012-10-10 5:37 ` [PATCH V2] " Lai Jiangshan
@ 2012-10-10 17:50 ` Oleg Nesterov
2012-10-10 17:50 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2012-10-10 17:50 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Peter Zijlstra, LKML, Al Viro, Ingo Molnar, Eric Dumazet
On 10/10, Lai Jiangshan wrote:
>
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -52,16 +52,7 @@ void task_work_run(void)
> struct callback_head *work, *head, *next;
>
> for (;;) {
> - /*
> - * work->func() can do task_work_add(), do not set
> - * work_exited unless the list is empty.
> - */
> - do {
> - work = ACCESS_ONCE(task->task_works);
> - head = !work && (task->flags & PF_EXITING) ?
> - &work_exited : NULL;
> - } while (cmpxchg(&task->task_works, work, head) != work);
> -
> + work = xchg(&task->task_works, NULL);
> if (!work)
> break;
> /*
> @@ -90,3 +81,17 @@ void task_work_run(void)
> } while (work);
> }
> }
> +
> +void exit_task_work(struct task_struct *task)
> +{
> + for (;;) {
> + /*
> + * work->func() can do task_work_add(), do not set
> + * work_exited unless the list is empty.
> + */
> + if (unlikely(task->task_works))
> + task_work_run();
> + if (cmpxchg(&task->task_works, NULL, &work_exited) == NULL)
> + break;
> + }
> +}
I agree, this looks fine.
But if you add "unlikely" before task_work_run(), then probably
it should do
while (cmpxchg(&task->task_works, NULL, work_exited))
task_work_run();
? it looks more simple/clean.
(OTOH I am not sure "unlikely" is true, note that exit_files() will
offload ____fput() to task_work_run()).
But you did not answer, and I am curious. What was your original
motivation? Is xchg really faster than cmpxchg?
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] task_work: avoid unneeded cmpxchg() in task_work_run()
2012-10-10 17:50 ` Oleg Nesterov
@ 2012-10-10 17:50 ` Peter Zijlstra
0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2012-10-10 17:50 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Lai Jiangshan, LKML, Al Viro, Ingo Molnar, Eric Dumazet
On Wed, 2012-10-10 at 19:50 +0200, Oleg Nesterov wrote:
>
> But you did not answer, and I am curious. What was your original
> motivation? Is xchg really faster than cmpxchg?
And is this true over multiple architectures? Or are we optimizing for
x86_64 (again) ?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-10 17:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-08 9:18 [PATCH] task_work: avoid unneeded cmpxchg() in task_work_run() Lai Jiangshan
2012-10-08 12:38 ` Oleg Nesterov
2012-10-09 11:04 ` Peter Zijlstra
2012-10-10 5:37 ` [PATCH V2] " Lai Jiangshan
2012-10-10 17:50 ` Oleg Nesterov
2012-10-10 17:50 ` Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox