Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221008 > unrolled thread

[PATCH 3/3] Revert "task_work: remove fifo ordering guarantee"

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Oleg Nesterov <oleg@redhat.com> - 2015-09-08 19:20 +0200
    Re: [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-08 19:40 +0200
      Re: [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-08 19:50 +0200
      Re: [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Oleg Nesterov <oleg@redhat.com> - 2015-09-09 15:20 +0200
        Re: [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-09 18:20 +0200
          Re: [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee" Oleg Nesterov <oleg@redhat.com> - 2015-09-09 18:50 +0200

#1221008 — [PATCH 3/3] Revert "task_work: remove fifo ordering guarantee"

This reverts commit c82199061009d1561e31e17fca5e47a87cb7ff4c.

Now that fput() can't abuse ->task_works list, we can restore the FIFO
ordering. Yes, currently there are no in-kernel users which need this,
but I think task_work_add() will have more users and FIFO makes more
sense if (unlike fput/mntput) the callbacks change the task's state.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/task_work.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/task_work.c b/kernel/task_work.c
index 53fa971..8727032 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -18,8 +18,6 @@ static struct callback_head work_exited; /* all we need is ->next == NULL */
  * This is like the signal handler which runs in kernel mode, but it doesn't
  * try to wake up the @task.
  *
- * Note: there is no ordering guarantee on works queued here.
- *
  * RETURNS:
  * 0 if succeeds or -ESRCH.
  */
@@ -110,6 +108,16 @@ void task_work_run(void)
 		raw_spin_unlock_wait(&task->pi_lock);
 		smp_mb();
 
+		/* Reverse the list to run the works in fifo order */
+		head = NULL;
+		do {
+			next = work->next;
+			work->next = head;
+			head = work;
+			work = next;
+		} while (work);
+
+		work = head;
 		do {
 			next = work->next;
 			work->func(work);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221016

On Tue, Sep 8, 2015 at 10:14 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Now that fput() can't abuse ->task_works list, we can restore the FIFO
> ordering. Yes, currently there are no in-kernel users which need this,
> but I think task_work_add() will have more users and FIFO makes more
> sense if (unlike fput/mntput) the callbacks change the task's state.

So quite frankly, regardless of the other patches, I'd almost rather
see the workqueue not being ordered. I don't think anybody pointed at
any code that could possibly care. And if nobody cares, why add the
code and the CPU cycles to do this?

The other patches I do like - why add those list operations that are
just guaranteed to mess up the cache to add the file descriptors to
the workqueue list, when we can just do the operation directly? So I
think that's a separate issue.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221017

On Tue, Sep 8, 2015 at 10:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The other patches I do like - why add those list operations that are
> just guaranteed to mess up the cache to add the file descriptors to
> the workqueue list, when we can just do the operation directly?

Side note, it's not just messing with the cache unnecessarily when the
lists grow long, the "cmpxchg" involved in adding the work is fairly
expensive. It may be an unlocked operation, but it's generally as
expensive as a locked one - just without the possibility of a
deadlock.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221421

sorry for delay,

On 09/08, Linus Torvalds wrote:
>
> On Tue, Sep 8, 2015 at 10:14 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Now that fput() can't abuse ->task_works list, we can restore the FIFO
> > ordering. Yes, currently there are no in-kernel users which need this,
> > but I think task_work_add() will have more users and FIFO makes more
> > sense if (unlike fput/mntput) the callbacks change the task's state.
>
> So quite frankly, regardless of the other patches, I'd almost rather
> see the workqueue not being ordered. I don't think anybody pointed at
> any code that could possibly care. And if nobody cares, why add the
> code and the CPU cycles to do this?

Currently nobody cares, yes. IIRC, even the out-of-tree code I know about,
although I didn't recheck.

Again, rightly or not I believe that FIFO makes task_work_add() more useful.
Perhaps I am wrong, so far I can only provide the artificial examples...

To me this does not differ from, say, stop_one_cpu_nowait(). I would be
surprised if it wasn't FIFO.

At least this should be cheap after 1/3. And in any case the time we spend
in the "reverse" loop is nothing compared to the next one which actually
runs the callbacks.

Thanks. Lets see what Al thinks...

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221549

On Wed, Sep 9, 2015 at 6:16 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Again, rightly or not I believe that FIFO makes task_work_add() more useful.
> Perhaps I am wrong, so far I can only provide the artificial examples...

I'd rather wait until somebody has a real use case. I hate adding
infrastructure for "what if.." scenarios. We're better off if we can
make do with minimal semantics (ie "there are no guarantees except
that the work will be done before returning to user space") than with
stronger semantics that people then perhaps start depending on even if
they didn't really need them.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221560

On 09/09, Linus Torvalds wrote:
>
> On Wed, Sep 9, 2015 at 6:16 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Again, rightly or not I believe that FIFO makes task_work_add() more useful.
> > Perhaps I am wrong, so far I can only provide the artificial examples...
>
> I'd rather wait until somebody has a real use case. I hate adding
> infrastructure for "what if.." scenarios. We're better off if we can
> make do with minimal semantics (ie "there are no guarantees except
> that the work will be done before returning to user space") than with
> stronger semantics that people then perhaps start depending on even if
> they didn't really need them.

OK, I see. Thanks.

At least you seem to agree with 1-2, so if Al takes these changes we
can easily reconsider 3/3 later, if/when we have the new user which
needs FIFO.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web