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

Groups > linux.kernel > #1215658 > unrolled thread

[PATCH] task_work: remove fifo ordering guarantee

Back to article view | Back to linux.kernel

Contents

  [PATCH] task_work: remove fifo ordering guarantee Eric Dumazet <eric.dumazet@gmail.com> - 2015-08-29 04:50 +0200
    Re: [PATCH] task_work: remove fifo ordering guarantee Linus Torvalds <torvalds@linux-foundation.org> - 2015-08-29 05:20 +0200
      Re: [PATCH] task_work: remove fifo ordering guarantee Ingo Molnar <mingo@kernel.org> - 2015-08-29 11:30 +0200
        Re: [PATCH] task_work: remove fifo ordering guarantee Oleg Nesterov <oleg@redhat.com> - 2015-08-29 15:00 +0200
          Re: [PATCH] task_work: remove fifo ordering guarantee Ingo Molnar <mingo@kernel.org> - 2015-08-31 08:10 +0200
            Re: [PATCH] task_work: remove fifo ordering guarantee Oleg Nesterov <oleg@redhat.com> - 2015-08-31 15:00 +0200
    Re: [PATCH] task_work: remove fifo ordering guarantee Oleg Nesterov <oleg@redhat.com> - 2015-08-29 15:00 +0200
      Re: [PATCH] task_work: remove fifo ordering guarantee Eric Dumazet <eric.dumazet@gmail.com> - 2015-08-29 16:00 +0200
        Re: [PATCH] task_work: remove fifo ordering guarantee Eric Dumazet <eric.dumazet@gmail.com> - 2015-08-29 16:20 +0200
          Re: [PATCH] task_work: remove fifo ordering guarantee Linus Torvalds <torvalds@linux-foundation.org> - 2015-08-29 19:10 +0200
            Re: [PATCH] task_work: remove fifo ordering guarantee yalin wang <yalin.wang2010@gmail.com> - 2015-08-31 07:30 +0200
              Re: [PATCH] task_work: remove fifo ordering guarantee Al Viro <viro@ZenIV.linux.org.uk> - 2015-09-05 07:20 +0200
            Re: [PATCH] task_work: remove fifo ordering guarantee Oleg Nesterov <oleg@redhat.com> - 2015-08-31 14:50 +0200
            Re: [PATCH] task_work: remove fifo ordering guarantee Al Viro <viro@ZenIV.linux.org.uk> - 2015-09-05 07:20 +0200
              Re: [PATCH] task_work: remove fifo ordering guarantee Al Viro <viro@ZenIV.linux.org.uk> - 2015-09-05 07:50 +0200
                Re: [PATCH] task_work: remove fifo ordering guarantee Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-05 22:50 +0200
        change filp_close() to use __fput_sync() ? (Was: [PATCH]  task_work: remove fifo ordering guarantee) Oleg Nesterov <oleg@redhat.com> - 2015-08-31 14:10 +0200
      Re: [PATCH] task_work: remove fifo ordering guarantee Al Viro <viro@ZenIV.linux.org.uk> - 2015-09-05 07:40 +0200

#1215658 — [PATCH] task_work: remove fifo ordering guarantee

From: Eric Dumazet <edumazet@google.com>

In commit f341861fb0b ("task_work: add a scheduling point in
task_work_run()") I fixed a latency problem adding a cond_resched()
call.

Later, commit ac3d0da8f329 added yet another loop to reverse a list,
bringing back the latency spike :

I've seen in some cases this loop taking 275 ms, if for example a
process with 2,000,000 files is killed.

We could add yet another cond_resched() in the reverse loop, or we
can simply remove the reversal, as I do not think anything
would depend on order of task_work_add() submitted works.

Fixes: ac3d0da8f329 ("task_work: Make task_work_add() lockless")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Maciej Żenczykowski <maze@google.com>
---
 kernel/task_work.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/task_work.c b/kernel/task_work.c
index 8727032e3a6f..53fa971d000d 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -18,6 +18,8 @@ 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.
  */
@@ -108,16 +110,6 @@ 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);


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

#1215664

On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> We could add yet another cond_resched() in the reverse loop, or we
> can simply remove the reversal, as I do not think anything
> would depend on order of task_work_add() submitted works.

So I think this should be ok, with things like file closing not really
caring about ordering as far as I can tell.

However, has anybody gone through all the task-work users? I looked
quickly at the task_work_add() cases, and didn't see anything that
looked like it would care, but others should look too. In the vfs,
theres' the delayed fput and mnt freeing, and there's a keyring
installation one.

