Re: [PATCH] task_work: remove fifo ordering guarantee

On Fri, Sep 4, 2015 at 10:42 PM, Al Viro 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Fri, Sep 4, 2015 at 10:42 PM, Al Viro 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()

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

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.

Re: [PATCH] task_work: remove fifo ordering guarantee

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.

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

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.

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

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.

Re: [PATCH] task_work: remove fifo ordering guarantee

Oleg Nesterov wrote: > Actually you need a single tail pointer, See 158e1645e07f3e9f7e49. > But this doesn't matter. (This uses a circularly linked list, keeping a pointer to the tail, and tail->next pointing to the head.) Yes, if you're not trying to be lockless, that works quite well. > And

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/29, George Spelvin wrote: > > Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/31, Ingo Molnar wrote: > > * Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/29, Linus Torvalds wrote: > > On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet 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

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 > > > > > > In commit f341861fb0b ("task_work: add a scheduling point in > > > task_work_run()") I fixed a latency problem adding a

Re: [PATCH] task_work: remove fifo ordering guarantee

* Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

* Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

Oleg Nesterov wrote: > Actually you need a single tail pointer, See 158e1645e07f3e9f7e49. > But this doesn't matter. (This uses a circularly linked list, keeping a pointer to the tail, and tail->next pointing to the head.) Yes, if you're not trying to be lockless, that works quite well. > And

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/29, Linus Torvalds wrote: > > On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/29, George Spelvin wrote: > > Oleg Nesterov 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

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 > > > > > > In commit f341861fb0b ("task_work: add a scheduling point in > > > task_work_run()") I fixed a latency

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/31, Ingo Molnar wrote: > > * Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

> On Aug 30, 2015, at 01:08, Linus Torvalds > wrote: > > On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Aug 30, 2015, at 01:08, Linus Torvalds torva...@linux-foundation.org wrote: On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet eric.duma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

Oleg Nesterov 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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet 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,

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Sat, 2015-08-29 at 14:49 +0200, Oleg Nesterov wrote: > On 08/28, Eric Dumazet wrote: > > > > From: Eric Dumazet > > > > 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

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/28, Eric Dumazet wrote: > > From: Eric Dumazet > > 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

Re: [PATCH] task_work: remove fifo ordering guarantee

* Linus Torvalds wrote: > On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet 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

Re: [PATCH] task_work: remove fifo ordering guarantee

* Linus Torvalds torva...@linux-foundation.org wrote: On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet eric.duma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

On 08/28, Eric Dumazet wrote: From: Eric Dumazet eduma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Sat, 2015-08-29 at 14:49 +0200, Oleg Nesterov wrote: On 08/28, Eric Dumazet wrote: From: Eric Dumazet eduma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Sat, Aug 29, 2015 at 7:11 AM, Eric Dumazet eric.duma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

Oleg Nesterov 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.

Re: [PATCH] task_work: remove fifo ordering guarantee

On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet 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

[PATCH] task_work: remove fifo ordering guarantee

From: Eric Dumazet 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

[PATCH] task_work: remove fifo ordering guarantee

From: Eric Dumazet eduma...@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

Re: [PATCH] task_work: remove fifo ordering guarantee

On Fri, Aug 28, 2015 at 7:42 PM, Eric Dumazet eric.duma...@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,