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

Groups > linux.kernel > #1280841 > unrolled thread

Re: [BISECTED] rcu_sched self-detected stall since 3.17

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

  Re: [BISECTED] rcu_sched self-detected stall since 3.17 Peter Zijlstra <peterz@infradead.org> - 2015-12-01 14:10 +0100
    Re: [BISECTED] rcu_sched self-detected stall since 3.17 Vladimir Murzin <vladimir.murzin@arm.com> - 2015-12-02 10:10 +0100
    [tip:locking/core] sched/wait:   Fix signal handling in bit wait helpers tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2015-12-04 13:00 +0100
      Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Peter Zijlstra <peterz@infradead.org> - 2015-12-08 11:50 +0100
        Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers NeilBrown <nfbrown@novell.com> - 2015-12-09 02:10 +0100
          Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Peter Zijlstra <peterz@infradead.org> - 2015-12-09 08:50 +0100
            Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers NeilBrown <nfbrown@novell.com> - 2015-12-09 22:40 +0100
              Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Peter Zijlstra <peterz@infradead.org> - 2015-12-10 14:20 +0100
                Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers Paul Turner <pjt@google.com> - 2015-12-11 12:40 +0100
                  Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Peter Zijlstra <peterz@infradead.org> - 2015-12-11 12:50 +0100
                    Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Vladimir Murzin <vladimir.murzin@arm.com> - 2015-12-11 13:00 +0100
                    Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Jan Stancek <jstancek@redhat.com> - 2015-12-11 14:10 +0100
                      Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Peter Zijlstra <peterz@infradead.org> - 2015-12-11 14:30 +0100
                    Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait  helpers Vladimir Murzin <vladimir.murzin@arm.com> - 2015-12-11 19:00 +0100

#1280841 — Re: [BISECTED] rcu_sched self-detected stall since 3.17

Sorry for the delay and thanks for the reminder!

On Fri, Nov 20, 2015 at 03:35:38PM +0000, Vladimir Murzin wrote:
> commit 743162013d40ca612b4cb53d3a200dff2d9ab26e
> Author: NeilBrown <neilb@suse.de>
> Date:   Mon Jul 7 15:16:04 2014 +1000
> 
>     sched: Remove proliferation of wait_on_bit() action functions
> 
> The only change I noticed is from (mm/filemap.c)
> 
> 	io_schedule();
> 	fatal_signal_pending(current)
> 
> to (kernel/sched/wait.c)
> 
> 	signal_pending_state(current->state, current)
> 	io_schedule();
> 
> and if I apply following diff I don't see stalls anymore.
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index a104879..2d68cdb 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -514,9 +514,10 @@ EXPORT_SYMBOL(bit_wait);
> 
>  __sched int bit_wait_io(void *word)
>  {
> +       io_schedule();
> +
>         if (signal_pending_state(current->state, current))
>                 return 1;
> -       io_schedule();
>         return 0;
>  }
>  EXPORT_SYMBOL(bit_wait_io);
> 
> Any ideas why it might happen and why diff above helps?

Yes, the code as presented is simply wrong. And in fact most of the code
it replaced was of the right form (with a few exceptions which would
indeed have been subject to the same problem you've observed.

Note how the late:

  - cifs_sb_tcon_pending_wait
  - fscache_wait_bit_interruptible
  - sleep_on_page_killable
  - wait_inquiry
  - key_wait_bit_intr

All check the signal state _after_ calling schedule().

As opposed to:

  - gfs2_journalid_wait

which follows the broken pattern.

Further notice that most expect a return of -EINTR, which also seems
correct given that this is a signal, those that do not return -EINTR
only check for a !0 return value so would work equally well with -EINTR.

The reason this is broken is that schedule() will no-op when there is a
pending signal, while raising a signal will also issue a wakeup.

Thus the right thing to do is check for the signal state after, that way
you handle both cases:

 - calling schedule() with a signal pending
 - receiving a signal while sleeping

As such, I would propose the below patch. Oleg, do you concur?

---
Subject: sched,wait: Fix signal handling in bit wait helpers

Vladimir reported getting RCU stall warnings and bisected it back to
commit 743162013d40. That commit inadvertently reversed the calls to
schedule() and signal_pending(), thereby not handling the case where the
signal receives while we sleep.

Fixes: 743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")
Fixes: cbbce8220949 ("SCHED: add some "wait..on_bit...timeout()" interfaces.")
Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/wait.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 052e02672d12..f10bd873e684 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -583,18 +583,18 @@ EXPORT_SYMBOL(wake_up_atomic_t);
 
 __sched int bit_wait(struct wait_bit_key *word)
 {
-	if (signal_pending_state(current->state, current))
-		return 1;
 	schedule();
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait);
 
 __sched int bit_wait_io(struct wait_bit_key *word)
 {
-	if (signal_pending_state(current->state, current))
-		return 1;
 	io_schedule();
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait_io);
@@ -602,11 +602,11 @@ EXPORT_SYMBOL(bit_wait_io);
 __sched int bit_wait_timeout(struct wait_bit_key *word)
 {
 	unsigned long now = READ_ONCE(jiffies);
-	if (signal_pending_state(current->state, current))
-		return 1;
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	schedule_timeout(word->timeout - now);
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bit_wait_timeout);
@@ -614,11 +614,11 @@ EXPORT_SYMBOL_GPL(bit_wait_timeout);
 __sched int bit_wait_io_timeout(struct wait_bit_key *word)
 {
 	unsigned long now = READ_ONCE(jiffies);
-	if (signal_pending_state(current->state, current))
-		return 1;
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	io_schedule_timeout(word->timeout - now);
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bit_wait_io_timeout);
--
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]