The threaded irq handlers use it as that exit-time hack, which
certainly shouldn't care, and there's some uprobe thing.

Can anybody see anything fishy?

               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]

#1215739

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > We could add yet another cond_resched() in the reverse loop, or we can simply 
> > remove the reversal, as I do not think anything would depend on order of 
> > task_work_add() submitted works.
> 
> So I think this should be ok, with things like file closing not really caring 
> about ordering as far as I can tell.
> 
> However, has anybody gone through all the task-work users? I looked quickly at 
> the task_work_add() cases, and didn't see anything that looked like it would 
> care, but others should look too. In the vfs, theres' the delayed fput and mnt 
> freeing, and there's a keyring installation one.
> 
> The threaded irq handlers use it as that exit-time hack, which certainly 
> shouldn't care, and there's some uprobe thing.
> 
> Can anybody see anything fishy?

So I'm wondering, is there any strong reason why we couldn't use a double linked 
list and still do FIFO and remove that silly linear list walking hack?

Thanks,

	Ingo
--
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]

#1215771

On 08/29, Ingo Molnar wrote:
>
> So I'm wondering, is there any strong reason why we couldn't use a double linked
> list and still do FIFO and remove that silly linear list walking hack?

This will obviously enlarge callback_head, and it is often embedded.
But this is minor.

If we use a double linked list we can't do task_work_add() lockless.
So we will need another spinlock_t in task_struct. We can't use pi_lock.

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]

#1216029

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 08/29, Ingo Molnar wrote:
> >
> > So I'm wondering, is there any strong reason why we couldn't use a double linked
> > list and still do FIFO and remove that silly linear list walking hack?
> 
> This will obviously enlarge callback_head, and it is often embedded.
> But this is minor.
> 
> If we use a double linked list we can't do task_work_add() lockless.
> So we will need another spinlock_t in task_struct. We can't use pi_lock.

The fact that the O(N) overhead was measured in real apps to be in the 
milliseconds IMHO weakens cycle-level concerns about also having a spinlock next 
to the list head. (There's no additional cacheline bouncing concerns with the 
spinlock: the head of a LIFO list is essentially a bouncing cacheline.)

If there's some other solution, sure, but LIFO queues tend to be trouble down the 
line.

Thanks,

	Ingo
--
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]

#1216167

On 08/31, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 08/29, Ingo Molnar wrote:
> > >
> > > So I'm wondering, is there any strong reason why we couldn't use a double linked
> > > list and still do FIFO and remove that silly linear list walking hack?
> >
> > This will obviously enlarge callback_head, and it is often embedded.
> > But this is minor.
> >
> > If we use a double linked list we can't do task_work_add() lockless.
> > So we will need another spinlock_t in task_struct. We can't use pi_lock.
>
> The fact that the O(N) overhead was measured in real apps to be in the
> milliseconds IMHO weakens cycle-level concerns about also having a spinlock next
> to the list head. (There's no additional cacheline bouncing concerns with the
> spinlock: the head of a LIFO list is essentially a bouncing cacheline.)

I agree. I just tried to explain that we need a bit more changes than
just s/callback_head/list_head/ in task_struct.

And. The fact that this O(N) overhead was measured means that we have
more overhead with offload-fput-to-exit_task_work which would be nice
to remove as well.

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]

#1215770

On 08/28, Eric Dumazet wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> In commit f341861fb0b ("task_work: add a scheduling point in
> task_work_run()") I fixed a latency problem adding a cond_resched()
> call.
>
> Later, commit ac3d0da8f329 added yet another loop to reverse a list,
> bringing back the latency spike :
>
> I've seen in some cases this loop taking 275 ms, if for example a
> process with 2,000,000 files is killed.
>
> We could add yet another cond_resched() in the reverse loop,

Can't we do this?

> or we
> can simply remove the reversal, as I do not think anything
> would depend on order of task_work_add() submitted works.

Personally I'd prefer to keep the fifo ordering. It just makes
more sense imho. Even if currently nobody depends on it (although
I am not sure about out-of-tree modules, say, systemtap).

Let's look keyctl_session_to_parent(). It does task_work_cancel()
but only because we can not trust user-space. Otherwise we could
remove it and just do task_work_add(), but this needs fifo.

Fifo just looks more sane to me.

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]

#1215775