#1281535

On 01/12/15 13:04, Peter Zijlstra wrote:
> Sorry for the delay and thanks for the reminder!
> 
> On Fri, Nov 20, 2015 at 03:35:38PM +0000, Vladimir Murzin wrote:
>> commit 743162013d40ca612b4cb53d3a200dff2d9ab26e
>> Author: NeilBrown <neilb@suse.de>
>> Date:   Mon Jul 7 15:16:04 2014 +1000
>>
>>     sched: Remove proliferation of wait_on_bit() action functions
>>
>> The only change I noticed is from (mm/filemap.c)
>>
>> 	io_schedule();
>> 	fatal_signal_pending(current)
>>
>> to (kernel/sched/wait.c)
>>
>> 	signal_pending_state(current->state, current)
>> 	io_schedule();
>>
>> and if I apply following diff I don't see stalls anymore.
>>
>> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
>> index a104879..2d68cdb 100644
>> --- a/kernel/sched/wait.c
>> +++ b/kernel/sched/wait.c
>> @@ -514,9 +514,10 @@ EXPORT_SYMBOL(bit_wait);
>>
>>  __sched int bit_wait_io(void *word)
>>  {
>> +       io_schedule();
>> +
>>         if (signal_pending_state(current->state, current))
>>                 return 1;
>> -       io_schedule();
>>         return 0;
>>  }
>>  EXPORT_SYMBOL(bit_wait_io);
>>
>> Any ideas why it might happen and why diff above helps?
> 
> Yes, the code as presented is simply wrong. And in fact most of the code
> it replaced was of the right form (with a few exceptions which would
> indeed have been subject to the same problem you've observed.
> 
> Note how the late:
> 
>   - cifs_sb_tcon_pending_wait
>   - fscache_wait_bit_interruptible
>   - sleep_on_page_killable
>   - wait_inquiry
>   - key_wait_bit_intr
> 
> All check the signal state _after_ calling schedule().
> 
> As opposed to:
> 
>   - gfs2_journalid_wait
> 
> which follows the broken pattern.
> 
> Further notice that most expect a return of -EINTR, which also seems
> correct given that this is a signal, those that do not return -EINTR
> only check for a !0 return value so would work equally well with -EINTR.
> 
> The reason this is broken is that schedule() will no-op when there is a
> pending signal, while raising a signal will also issue a wakeup.
> 

Glad to hear confirmation on a problem. Thanks for detailed answer!

> Thus the right thing to do is check for the signal state after, that way
> you handle both cases:
> 
>  - calling schedule() with a signal pending
>  - receiving a signal while sleeping
> 
> As such, I would propose the below patch. Oleg, do you concur?
> 
> ---
> Subject: sched,wait: Fix signal handling in bit wait helpers
> 
> Vladimir reported getting RCU stall warnings and bisected it back to
> commit 743162013d40. That commit inadvertently reversed the calls to
> schedule() and signal_pending(), thereby not handling the case where the
> signal receives while we sleep.
> 
> Fixes: 743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")
> Fixes: cbbce8220949 ("SCHED: add some "wait..on_bit...timeout()" interfaces.")
> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/wait.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 052e02672d12..f10bd873e684 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -583,18 +583,18 @@ EXPORT_SYMBOL(wake_up_atomic_t);
>  
>  __sched int bit_wait(struct wait_bit_key *word)
>  {
> -	if (signal_pending_state(current->state, current))
> -		return 1;
>  	schedule();
> +	if (signal_pending(current))
> +		return -EINTR;
>  	return 0;
>  }
>  EXPORT_SYMBOL(bit_wait);
>  
>  __sched int bit_wait_io(struct wait_bit_key *word)
>  {
> -	if (signal_pending_state(current->state, current))
> -		return 1;
>  	io_schedule();
> +	if (signal_pending(current))
> +		return -EINTR;
>  	return 0;
>  }
>  EXPORT_SYMBOL(bit_wait_io);
> @@ -602,11 +602,11 @@ EXPORT_SYMBOL(bit_wait_io);
>  __sched int bit_wait_timeout(struct wait_bit_key *word)
>  {
>  	unsigned long now = READ_ONCE(jiffies);
> -	if (signal_pending_state(current->state, current))
> -		return 1;
>  	if (time_after_eq(now, word->timeout))
>  		return -EAGAIN;
>  	schedule_timeout(word->timeout - now);
> +	if (signal_pending(current))
> +		return -EINTR;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(bit_wait_timeout);
> @@ -614,11 +614,11 @@ EXPORT_SYMBOL_GPL(bit_wait_timeout);
>  __sched int bit_wait_io_timeout(struct wait_bit_key *word)
>  {
>  	unsigned long now = READ_ONCE(jiffies);
> -	if (signal_pending_state(current->state, current))
> -		return 1;
>  	if (time_after_eq(now, word->timeout))
>  		return -EAGAIN;
>  	io_schedule_timeout(word->timeout - now);
> +	if (signal_pending(current))
> +		return -EINTR;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(bit_wait_io_timeout);
> 

I run it overnight on top of 4.3 and didn't see stalls. So in case it helps

Tested-by: Vladimir Murzin <vladimir.murzin@arm.com>

Cheers
Vladimir

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

#1283742 — [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

Commit-ID:  68985633bccb6066bf1803e316fbc6c1f5b796d6
Gitweb:     http://git.kernel.org/tip/68985633bccb6066bf1803e316fbc6c1f5b796d6
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 1 Dec 2015 14:04:04 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 4 Dec 2015 10:10:15 +0100

sched/wait: Fix signal handling in bit wait helpers

Vladimir reported getting RCU stall warnings and bisected it back to
commit:

  743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")

That commit inadvertently reversed the calls to schedule() and signal_pending(),
thereby not handling the case where the signal receives while we sleep.

Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
Tested-by: Vladimir Murzin <vladimir.murzin@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: mark.rutland@arm.com
Cc: neilb@suse.de
Cc: oleg@redhat.com
Fixes: 743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")
Fixes: cbbce8220949 ("SCHED: add some "wait..on_bit...timeout()" interfaces.")
Link: http://lkml.kernel.org/r/20151201130404.GL3816@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/wait.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 052e026..f10bd87 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -583,18 +583,18 @@ EXPORT_SYMBOL(wake_up_atomic_t);
 
 __sched int bit_wait(struct wait_bit_key *word)
 {
-	if (signal_pending_state(current->state, current))
-		return 1;
 	schedule();
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait);
 
 __sched int bit_wait_io(struct wait_bit_key *word)
 {
-	if (signal_pending_state(current->state, current))
-		return 1;
 	io_schedule();
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait_io);
@@ -602,11 +602,11 @@ EXPORT_SYMBOL(bit_wait_io);
 __sched int bit_wait_timeout(struct wait_bit_key *word)
 {
 	unsigned long now = READ_ONCE(jiffies);
-	if (signal_pending_state(current->state, current))
-		return 1;
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	schedule_timeout(word->timeout - now);
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bit_wait_timeout);
@@ -614,11 +614,11 @@ EXPORT_SYMBOL_GPL(bit_wait_timeout);
 __sched int bit_wait_io_timeout(struct wait_bit_key *word)
 {
 	unsigned long now = READ_ONCE(jiffies);
-	if (signal_pending_state(current->state, current))
-		return 1;
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	io_schedule_timeout(word->timeout - now);
+	if (signal_pending(current))
+		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bit_wait_io_timeout);
--
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]

#1286387 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Fri, Dec 04, 2015 at 03:52:12AM -0800, tip-bot for Peter Zijlstra wrote:
> Commit-ID:  68985633bccb6066bf1803e316fbc6c1f5b796d6
> Gitweb:     http://git.kernel.org/tip/68985633bccb6066bf1803e316fbc6c1f5b796d6
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Tue, 1 Dec 2015 14:04:04 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 4 Dec 2015 10:10:15 +0100
> 
> sched/wait: Fix signal handling in bit wait helpers
> 
> Vladimir reported getting RCU stall warnings and bisected it back to
> commit:
> 
>   743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")
> 
> That commit inadvertently reversed the calls to schedule() and signal_pending(),
> thereby not handling the case where the signal receives while we sleep.
> 
> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Tested-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: mark.rutland@arm.com
> Cc: neilb@suse.de
> Cc: oleg@redhat.com
> Fixes: 743162013d40 ("sched: Remove proliferation of wait_on_bit() action functions")
> Fixes: cbbce8220949 ("SCHED: add some "wait..on_bit...timeout()" interfaces.")
> Link: http://lkml.kernel.org/r/20151201130404.GL3816@twins.programming.kicks-ass.net
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/sched/wait.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 052e026..f10bd87 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -583,18 +583,18 @@ EXPORT_SYMBOL(wake_up_atomic_t);
>  
>  __sched int bit_wait(struct wait_bit_key *word)
>  {
> -	if (signal_pending_state(current->state, current))
> -		return 1;
>  	schedule();
> +	if (signal_pending(current))
> +		return -EINTR;
>  	return 0;
>  }

*sigh*, so that patch was broken.. the below might fix it, but please
someone look at it, I seem to have a less than stellar track record
here...

---
Subject: sched/wait: Fix the signal handling fix

Jan Stancek reported that I wrecked things for him by fixing things for
Vladimir :/

His report was due to an UNINTERRUPTIBLE wait getting -EINTR, which
should not be possible, however my previous patch made this possible by
unconditionally checking signal_pending().

We cannot use current->state as was done previously, because the
instruction after the store to that variable it can be changed. We must
instead pass the initial state along and use that.

Fixes: 68985633bccb ("sched/wait: Fix signal handling in bit wait helpers")
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: mark.rutland@arm.com
Cc: neilb@suse.de
Cc: oleg@redhat.com
Cc: Vladimir Murzin <vladimir.murzin@arm.com>
Reported-by: Jan Stancek <jstancek@redhat.com>
Tested-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 fs/cifs/inode.c      |    6 +++---
 fs/nfs/inode.c       |    6 +++---
 fs/nfs/internal.h    |    2 +-
 fs/nfs/pagelist.c    |    2 +-
 fs/nfs/pnfs.c        |    4 ++--
 include/linux/wait.h |   10 +++++-----
 kernel/sched/wait.c  |   20 ++++++++++----------
 net/sunrpc/sched.c   |    6 +++---
 8 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 6b66dd5..a329f5b 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1831,11 +1831,11 @@ cifs_invalidate_mapping(struct inode *inode)
  * @word: long word containing the bit lock
  */
 static int
-cifs_wait_bit_killable(struct wait_bit_key *key)
+cifs_wait_bit_killable(struct wait_bit_key *key, int mode)
 {
-	if (fatal_signal_pending(current))
-		return -ERESTARTSYS;
 	freezable_schedule_unsafe();
+	if (signal_pending_state(mode, current))
+		return -ERESTARTSYS;
 	return 0;
 }
 
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 31b0a52..c7e8b87 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -75,11 +75,11 @@ nfs_fattr_to_ino_t(struct nfs_fattr *fattr)
  * nfs_wait_bit_killable - helper for functions that are sleeping on bit locks
  * @word: long word containing the bit lock
  */
-int nfs_wait_bit_killable(struct wait_bit_key *key)
+int nfs_wait_bit_killable(struct wait_bit_key *key, int mode)
 {
-	if (fatal_signal_pending(current))
-		return -ERESTARTSYS;
 	freezable_schedule_unsafe();
+	if (signal_pending_state(mode, current))
+		return -ERESTARTSYS;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 56cfde2..9dea85f 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -379,7 +379,7 @@ extern int nfs_drop_inode(struct inode *);
 extern void nfs_clear_inode(struct inode *);
 extern void nfs_evict_inode(struct inode *);
 void nfs_zap_acl_cache(struct inode *inode);
-extern int nfs_wait_bit_killable(struct wait_bit_key *key);
+extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
 
 /* super.c */
 extern const struct super_operations nfs_sops;
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index fe3ddd2..452a011 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -129,7 +129,7 @@ __nfs_iocounter_wait(struct nfs_io_counter *c)
 		set_bit(NFS_IO_INPROGRESS, &c->flags);
 		if (atomic_read(&c->io_count) == 0)
 			break;
-		ret = nfs_wait_bit_killable(&q.key);
+		ret = nfs_wait_bit_killable(&q.key, TASK_KILLABLE);
 	} while (atomic_read(&c->io_count) != 0 && !ret);
 	finish_wait(wq, &q.wait);
 	return ret;
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 5a8ae21..bec0384 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1466,11 +1466,11 @@ static bool pnfs_within_mdsthreshold(struct nfs_open_context *ctx,
 }
 
 /* stop waiting if someone clears NFS_LAYOUT_RETRY_LAYOUTGET bit. */
-static int pnfs_layoutget_retry_bit_wait(struct wait_bit_key *key)
+static int pnfs_layoutget_retry_bit_wait(struct wait_bit_key *key, int mode)
 {
 	if (!test_bit(NFS_LAYOUT_RETRY_LAYOUTGET, key->flags))
 		return 1;
-	return nfs_wait_bit_killable(key);
+	return nfs_wait_bit_killable(key, mode);
 }
 
 static bool pnfs_prepare_to_retry_layoutget(struct pnfs_layout_hdr *lo)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 1e1bf9f..513b36f 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -145,7 +145,7 @@ __remove_wait_queue(wait_queue_head_t *head, wait_queue_t *old)
 	list_del(&old->task_list);
 }
 
-typedef int wait_bit_action_f(struct wait_bit_key *);
+typedef int wait_bit_action_f(struct wait_bit_key *, int mode);
 void __wake_up(wait_queue_head_t *q, unsigned int mode, int nr, void *key);
 void __wake_up_locked_key(wait_queue_head_t *q, unsigned int mode, void *key);
 void __wake_up_sync_key(wait_queue_head_t *q, unsigned int mode, int nr, void *key);
@@ -960,10 +960,10 @@ int wake_bit_function(wait_queue_t *wait, unsigned mode, int sync, void *key);
 	} while (0)
 
 