On Sat, 2015-08-29 at 14:49 +0200, Oleg Nesterov wrote:
> On 08/28, Eric Dumazet wrote:
> >
> > From: Eric Dumazet <edumazet@google.com>
> >
> > In commit f341861fb0b ("task_work: add a scheduling point in
> > task_work_run()") I fixed a latency problem adding a cond_resched()
> > call.
> >
> > Later, commit ac3d0da8f329 added yet another loop to reverse a list,
> > bringing back the latency spike :
> >
> > I've seen in some cases this loop taking 275 ms, if for example a
> > process with 2,000,000 files is killed.
> >
> > We could add yet another cond_resched() in the reverse loop,
> 
> Can't we do this?

Well, I stated in the changelog we could. Obviously we can.

Adding 275 ms of pure overhead to perform this list reversal for files
to be closed is quite unfortunate.


> Personally I'd prefer to keep the fifo ordering. It just makes
> more sense imho. Even if currently nobody depends on it (although
> I am not sure about out-of-tree modules, say, systemtap).
> 
> Let's look keyctl_session_to_parent(). It does task_work_cancel()
> but only because we can not trust user-space. Otherwise we could
> remove it and just do task_work_add(), but this needs fifo.

So it looks like there is no problem today, right, other than the
possibility to parse a long list while blocking IRQ ?

> 
> Fifo just looks more sane to me.

Well, files are closed in a random order. These are the main user of
this stuff.

If this is that critical, maybe use 2 lists, one for stuff needing fifo,
and another one for un-ordered stuff (ed : file closing), and add a
boolean to task_work_add()/task_work_cancel(). This adds yet another
field into struct task_struct.

Now we also could question why we needed commit
4a9d4b024a3102fc083c925c242d98ac27b1c5f6 ("switch fput to task_work_add
") since it seems quite an overhead at task exit with 10^6 of files to
close.

I understood the 'schedule_work() for interrupt/kernel_thread callers'
part, but not the task_work_add() one.




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

#1215780

On Sat, 2015-08-29 at 06:57 -0700, Eric Dumazet wrote:

> Now we also could question why we needed commit
> 4a9d4b024a3102fc083c925c242d98ac27b1c5f6 ("switch fput to task_work_add
> ") since it seems quite an overhead at task exit with 10^6 of files to
> close.
> 
> I understood the 'schedule_work() for interrupt/kernel_thread callers'
> part, but not the task_work_add() one.

If this needs to be kept, maybe then add following, to make sure
we flush the list at most every BITS_PER_LONG files

diff --git a/fs/file.c b/fs/file.c
index 6c672ad329e9..f3d0a79cef05 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/workqueue.h>
+#include <linux/task_work.h>
 
 int sysctl_nr_open __read_mostly = 1024*1024;
 int sysctl_nr_open_min = BITS_PER_LONG;
@@ -392,6 +393,7 @@ static struct fdtable *close_files(struct
files_struct * files)
 			i++;
 			set >>= 1;
 		}
+		task_work_run();
 	}
 
 	return fdt;


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

#1215819

On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> If this needs to be kept, maybe then add following, to make sure
> we flush the list at most every BITS_PER_LONG files

Hmm.

I'm wondering if we should just make close_files() (or maybe even
filp_close()) use a synchronous fput().

Iirc, the reason we delay fput() is that we had some nasty issues for
the generic fput case. It was called from interrupt context by the aio
code, and just in general there's a lot of nasty cases that can cause
the final fput to happen (so there are lockdep issues with the mmap
locks because the last fput being from munmap etc).

Maybe I forget some detail - it's been several years by now - but I
think we could make the regular "close()" and "exit()" cases just use
the synchronous fput (it's called "__fput_sync()" and currently
explicitly limited to just kernel threads).

Al?

Because it feels all kinds of stupid to add things to the task-work
queue just to then remove it almost immediately again. And
close_files() is also called from various contexts. but the whole "put
the final 'files_struct' case is certainly not at all as special as
the 'put the final file'.

                    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]

#1216024

> On Aug 30, 2015, at 01:08, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> 
>> If this needs to be kept, maybe then add following, to make sure
>> we flush the list at most every BITS_PER_LONG files
> 
> Hmm.
> 
> I'm wondering if we should just make close_files() (or maybe even
> filp_close()) use a synchronous fput().
> 
> Iirc, the reason we delay fput() is that we had some nasty issues for
> the generic fput case. It was called from interrupt context by the aio
> code, and just in general there's a lot of nasty cases that can cause
> the final fput to happen (so there are lockdep issues with the mmap
> locks because the last fput being from munmap etc).
> 
> Maybe I forget some detail - it's been several years by now - but I
> think we could make the regular "close()" and "exit()" cases just use
> the synchronous fput (it's called "__fput_sync()" and currently
> explicitly limited to just kernel threads).
> 
> Al?
> 
> Because it feels all kinds of stupid to add things to the task-work
> queue just to then remove it almost immediately again. And
> close_files() is also called from various contexts. but the whole "put
> the final 'files_struct' case is certainly not at all as special as
> the 'put the final file'.
> 
>                    Linus
> —
why not provide API like:
fput()
fput_nosync() ?