-extern int bit_wait(struct wait_bit_key *);
-extern int bit_wait_io(struct wait_bit_key *);
-extern int bit_wait_timeout(struct wait_bit_key *);
-extern int bit_wait_io_timeout(struct wait_bit_key *);
+extern int bit_wait(struct wait_bit_key *, int);
+extern int bit_wait_io(struct wait_bit_key *, int);
+extern int bit_wait_timeout(struct wait_bit_key *, int);
+extern int bit_wait_io_timeout(struct wait_bit_key *, int);
 
 /**
  * wait_on_bit - wait for a bit to be cleared
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index f10bd87..f15d6b6 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -392,7 +392,7 @@ __wait_on_bit(wait_queue_head_t *wq, struct wait_bit_queue *q,
 	do {
 		prepare_to_wait(wq, &q->wait, mode);
 		if (test_bit(q->key.bit_nr, q->key.flags))
-			ret = (*action)(&q->key);
+			ret = (*action)(&q->key, mode);
 	} while (test_bit(q->key.bit_nr, q->key.flags) && !ret);
 	finish_wait(wq, &q->wait);
 	return ret;
@@ -431,7 +431,7 @@ __wait_on_bit_lock(wait_queue_head_t *wq, struct wait_bit_queue *q,
 		prepare_to_wait_exclusive(wq, &q->wait, mode);
 		if (!test_bit(q->key.bit_nr, q->key.flags))
 			continue;
-		ret = action(&q->key);
+		ret = action(&q->key, mode);
 		if (!ret)
 			continue;
 		abort_exclusive_wait(wq, &q->wait, mode, &q->key);
@@ -581,43 +581,43 @@ void wake_up_atomic_t(atomic_t *p)
 }
 EXPORT_SYMBOL(wake_up_atomic_t);
 
-__sched int bit_wait(struct wait_bit_key *word)
+__sched int bit_wait(struct wait_bit_key *word, int mode)
 {
 	schedule();
-	if (signal_pending(current))
+	if (signal_pending_state(mode, current))
 		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait);
 
-__sched int bit_wait_io(struct wait_bit_key *word)
+__sched int bit_wait_io(struct wait_bit_key *word, int mode)
 {
 	io_schedule();
-	if (signal_pending(current))
+	if (signal_pending_state(mode, current))
 		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL(bit_wait_io);
 
-__sched int bit_wait_timeout(struct wait_bit_key *word)
+__sched int bit_wait_timeout(struct wait_bit_key *word, int mode)
 {
 	unsigned long now = READ_ONCE(jiffies);
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	schedule_timeout(word->timeout - now);
-	if (signal_pending(current))
+	if (signal_pending_state(mode, current))
 		return -EINTR;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bit_wait_timeout);
 
-__sched int bit_wait_io_timeout(struct wait_bit_key *word)
+__sched int bit_wait_io_timeout(struct wait_bit_key *word, int mode)
 {
 	unsigned long now = READ_ONCE(jiffies);
 	if (time_after_eq(now, word->timeout))
 		return -EAGAIN;
 	io_schedule_timeout(word->timeout - now);
-	if (signal_pending(current))
+	if (signal_pending_state(mode, current))
 		return -EINTR;
 	return 0;
 }
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index f14f24e..73ad57a 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -250,11 +250,11 @@ void rpc_destroy_wait_queue(struct rpc_wait_queue *queue)
 }
 EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
-static int rpc_wait_bit_killable(struct wait_bit_key *key)
+static int rpc_wait_bit_killable(struct wait_bit_key *key, int mode)
 {
-	if (fatal_signal_pending(current))
-		return -ERESTARTSYS;
 	freezable_schedule_unsafe();
+	if (signal_pending_state(mode, current))
+		return -ERESTARTSYS;
 	return 0;
 }
 
--
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]

#1286961 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

[Multipart message — attachments visible in raw view] — view raw

On Tue, Dec 08 2015, Peter Zijlstra wrote:

>>  
>
> *sigh*, so that patch was broken.. the below might fix it, but please
> someone look at it, I seem to have a less than stellar track record
> here...

This new change seems to be more intrusive than should be needed.
Can't we just do:


 __sched int bit_wait(struct wait_bit_key *word)
 {
+	long state = current->state;
-	if (signal_pending_state(current->state, current))
-		return 1;
 	schedule();
+	if (signal_pending_state(state, current))
+		return -EINTR;
 	return 0;
 }

??

(and sorry for breaking this in the first place!)

NeilBrown

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

#1287167 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Wed, Dec 09, 2015 at 12:06:33PM +1100, NeilBrown wrote:
> On Tue, Dec 08 2015, Peter Zijlstra wrote:
> 
> >>  
> >
> > *sigh*, so that patch was broken.. the below might fix it, but please
> > someone look at it, I seem to have a less than stellar track record
> > here...
> 
> This new change seems to be more intrusive than should be needed.
> Can't we just do:
> 
> 
>  __sched int bit_wait(struct wait_bit_key *word)
>  {
> +	long state = current->state;

No, current->state can already be changed by this time.
--
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]

#1287878 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

[Multipart message — attachments visible in raw view] — view raw

On Wed, Dec 09 2015, Peter Zijlstra wrote:

> On Wed, Dec 09, 2015 at 12:06:33PM +1100, NeilBrown wrote:
>> On Tue, Dec 08 2015, Peter Zijlstra wrote:
>> 
>> >>  
>> >
>> > *sigh*, so that patch was broken.. the below might fix it, but please
>> > someone look at it, I seem to have a less than stellar track record
>> > here...
>> 
>> This new change seems to be more intrusive than should be needed.
>> Can't we just do:
>> 
>> 
>>  __sched int bit_wait(struct wait_bit_key *word)
>>  {
>> +	long state = current->state;
>
> No, current->state can already be changed by this time.

Does that matter?
It can only have changed to TASK_RUNNING - right?
In that case signal_pending_state() will return 0 and the bit_wait() acts
as though the thread was woken up normally (which it was) rather than by
a signal (which maybe it was too, but maybe that happened just a tiny
bit later).

As long as signal delivery doesn't change ->state, we should be safe.
We should even be safe testing ->state *after* the call the schedule().

NeilBrown

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

#1288505 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Thu, Dec 10, 2015 at 08:30:01AM +1100, NeilBrown wrote:
> On Wed, Dec 09 2015, Peter Zijlstra wrote:
> 
> > On Wed, Dec 09, 2015 at 12:06:33PM +1100, NeilBrown wrote:
> >> On Tue, Dec 08 2015, Peter Zijlstra wrote:
> >> 
> >> >>  
> >> >
> >> > *sigh*, so that patch was broken.. the below might fix it, but please
> >> > someone look at it, I seem to have a less than stellar track record
> >> > here...
> >> 
> >> This new change seems to be more intrusive than should be needed.
> >> Can't we just do:
> >> 
> >> 
> >>  __sched int bit_wait(struct wait_bit_key *word)
> >>  {
> >> +	long state = current->state;
> >
> > No, current->state can already be changed by this time.
> 
> Does that matter?
> It can only have changed to TASK_RUNNING - right?
> In that case signal_pending_state() will return 0 and the bit_wait() acts
> as though the thread was woken up normally (which it was) rather than by
> a signal (which maybe it was too, but maybe that happened just a tiny
> bit later).
> 
> As long as signal delivery doesn't change ->state, we should be safe.
> We should even be safe testing ->state *after* the call the schedule().

Blergh, all I've managed to far is to confuse myself further. Even
something like the original (+- the EINTR) should work when we consider
the looping, even when mixed with an occasional spurious wakeup.


int bit_wait()
{
	if (signal_pending_state(current->state, current))
		return -EINTR;
	schedule();
}


This can go wrong against raising a signal thusly:

	prepare_to_wait()
1:	if (signal_pending_state(current->state, current))
		// false, nothing pending
	schedule();
				set_tsk_thread_flag(t, TIF_SIGPENDING);

		<spurious wakeup>

	prepare_to_wait()
				wake_up_state(t, ...);
2:	if (signal_pending_state(current->state, current))
		// false, TASK_RUNNING

	schedule(); // doesn't block because pending

	prepare_to_wait()
3:	if (signal_pending_state(current->state, current))
		// true, pending


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

#1289478 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Thu, Dec 10, 2015 at 5:09 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Dec 10, 2015 at 08:30:01AM +1100, NeilBrown wrote:
>> On Wed, Dec 09 2015, Peter Zijlstra wrote:
>>
>> > On Wed, Dec 09, 2015 at 12:06:33PM +1100, NeilBrown wrote:
>> >> On Tue, Dec 08 2015, Peter Zijlstra wrote:
>> >>
>> >> >>
>> >> >
>> >> > *sigh*, so that patch was broken.. the below might fix it, but please
>> >> > someone look at it, I seem to have a less than stellar track record
>> >> > here...
>> >>
>> >> This new change seems to be more intrusive than should be needed.
>> >> Can't we just do:
>> >>
>> >>
>> >>  __sched int bit_wait(struct wait_bit_key *word)
>> >>  {
>> >> +  long state = current->state;
>> >
>> > No, current->state can already be changed by this time.
>>
>> Does that matter?
>> It can only have changed to TASK_RUNNING - right?
>> In that case signal_pending_state() will return 0 and the bit_wait() acts
>> as though the thread was woken up normally (which it was) rather than by
>> a signal (which maybe it was too, but maybe that happened just a tiny
>> bit later).
>>
>> As long as signal delivery doesn't change ->state, we should be safe.
>> We should even be safe testing ->state *after* the call the schedule().
>
> Blergh, all I've managed to far is to confuse myself further. Even
> something like the original (+- the EINTR) should work when we consider
> the looping, even when mixed with an occasional spurious wakeup.
>
>
> int bit_wait()
> {
>         if (signal_pending_state(current->state, current))
>                 return -EINTR;
>         schedule();
> }
>
>
> This can go wrong against raising a signal thusly:
>
>         prepare_to_wait()
> 1:      if (signal_pending_state(current->state, current))
>                 // false, nothing pending
>         schedule();
>                                 set_tsk_thread_flag(t, TIF_SIGPENDING);
>
>                 <spurious wakeup>
>
>         prepare_to_wait()
>                                 wake_up_state(t, ...);
> 2:      if (signal_pending_state(current->state, current))
>                 // false, TASK_RUNNING
>
>         schedule(); // doesn't block because pending

Note that a quick inspection does not turn up _any_ TASK_INTERRUPTIBLE
callers.  When this previously occurred, it could likely only be with
a fatal signal, which would have hidden these sins.

>
>         prepare_to_wait()
> 3:      if (signal_pending_state(current->state, current))
>                 // true, pending
>

Hugh asked me about this after seeing a crash, here's another exciting
way in which the current code breaks -- this one actually quite
serious:

Consider __lock_page:

void __lock_page(struct page *page)
{
        DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
        __wait_on_bit_lock(page_waitqueue(page), &wait, bit_wait_io,
TASK_UNINTERRUPTIBLE);
}

With the current state of the world,

 __sched int bit_wait_io(struct wait_bit_key *word)
 {
-       if (signal_pending_state(current->state, current))
-               return 1;
        io_schedule();
+       if (signal_pending(current))
+               return -EINTR;
        return 0;
 }

Called from __wait_on_bit_lock.

Previously, signal_pending_state() was checked under
TASK_UNINTERRUPTIBLE (via prepare_to_wait_exclusive).  Now, we simply
check for the presence of any signal -- after we have returned to
running state, e.g. post io_schedule() when somebody has kicked the
wait-queue.

However, this now means that _wait_on_bit_lock can return -EINTR up to
__lock_page; which does not validate the return code and blindly
returns.  This looks to have been a previously existing bug, but it
was at least masked by the fact that it required a fatal signal
previously (and that the page we return unlocked is likely going to be
freed from the dying process anyway).

Peter's proposed follow-up above looks strictly more correct.  We need
to evaluate the potential existence of a signal, *after* we return
from schedule, but in the context of the state which we previously
_entered_ schedule() on.

Reviewed-by: Paul Turner <pjt@google.com>


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

#1289494 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Fri, Dec 11, 2015 at 03:30:33AM -0800, Paul Turner wrote:

> > Blergh, all I've managed to far is to confuse myself further. Even
> > something like the original (+- the EINTR) should work when we consider
> > the looping, even when mixed with an occasional spurious wakeup.
> >
> >
> > int bit_wait()
> > {
> >         if (signal_pending_state(current->state, current))
> >                 return -EINTR;
> >         schedule();
> > }

So I asked Vladimir to test that (simply changing the return from 1 to
-EINTR) and it made his fail much less likely but it still failed in the
same way.

So I'm fairly sure I'm still missing something :/

> Hugh asked me about this after seeing a crash, here's another exciting
> way in which the current code breaks -- this one actually quite
> serious:

Yep, this got reported by Jan and I did kick myself for that.

> Peter's proposed follow-up above looks strictly more correct.  We need
> to evaluate the potential existence of a signal, *after* we return
> from schedule, but in the context of the state which we previously
> _entered_ schedule() on.
> 
> Reviewed-by: Paul Turner <pjt@google.com>

Right, its maybe a bit overkill, but at this point I'm a tad
conservative/paranoid.

Vladimir, Jan could you both please that patch?

 lkml.kernel.org/r/20151208104712.GJ6356@twins.programming.kicks-ass.net


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]

#1289500 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On 11/12/15 11:39, Peter Zijlstra wrote:
> On Fri, Dec 11, 2015 at 03:30:33AM -0800, Paul Turner wrote:
> 
>>> Blergh, all I've managed to far is to confuse myself further. Even
>>> something like the original (+- the EINTR) should work when we consider
>>> the looping, even when mixed with an occasional spurious wakeup.
>>>
>>>
>>> int bit_wait()
>>> {
>>>         if (signal_pending_state(current->state, current))
>>>                 return -EINTR;
>>>         schedule();
>>> }
> 
> So I asked Vladimir to test that (simply changing the return from 1 to
> -EINTR) and it made his fail much less likely but it still failed in the
> same way.
> 
> So I'm fairly sure I'm still missing something :/
> 
>> Hugh asked me about this after seeing a crash, here's another exciting
>> way in which the current code breaks -- this one actually quite
>> serious:
> 
> Yep, this got reported by Jan and I did kick myself for that.
> 
>> Peter's proposed follow-up above looks strictly more correct.  We need
>> to evaluate the potential existence of a signal, *after* we return
>> from schedule, but in the context of the state which we previously
>> _entered_ schedule() on.
>>
>> Reviewed-by: Paul Turner <pjt@google.com>
> 
> Right, its maybe a bit overkill, but at this point I'm a tad
> conservative/paranoid.
> 
> Vladimir, Jan could you both please that patch?
> 
>  lkml.kernel.org/r/20151208104712.GJ6356@twins.programming.kicks-ass.net

Already in a queue!

Cheers
Vladimir

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

#1289548 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

----- Original Message -----
> From: "Peter Zijlstra" <peterz@infradead.org>
> To: "Paul Turner" <pjt@google.com>
> Cc: "NeilBrown" <nfbrown@novell.com>, "Linus Torvalds" <torvalds@linux-foundation.org>, "Thomas Gleixner"
> <tglx@linutronix.de>, "LKML" <linux-kernel@vger.kernel.org>, "Mike Galbraith" <efault@gmx.de>, "Ingo Molnar"
> <mingo@kernel.org>, "Peter Anvin" <hpa@zytor.com>, "vladimir murzin" <vladimir.murzin@arm.com>,
> linux-tip-commits@vger.kernel.org, jstancek@redhat.com, "Oleg Nesterov" <oleg@redhat.com>
> Sent: Friday, 11 December, 2015 12:39:59 PM
> Subject: Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers
> 
> On Fri, Dec 11, 2015 at 03:30:33AM -0800, Paul Turner wrote:
> 
> > > Blergh, all I've managed to far is to confuse myself further. Even
> > > something like the original (+- the EINTR) should work when we consider
> > > the looping, even when mixed with an occasional spurious wakeup.
> > >
> > >
> > > int bit_wait()
> > > {
> > >         if (signal_pending_state(current->state, current))
> > >                 return -EINTR;
> > >         schedule();
> > > }
> 
> So I asked Vladimir to test that (simply changing the return from 1 to
> -EINTR) and it made his fail much less likely but it still failed in the
> same way.
> 
> So I'm fairly sure I'm still missing something :/
> 
> > Hugh asked me about this after seeing a crash, here's another exciting
> > way in which the current code breaks -- this one actually quite
> > serious:
> 
> Yep, this got reported by Jan and I did kick myself for that.
> 
> > Peter's proposed follow-up above looks strictly more correct.  We need
> > to evaluate the potential existence of a signal, *after* we return
> > from schedule, but in the context of the state which we previously
> > _entered_ schedule() on.
> > 
> > Reviewed-by: Paul Turner <pjt@google.com>
> 
> Right, its maybe a bit overkill, but at this point I'm a tad
> conservative/paranoid.
> 
> Vladimir, Jan could you both please that patch?
> 
>  lkml.kernel.org/r/20151208104712.GJ6356@twins.programming.kicks-ass.net

This appears to exactly match patch I tested against v4.4-rc4 here:
  http://marc.info/?l=linux-mm&m=144950957622869&w=2

Anyway, I repeated the test with v4.4-rc4-113-g0bd0f1e as base.
Results look good. With patch applied, I can't trigger
"kernel BUG at mm/filemap.c:238!" anymore.

Regards,
Jan

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

#1289565 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On Fri, Dec 11, 2015 at 08:08:36AM -0500, Jan Stancek wrote:
> >  lkml.kernel.org/r/20151208104712.GJ6356@twins.programming.kicks-ass.net
> 
> This appears to exactly match patch I tested against v4.4-rc4 here:
>   http://marc.info/?l=linux-mm&m=144950957622869&w=2

Ah, I forgot I posted the thing twice and you already tested it :/

> Anyway, I repeated the test with v4.4-rc4-113-g0bd0f1e as base.
> Results look good. With patch applied, I can't trigger
> "kernel BUG at mm/filemap.c:238!" anymore.

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]

#1289783 — Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers

On 11/12/15 11:39, Peter Zijlstra wrote:
> On Fri, Dec 11, 2015 at 03:30:33AM -0800, Paul Turner wrote:
> 
>>> Blergh, all I've managed to far is to confuse myself further. Even
>>> something like the original (+- the EINTR) should work when we consider
>>> the looping, even when mixed with an occasional spurious wakeup.
>>>
>>>
>>> int bit_wait()
>>> {
>>>         if (signal_pending_state(current->state, current))
>>>                 return -EINTR;
>>>         schedule();
>>> }
> 
> So I asked Vladimir to test that (simply changing the return from 1 to
> -EINTR) and it made his fail much less likely but it still failed in the
> same way.
> 
> So I'm fairly sure I'm still missing something :/
> 
>> Hugh asked me about this after seeing a crash, here's another exciting
>> way in which the current code breaks -- this one actually quite
>> serious:
> 
> Yep, this got reported by Jan and I did kick myself for that.
> 
>> Peter's proposed follow-up above looks strictly more correct.  We need
>> to evaluate the potential existence of a signal, *after* we return
>> from schedule, but in the context of the state which we previously
>> _entered_ schedule() on.
>>
>> Reviewed-by: Paul Turner <pjt@google.com>
> 
> Right, its maybe a bit overkill, but at this point I'm a tad
> conservative/paranoid.
> 
> Vladimir, Jan could you both please that patch?
> 
>  lkml.kernel.org/r/20151208104712.GJ6356@twins.programming.kicks-ass.net
> 

By this time my test has been run ~500 times without any stalls. I'll
keep running overnight (just in case), but I think that patch can be
marked as tested.

Cheers
Vladimir

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

Back to top | Article view | linux.kernel

csiph-web