because synchronous version are reasonable and safe in most time,
let the user to select which version to use is more feasible, no matter if it is kthread or not.

Thanks


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

#1219425

On Mon, Aug 31, 2015 at 01:22:26PM +0800, yalin wang wrote:
> why not provide API like:
> fput()
> fput_nosync() ?
> 
> because synchronous version are reasonable and safe in most time,
> let the user to select which version to use is more feasible, no matter if it is kthread or not.

Synchronous version is *NOT* safe in a lot of situations, from "deep enough
in kernel stack" to "now a function seven levels out in call chain happens
to hold a mutex grabbed elsewhere inside a mutex taken by unexpected
->release() instance, causing a deadlock", etc.

It's not sync vs. async; we still guarantee execution before return from
syscall.  The only case when we really get async is kernel threads - there
we do *not* return to userland at all, so we have to schedule it really
asynchronous.  Which is why we need an explicit sync version (for kernel
threads only, not exported, don't use unless you really understand what
you are doing and can explain why that particular case is safe, etc.)
--
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]

#1216163

On 08/29, Linus Torvalds wrote:
>
> On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > If this needs to be kept, maybe then add following, to make sure
> > we flush the list at most every BITS_PER_LONG files
>
> Hmm.
>
> I'm wondering if we should just make close_files() (or maybe even
> filp_close()) use a synchronous fput().

Heh. I thought about the same change. So perhaps it is even the right
thing to do. Still I am worried, because "it can't be that simple" ;)

And, with this change close_files() is called before exit_fs() and
exit_task_namespaces(). This is fine (iiuc), but this means that the
creative code in drivers/ can (wrongly) rely on this fact again. IIRC,
the change which moved __fput() into task_work_exit() uncovered some
interesting problems, like filp_open() called from fop->release().

Anyway, this is the question to Al, I guess.

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]

#1219424

On Sat, Aug 29, 2015 at 10:08:30AM -0700, Linus Torvalds wrote:
> Hmm.
> 
> I'm wondering if we should just make close_files() (or maybe even
> filp_close()) use a synchronous fput().
> 
> Iirc, the reason we delay fput() is that we had some nasty issues for
> the generic fput case. It was called from interrupt context by the aio
> code, and just in general there's a lot of nasty cases that can cause
> the final fput to happen (so there are lockdep issues with the mmap
> locks because the last fput being from munmap etc).
> 
> Maybe I forget some detail - it's been several years by now - but I
> think we could make the regular "close()" and "exit()" cases just use
> the synchronous fput (it's called "__fput_sync()" and currently
> explicitly limited to just kernel threads).
> 
> Al?

First of all, we'd better not count on e.g. delayed fput() *NOT* doing
task_work_add() - we still need to check if any new work had been added.
After all, final close() might very well have done a final mntput()
on a lazy-unmounted filesystem, possibly leaving us with fs shutdown via
task_work_add().  And if that sucker e.g. closes a socket, well, we are
back to closing an opened struct file, with task_work_add() etc.

I'm a bit nervious about filp_close() (that sucker is exported and widely
abused), but close_files()... sure, shouldn't be a problem.  And yes,
we can teach __close_fd() to do the same.  I really don't understand what's
the benefit, though - it's about the case when we are closing the last
descriptor for given opened file, so I would be rather surprised if slower
path taken on the way out to userland was not lost in noise...
--
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]

#1219427

On Sat, Sep 05, 2015 at 06:12:34AM +0100, Al Viro wrote:

> First of all, we'd better not count on e.g. delayed fput() *NOT* doing
> task_work_add() - we still need to check if any new work had been added.
> After all, final close() might very well have done a final mntput()
> on a lazy-unmounted filesystem, possibly leaving us with fs shutdown via
> task_work_add().  And if that sucker e.g. closes a socket, well, we are
> back to closing an opened struct file, with task_work_add() etc.
> 
> I'm a bit nervious about filp_close() (that sucker is exported and widely
> abused), but close_files()... sure, shouldn't be a problem.  And yes,
> we can teach __close_fd() to do the same.  I really don't understand what's
> the benefit, though - it's about the case when we are closing the last
> descriptor for given opened file, so I would be rather surprised if slower
> path taken on the way out to userland was not lost in noise...

OK, having found the beginning of the thread, I understand what is being
attempted, but... why the hell bother with FIFO in the first place?  AFAICS,
task_work_add() uses in VFS (final fput() and final mntput() alike)
do not care about the FIFO at all.

Sure, some out-of-tree mer^H^Hodule might rely on that.  So what?

IMO, unless we have a good in-tree reason for insisting on FIFO, dropping it
is the most obvious solution...
--
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]

#1219645

On Fri, Sep 4, 2015 at 10:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> OK, having found the beginning of the thread, I understand what is being
> attempted, but... why the hell bother with FIFO in the first place?  AFAICS,
> task_work_add() uses in VFS (final fput() and final mntput() alike)
> do not care about the FIFO at all.
>
> Sure, some out-of-tree mer^H^Hodule might rely on that.  So what?
>
> IMO, unless we have a good in-tree reason for insisting on FIFO, dropping it
> is the most obvious solution...

I agree. We should just try that.

I'll apply Eric's patch from the beginning of this tree, and let's
just see if anybody ever notices.

Removing code and possibly fixing a latency issue sounds like a win-win.

              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]

#1216155 — change filp_close() to use __fput_sync() ? (Was: [PATCH] task_work: remove fifo ordering guarantee)

On 08/29, Eric Dumazet wrote:
>
> On Sat, 2015-08-29 at 14:49 +0200, Oleg Nesterov wrote:
> > On 08/28, Eric Dumazet wrote:
> > >
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > In commit f341861fb0b ("task_work: add a scheduling point in
> > > task_work_run()") I fixed a latency problem adding a cond_resched()
> > > call.
> > >
> > > Later, commit ac3d0da8f329 added yet another loop to reverse a list,
> > > bringing back the latency spike :
> > >
> > > I've seen in some cases this loop taking 275 ms, if for example a
> > > process with 2,000,000 files is killed.
> > >
> > > We could add yet another cond_resched() in the reverse loop,
> >
> > Can't we do this?
>
> Well, I stated in the changelog we could. Obviously we can.
>
> Adding 275 ms of pure overhead to perform this list reversal for files
> to be closed is quite unfortunate.

Well, if the first loop takes 275 ms, then probably the next one which
actually does a lot of __fput's takes much, much more time, so perhaps
these 275 ms are not very noticable. Ignoring the latency problem.

But of course, this is not good, I agree. Please see below.

> > Fifo just looks more sane to me.
>
> Well, files are closed in a random order. These are the main user of
> this stuff.

This is the most "heavy" user. But task_works is the generic API.

> Now we also could question why we needed commit
> 4a9d4b024a3102fc083c925c242d98ac27b1c5f6 ("switch fput to task_work_add
> ") since it seems quite an overhead at task exit with 10^6 of files to
> close.

How about the patch below? I didn't try to test it yet, but since
filp_close() does ->flush() I think __fput_sync() should be safe here.

Al, what do you think?

Oleg.


--- x/fs/file_table.c
+++ x/fs/file_table.c
@@ -292,11 +292,8 @@ void fput(struct file *file)
  */
 void __fput_sync(struct file *file)
 {
-	if (atomic_long_dec_and_test(&file->f_count)) {
-		struct task_struct *task = current;
-		BUG_ON(!(task->flags & PF_KTHREAD));
+	if (atomic_long_dec_and_test(&file->f_count))
 		__fput(file);
-	}
 }
 
 EXPORT_SYMBOL(fput);
--- x/fs/open.c
+++ x/fs/open.c
@@ -1074,7 +1074,7 @@ int filp_close(struct file *filp, fl_owner_t id)
 		dnotify_flush(filp, id);
 		locks_remove_posix(filp, id);
 	}
-	fput(filp);
+	__fput_sync(filp);
 	return retval;
 }
 

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

#1219426

On Sat, Aug 29, 2015 at 02:49:21PM +0200, Oleg Nesterov wrote:

> Let's look keyctl_session_to_parent(). It does task_work_cancel()
> but only because we can not trust user-space. Otherwise we could
> remove it and just do task_work_add(), but this needs fifo.
> 
> Fifo just looks more sane to me.

Not if it costs us.  As far as files closing is concerned, the order
really doesn't matter.  Ditto for final mntput() uses of that stuff.

What *does* matter is task_work_add() issued by callback not getting lost.

IMO the obvious solution is to lose the reordering...
--
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