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

Groups > linux.kernel > #1397985 > unrolled thread

Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable

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: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-10 12:50 +0200
    Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-10 14:00 +0200
      Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-10 14:40 +0200
        Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-10 16:00 +0200
        Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 09:30 +0200
          Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 10:30 +0200
            Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-11 10:50 +0200
              Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 11:10 +0200
                Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-11 11:20 +0200
                  Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 11:40 +0200
                    Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-11 11:50 +0200
                      Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 16:00 +0200
                        Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 20:10 +0200
                          [PATCH] locking, rwsem: Fix down_write_killable() Peter Zijlstra <peterz@infradead.org> - 2016-05-12 14:00 +0200
                            [tip:locking/rwsem] locking/rwsem: Fix down_write_killable() tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-05-12 14:20 +0200
                            Re: [PATCH] locking, rwsem: Fix down_write_killable() Michal Hocko <mhocko@kernel.org> - 2016-05-12 19:10 +0200
                            [tip:locking/rwsem] locking/rwsem: Fix down_write_killable() tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-05-16 04:10 +0200
                          Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-12 14:20 +0200
                            Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-12 16:00 +0200
                            Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable Waiman Long <waiman.long@hpe.com> - 2016-05-12 21:50 +0200
                          Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-12 14:20 +0200
          Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Peter Zijlstra <peterz@infradead.org> - 2016-05-11 10:40 +0200
            Re: [PATCH 03/11] locking, rwsem: introduce basis for  down_write_killable Michal Hocko <mhocko@kernel.org> - 2016-05-11 11:10 +0200

Page 1 of 2  [1] 2  Next page →

#1397985 — Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable

I hit "allowing the OOM killer to select the same thread again" problem
( http://lkml.kernel.org/r/20160408113425.GF29820@dhcp22.suse.cz ), but
I think that there is a bug in down_write_killable() series (at least
"locking, rwsem: introduce basis for down_write_killable" patch).

Complete log is at http://I-love.SAKURA.ne.jp/tmp/serial-20160510-sem.txt.xz .
----------
[   48.303867] Out of memory: Kill process 1314 (tgid=1314) score 1000 or sacrifice child
[   48.308582] Killed process 1314 (tgid=1314) total-vm:70844kB, anon-rss:1980kB, file-rss:0kB, shmem-rss:0kB
[   49.323719] oom_reaper: unable to reap pid:1314 (tgid=1314)
[   49.338146]
[   49.338146] Showing all locks held in the system:
(...snipped...)
[   49.801071] 1 lock held by tgid=1314/1314:
[   49.803953]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff810fddac>] acct_collect+0x5c/0x1e0
[   49.809200] 1 lock held by tgid=1314/1443:
[   49.812102]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff81073a45>] do_exit+0x175/0xb10
(...snipped...)
[   51.188928] oom_reaper: unable to reap pid:1443 (tgid=1314)
[   55.576750] oom_reaper: unable to reap pid:1314 (tgid=1314)
[   57.717917] oom_reaper: unable to reap pid:1314 (tgid=1314)
[   59.285880] oom_reaper: unable to reap pid:1314 (tgid=1314)
[   60.818697] oom_reaper: unable to reap pid:1314 (tgid=1314)
(...snipped...)
[  174.429572] tgid=1314       D ffff88003ad93b90     0  1314   1209 0x00100084
[  174.429573]  ffff88003ad93b90 ffff88003ad8f6b8 ffff880039c3a140 ffff88003ad8c080
[  174.429574]  ffff88003ad94000 ffff88003ad8f6a0 ffff88003ad8f6b8 0000000000000000
[  174.429575]  0000000000000008 ffff88003ad93ba8 ffffffff81616190 ffff88003ad8c080
[  174.429585] Call Trace:
[  174.429586]  [<ffffffff81616190>] schedule+0x30/0x80
[  174.429587]  [<ffffffff81619e26>] rwsem_down_read_failed+0xd6/0x140
[  174.429589]  [<ffffffff812dd6f8>] call_rwsem_down_read_failed+0x18/0x30
[  174.429590]  [<ffffffff816196dd>] down_read+0x3d/0x50
[  174.429592]  [<ffffffff810fddac>] ? acct_collect+0x5c/0x1e0
[  174.429593]  [<ffffffff810fddac>] acct_collect+0x5c/0x1e0
[  174.429594]  [<ffffffff81073ff5>] do_exit+0x725/0xb10
[  174.429594]  [<ffffffff81074467>] do_group_exit+0x47/0xc0
[  174.429596]  [<ffffffff8108075f>] get_signal+0x20f/0x7b0
[  174.429597]  [<ffffffff81024fb2>] do_signal+0x32/0x700
[  174.429598]  [<ffffffff810bdc69>] ? trace_hardirqs_on+0x9/0x10
[  174.429599]  [<ffffffff810c3552>] ? rwsem_wake+0x72/0xe0
[  174.429600]  [<ffffffff812dd78b>] ? call_rwsem_wake+0x1b/0x30
[  174.429601]  [<ffffffff810b9ee0>] ? up_read+0x30/0x40
[  174.429602]  [<ffffffff8106b495>] ? exit_to_usermode_loop+0x29/0x9e
[  174.429603]  [<ffffffff8106b4bf>] exit_to_usermode_loop+0x53/0x9e
[  174.429604]  [<ffffffff8100348d>] prepare_exit_to_usermode+0x7d/0x90
[  174.429605]  [<ffffffff8161bd3e>] retint_user+0x8/0x23
[  174.429605] tgid=1314       D ffff88003aa2fbd0     0  1443   1209 0x00000084
[  174.429607]  ffff88003aa2fbd0 ffff88003ad8f6b8 ffff8800382060c0 ffff88003aa2a140
[  174.429608]  ffff88003aa30000 ffff88003ad8f6a0 ffff88003ad8f6b8 ffff88003aa2a140
[  174.429609]  0000000000000008 ffff88003aa2fbe8 ffffffff81616190 ffff88003aa2a140
[  174.429610] Call Trace:
[  174.429611]  [<ffffffff81616190>] schedule+0x30/0x80
[  174.429612]  [<ffffffff81619e26>] rwsem_down_read_failed+0xd6/0x140
[  174.429613]  [<ffffffff810bdb99>] ? trace_hardirqs_on_caller+0xf9/0x1c0
[  174.429614]  [<ffffffff812dd6f8>] call_rwsem_down_read_failed+0x18/0x30
[  174.429615]  [<ffffffff816196dd>] down_read+0x3d/0x50
[  174.429616]  [<ffffffff81073a45>] ? do_exit+0x175/0xb10
[  174.429616]  [<ffffffff81073a45>] do_exit+0x175/0xb10
[  174.429617]  [<ffffffff81074467>] do_group_exit+0x47/0xc0
[  174.429618]  [<ffffffff8108075f>] get_signal+0x20f/0x7b0
[  174.429619]  [<ffffffff81024fb2>] do_signal+0x32/0x700
[  174.429620]  [<ffffffff8161acae>] ? _raw_spin_unlock_irq+0x2e/0x40
[  174.429621]  [<ffffffff8161a2bf>] ? rwsem_down_write_failed_killable+0x1ef/0x280
[  174.429631]  [<ffffffff8106b555>] ? syscall_slow_exit_work+0x4b/0x10d
[  174.429632]  [<ffffffff8106b495>] ? exit_to_usermode_loop+0x29/0x9e
[  174.429633]  [<ffffffff8106b4bf>] exit_to_usermode_loop+0x53/0x9e
[  174.429634]  [<ffffffff81003715>] do_syscall_64+0x135/0x1b0
[  174.429635]  [<ffffffff8161b43f>] entry_SYSCALL64_slow_path+0x25/0x25
(...snipped...)
[  217.651477] oom_reaper: unable to reap pid:1314 (tgid=1314)
[  219.071975] oom_reaper: unable to reap pid:1314 (tgid=1314)
[  220.508961] oom_reaper: unable to reap pid:1314 (tgid=1314)
[  222.022111] oom_reaper: unable to reap pid:1314 (tgid=1314)
[  223.560166] oom_reaper: unable to reap pid:1314 (tgid=1314)
[  225.267750] oom_reaper: unable to reap pid:1314 (tgid=1314)
----------

2 threads (PID: 1314 and 1443) are sleeping at rwsem_down_read_failed()
but no thread is sleeping at rwsem_down_write_failed_killable().
If there is no thread waiting for write lock, threads waiting for read
lock must be able to run. This suggests that one of threads which was
waiting for write lock forgot to wake up reader threads.

Looking at rwsem_down_read_failed(), reader threads waiting for the
writer thread to release the lock are waiting on sem->wait_list list.
Looking at __rwsem_down_write_failed_common(), when the writer thread
escaped the

                 /* Block until there are no active lockers. */
                 do {
                         if (signal_pending_state(state, current)) {
                                 raw_spin_lock_irq(&sem->wait_lock);
                                 ret = ERR_PTR(-EINTR);
                                 goto out;
                         }
                         schedule();
                         set_current_state(state);
                 } while ((count = sem->count) & RWSEM_ACTIVE_MASK);

loop due to SIGKILL, I think that the writer thread needs to check for
remaining threads on sem->wait_list list and wake up reader threads
before rwsem_down_write_failed_killable() returns -EINTR.

[toc] | [next] | [standalone]

#1398019

On Tue 10-05-16 19:43:20, Tetsuo Handa wrote:
> I hit "allowing the OOM killer to select the same thread again" problem
> ( http://lkml.kernel.org/r/20160408113425.GF29820@dhcp22.suse.cz ), but
> I think that there is a bug in down_write_killable() series (at least
> "locking, rwsem: introduce basis for down_write_killable" patch).
> 
> Complete log is at http://I-love.SAKURA.ne.jp/tmp/serial-20160510-sem.txt.xz .
[...]
> 2 threads (PID: 1314 and 1443) are sleeping at rwsem_down_read_failed()
> but no thread is sleeping at rwsem_down_write_failed_killable().
> If there is no thread waiting for write lock, threads waiting for read
> lock must be able to run. This suggests that one of threads which was
> waiting for write lock forgot to wake up reader threads.

Or that the write lock holder is still keeping the lock held. I do not
see such a process in your list though. Is it possible that the
debug_show_all_locks would just miss it as it is not sleeping?

> Looking at rwsem_down_read_failed(), reader threads waiting for the
> writer thread to release the lock are waiting on sem->wait_list list.
> Looking at __rwsem_down_write_failed_common(), when the writer thread
> escaped the
> 
>                  /* Block until there are no active lockers. */
>                  do {
>                          if (signal_pending_state(state, current)) {
>                                  raw_spin_lock_irq(&sem->wait_lock);
>                                  ret = ERR_PTR(-EINTR);
>                                  goto out;
>                          }
>                          schedule();
>                          set_current_state(state);
>                  } while ((count = sem->count) & RWSEM_ACTIVE_MASK);
> 
> loop due to SIGKILL, I think that the writer thread needs to check for
> remaining threads on sem->wait_list list and wake up reader threads
> before rwsem_down_write_failed_killable() returns -EINTR.

I am not sure I understand. The rwsem counter is not write locked while
the thread is sleeping and when we fail on the signal pending so readers
should be able to proceed, no?

Or are you suggesting that the failure path should call rwsem_wake? I
do not see __mutex_lock_common for killable wait doing something like
that and rwsem_wake is explicitly documented that it is called after the
lock state has been updated already. Now I might be missing something
subtle here but I guess the code is correct and it is more likely that
the holder of the lock wasn't killed but it is rather holding the lock
and doing something else.
-- 
Michal Hocko
SUSE Labs

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

#1398053

On Tue, May 10, 2016 at 01:53:20PM +0200, Michal Hocko wrote:
> On Tue 10-05-16 19:43:20, Tetsuo Handa wrote:
> > I hit "allowing the OOM killer to select the same thread again" problem
> > ( http://lkml.kernel.org/r/20160408113425.GF29820@dhcp22.suse.cz ), but
> > I think that there is a bug in down_write_killable() series (at least
> > "locking, rwsem: introduce basis for down_write_killable" patch).
> > 
> > Complete log is at http://I-love.SAKURA.ne.jp/tmp/serial-20160510-sem.txt.xz .
> [...]
> > 2 threads (PID: 1314 and 1443) are sleeping at rwsem_down_read_failed()
> > but no thread is sleeping at rwsem_down_write_failed_killable().
> > If there is no thread waiting for write lock, threads waiting for read
> > lock must be able to run. This suggests that one of threads which was
> > waiting for write lock forgot to wake up reader threads.
> 
> Or that the write lock holder is still keeping the lock held. I do not
> see such a process in your list though. Is it possible that the
> debug_show_all_locks would just miss it as it is not sleeping?
> 
> > Looking at rwsem_down_read_failed(), reader threads waiting for the
> > writer thread to release the lock are waiting on sem->wait_list list.
> > Looking at __rwsem_down_write_failed_common(), when the writer thread
> > escaped the
> > 
> >                  /* Block until there are no active lockers. */
> >                  do {
> >                          if (signal_pending_state(state, current)) {
> >                                  raw_spin_lock_irq(&sem->wait_lock);
> >                                  ret = ERR_PTR(-EINTR);
> >                                  goto out;
> >                          }
> >                          schedule();
> >                          set_current_state(state);
> >                  } while ((count = sem->count) & RWSEM_ACTIVE_MASK);
> > 
> > loop due to SIGKILL, I think that the writer thread needs to check for
> > remaining threads on sem->wait_list list and wake up reader threads
> > before rwsem_down_write_failed_killable() returns -EINTR.
> 
> I am not sure I understand. The rwsem counter is not write locked while
> the thread is sleeping and when we fail on the signal pending so readers
> should be able to proceed, no?
> 
> Or are you suggesting that the failure path should call rwsem_wake? I
> do not see __mutex_lock_common for killable wait doing something like
> that and rwsem_wake is explicitly documented that it is called after the
> lock state has been updated already. Now I might be missing something
> subtle here but I guess the code is correct and it is more likely that
> the holder of the lock wasn't killed but it is rather holding the lock
> and doing something else.

Mutex is much simpler; it doesn't have to do the reader-vs-writer
fairness thing.

However, at the time I was thinking that if we have:

	reader (owner)
	writer (pending)
	reader (blocked on writer)

and writer would get cancelled, the up_read() would do a wakeup and kick
the blocked reader.

But yes, immediately kicking further pending waiters might be better.

Also, looking at it again; I think we're forgetting to re-adjust the
BIAS for the cancelled writer.

Davidlohr, Waiman, can you look at this?

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

#1398123 — Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable

Michal Hocko wrote:
> On Tue 10-05-16 19:43:20, Tetsuo Handa wrote:
> > I hit "allowing the OOM killer to select the same thread again" problem
> > ( http://lkml.kernel.org/r/20160408113425.GF29820@dhcp22.suse.cz ), but
> > I think that there is a bug in down_write_killable() series (at least
> > "locking, rwsem: introduce basis for down_write_killable" patch).
> > 
> > Complete log is at http://I-love.SAKURA.ne.jp/tmp/serial-20160510-sem.txt.xz .
> [...]
> > 2 threads (PID: 1314 and 1443) are sleeping at rwsem_down_read_failed()
> > but no thread is sleeping at rwsem_down_write_failed_killable().
> > If there is no thread waiting for write lock, threads waiting for read
> > lock must be able to run. This suggests that one of threads which was
> > waiting for write lock forgot to wake up reader threads.
> 
> Or that the write lock holder is still keeping the lock held. I do not
> see such a process in your list though. Is it possible that the
> debug_show_all_locks would just miss it as it is not sleeping?

I don't think it is possible. This reproducer
( http://lkml.kernel.org/r/201605061958.HHG48967.JVFtSLFQOFOOMH@I-love.SAKURA.ne.jp )
creates a thread group with two threads, and two of these two threads are
sleeping at rwsem_down_read_failed() waiting for mmap_sem.
SysRq-t suggests that PID 1443 called rwsem_down_write_failed_killable()
before calling rwsem_down_read_failed().

By the way, I suggested you to show traces of threads which are using the OOM victim's mm
( http://lkml.kernel.org/r/201603172200.CIE52148.QOVSOHJFMLOFtF@I-love.SAKURA.ne.jp ),
but you said that showing all locks held by !TASK_RUNNING threads would be useful
( http://lkml.kernel.org/r/20160323120716.GE7059@dhcp22.suse.cz ).
Do you admit that debug_show_all_locks() is not always useful by suspecting
the possibility of debug_show_all_locks() failing to report a thread which
held mmap_sem for write? (This is a kmallocwd topic, so I stop here.)

> 
> > Looking at rwsem_down_read_failed(), reader threads waiting for the
> > writer thread to release the lock are waiting on sem->wait_list list.
> > Looking at __rwsem_down_write_failed_common(), when the writer thread
> > escaped the
> > 
> >                  /* Block until there are no active lockers. */
> >                  do {
> >                          if (signal_pending_state(state, current)) {
> >                                  raw_spin_lock_irq(&sem->wait_lock);
> >                                  ret = ERR_PTR(-EINTR);
> >                                  goto out;
> >                          }
> >                          schedule();
> >                          set_current_state(state);
> >                  } while ((count = sem->count) & RWSEM_ACTIVE_MASK);
> > 
> > loop due to SIGKILL, I think that the writer thread needs to check for
> > remaining threads on sem->wait_list list and wake up reader threads
> > before rwsem_down_write_failed_killable() returns -EINTR.
> 
> I am not sure I understand. The rwsem counter is not write locked while
> the thread is sleeping and when we fail on the signal pending so readers
> should be able to proceed, no?
> 
I guess __rwsem_do_wake() is needed for waking up the readers because
I guess the sequence occurred was

  (1) PID 1314 requested down_read() and succeeded.
  (2) PID 1443 requested down_write_killable() and blocked.
  (3) The OOM killer sent SIGKILL to PID 1314 and PID 1443.
  (4) PID 1443 left down_write_killable() with -EINTR.
  (5) PID 1314 called up_read() and down_read()
      while PID 1443 called down_read().

.

> Or are you suggesting that the failure path should call rwsem_wake?

I don't know how rwsem works. Please consult maintainers.

Peter Zijlstra wrote:
> Mutex is much simpler; it doesn't have to do the reader-vs-writer
> fairness thing.
> 
> However, at the time I was thinking that if we have:
> 
> 	reader (owner)
> 	writer (pending)
> 	reader (blocked on writer)
> 
> and writer would get cancelled, the up_read() would do a wakeup and kick
> the blocked reader.
> 
> But yes, immediately kicking further pending waiters might be better.
> 
> Also, looking at it again; I think we're forgetting to re-adjust the
> BIAS for the cancelled writer.

Yes, I think so.
> 
> Davidlohr, Waiman, can you look at this?
> 

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

#1398732

On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:
> On Tue, May 10, 2016 at 01:53:20PM +0200, Michal Hocko wrote:
> > On Tue 10-05-16 19:43:20, Tetsuo Handa wrote:
> > > I hit "allowing the OOM killer to select the same thread again" problem
> > > ( http://lkml.kernel.org/r/20160408113425.GF29820@dhcp22.suse.cz ), but
> > > I think that there is a bug in down_write_killable() series (at least
> > > "locking, rwsem: introduce basis for down_write_killable" patch).
> > > 
> > > Complete log is at http://I-love.SAKURA.ne.jp/tmp/serial-20160510-sem.txt.xz .
> > [...]
> > > 2 threads (PID: 1314 and 1443) are sleeping at rwsem_down_read_failed()
> > > but no thread is sleeping at rwsem_down_write_failed_killable().
> > > If there is no thread waiting for write lock, threads waiting for read
> > > lock must be able to run. This suggests that one of threads which was
> > > waiting for write lock forgot to wake up reader threads.
> > 
> > Or that the write lock holder is still keeping the lock held. I do not
> > see such a process in your list though. Is it possible that the
> > debug_show_all_locks would just miss it as it is not sleeping?
> > 
> > > Looking at rwsem_down_read_failed(), reader threads waiting for the
> > > writer thread to release the lock are waiting on sem->wait_list list.
> > > Looking at __rwsem_down_write_failed_common(), when the writer thread
> > > escaped the
> > > 
> > >                  /* Block until there are no active lockers. */
> > >                  do {
> > >                          if (signal_pending_state(state, current)) {
> > >                                  raw_spin_lock_irq(&sem->wait_lock);
> > >                                  ret = ERR_PTR(-EINTR);
> > >                                  goto out;
> > >                          }
> > >                          schedule();
> > >                          set_current_state(state);
> > >                  } while ((count = sem->count) & RWSEM_ACTIVE_MASK);
> > > 
> > > loop due to SIGKILL, I think that the writer thread needs to check for
> > > remaining threads on sem->wait_list list and wake up reader threads
> > > before rwsem_down_write_failed_killable() returns -EINTR.
> > 
> > I am not sure I understand. The rwsem counter is not write locked while
> > the thread is sleeping and when we fail on the signal pending so readers
> > should be able to proceed, no?
> > 
> > Or are you suggesting that the failure path should call rwsem_wake? I
> > do not see __mutex_lock_common for killable wait doing something like
> > that and rwsem_wake is explicitly documented that it is called after the
> > lock state has been updated already. Now I might be missing something
> > subtle here but I guess the code is correct and it is more likely that
> > the holder of the lock wasn't killed but it is rather holding the lock
> > and doing something else.
> 
> Mutex is much simpler; it doesn't have to do the reader-vs-writer
> fairness thing.
> 
> However, at the time I was thinking that if we have:
> 
> 	reader (owner)
> 	writer (pending)
> 	reader (blocked on writer)
> 
> and writer would get cancelled, the up_read() would do a wakeup and kick
> the blocked reader.
> 
> But yes, immediately kicking further pending waiters might be better.

OK, that makes sense. We shouldn't be waiting for the first reader to
do up_read.

> Also, looking at it again; I think we're forgetting to re-adjust the
> BIAS for the cancelled writer.

Hmm, __rwsem_down_write_failed_common does

	/* undo write bias from down_write operation, stop active locking */
	count = rwsem_atomic_update(-RWSEM_ACTIVE_WRITE_BIAS, sem);

which should remove the bias AFAIU. Later we do

	if (waiting) {
		count = READ_ONCE(sem->count);

		/*
		 * If there were already threads queued before us and there are
		 * no active writers, the lock must be read owned; so we try to
		 * wake any read locks that were queued ahead of us.
		 */
		if (count > RWSEM_WAITING_BIAS)
			sem = __rwsem_do_wake(sem, RWSEM_WAKE_READERS);

	} else
		count = rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);

and that might set RWSEM_WAITING_BIAS but the current holder of the lock
should handle that correctly and wake the waiting tasks IIUC. I will go
and check the code closer. It is quite easy to get this subtle code
wrong...
-- 
Michal Hocko
SUSE Labs

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

#1398782

On Wed 11-05-16 09:23:57, Michal Hocko wrote:
> On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:
[...]
> > However, at the time I was thinking that if we have:
> > 
> > 	reader (owner)
> > 	writer (pending)
> > 	reader (blocked on writer)
> > 
> > and writer would get cancelled, the up_read() would do a wakeup and kick
> > the blocked reader.
> > 
> > But yes, immediately kicking further pending waiters might be better.
> 
> OK, that makes sense. We shouldn't be waiting for the first reader to
> do up_read.

I am still trying to wrap my head around the wake up logic (I managed to
forget all the juicy details). I still do not see the correctness
issue with the current code so the following should be "just an
optimization". But as I've said I might be missing something here.

Does the following look correct/reasonable? This is absolutely untested
and more for a discussion:
---
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..726620c97366 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -516,7 +516,27 @@ EXPORT_SYMBOL(rwsem_down_write_failed);
 __visible struct rw_semaphore * __sched
 rwsem_down_write_failed_killable(struct rw_semaphore *sem)
 {
-	return __rwsem_down_write_failed_common(sem, TASK_KILLABLE);
+	struct rw_semaphore * ret;
+
+	ret = __rwsem_down_write_failed_common(sem, TASK_KILLABLE);
+	if (IS_ERR(ret)) {
+		long count;
+
+		raw_spin_lock_irq(&sem->wait_lock);
+		count = READ_ONCE(sem->count);
+
+		/*
+		 * If there were already threads queued before us and there are
+		 * no active writers, the lock must be read owned; so we try to
+		 * wake any read locks that were queued ahead of us so they
+		 * do not have to wait for up_read to wake up.
+		 */
+		if (count > RWSEM_WAITING_BIAS)
+			sem = __rwsem_do_wake(sem, RWSEM_WAKE_READERS);
+		raw_spin_unlock_irq(&sem->wait_lock);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(rwsem_down_write_failed_killable);
 
-- 
Michal Hocko
SUSE Labs

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

#1398798

On Wed, May 11, 2016 at 10:28:53AM +0200, Michal Hocko wrote:

> Does the following look correct/reasonable? This is absolutely untested
> and more for a discussion:

I would much rather see it in common; something like so perhaps.

---
 kernel/locking/rwsem-xadd.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..5d7f2831a475 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -487,11 +487,9 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 
 		/* Block until there are no active lockers. */
 		do {
-			if (signal_pending_state(state, current)) {
-				raw_spin_lock_irq(&sem->wait_lock);
-				ret = ERR_PTR(-EINTR);
-				goto out;
-			}
+			if (signal_pending_state(state, current))
+				goto out_nolock;
+
 			schedule();
 			set_current_state(state);
 		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
@@ -504,6 +502,18 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	return ret;
+
+out_nolock:
+	__set_current_state(TASK_RUNNING);
+	raw_spin_lock_irq(&sem->wait_lock);
+	list_del(&waiter.list);
+	if (list_empty(&sem->wait_list))
+		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
+	else
+		__rwsem_do_wake(sem, RWSEM_WAKE_READERS);
+	raw_spin_unlock_irq(&sem->wait_lock);
+
+	return ERR_PTR(-EINTR);
 }
 
 __visible struct rw_semaphore * __sched

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

#1398809

On Wed 11-05-16 10:44:01, Peter Zijlstra wrote:
[...]
> @@ -504,6 +502,18 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
>  	raw_spin_unlock_irq(&sem->wait_lock);
>  
>  	return ret;
> +
> +out_nolock:
> +	__set_current_state(TASK_RUNNING);
> +	raw_spin_lock_irq(&sem->wait_lock);
> +	list_del(&waiter.list);
> +	if (list_empty(&sem->wait_list))
> +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> +	else
> +		__rwsem_do_wake(sem, RWSEM_WAKE_READERS);
> +	raw_spin_unlock_irq(&sem->wait_lock);
> +
> +	return ERR_PTR(-EINTR);
>  }

Looks much better but don't we have to check the count for potentially
pending writers?

-- 
Michal Hocko
SUSE Labs

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

#1398827

On Wed, May 11, 2016 at 11:04:42AM +0200, Michal Hocko wrote:
> On Wed 11-05-16 10:44:01, Peter Zijlstra wrote:
> [...]
> > @@ -504,6 +502,18 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
> >  	raw_spin_unlock_irq(&sem->wait_lock);
> >  
> >  	return ret;
> > +
> > +out_nolock:
> > +	__set_current_state(TASK_RUNNING);
> > +	raw_spin_lock_irq(&sem->wait_lock);
> > +	list_del(&waiter.list);
> > +	if (list_empty(&sem->wait_list))
> > +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> > +	else
> > +		__rwsem_do_wake(sem, RWSEM_WAKE_READERS);
> > +	raw_spin_unlock_irq(&sem->wait_lock);
> > +
> > +	return ERR_PTR(-EINTR);
> >  }
> 
> Looks much better but don't we have to check the count for potentially
> pending writers?

Ah, so I was thinking that if we get here, there must still be an owner,
otherwise we'd have acquired the lock. And if there is an owner, we
cannot go wake writers. Hence the WAKE_READERS thing.

Then again, WAKE_ANY would not harm, in that if we do wake a pending
writer it will not proceed if it cannot and it'll go back to sleep
again.

So yeah, maybe WAKE_ANY is the prudent thing to do.

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

#1398856

On Wed 11-05-16 11:17:33, Peter Zijlstra wrote:
> On Wed, May 11, 2016 at 11:04:42AM +0200, Michal Hocko wrote:
> > On Wed 11-05-16 10:44:01, Peter Zijlstra wrote:
> > [...]
> > > @@ -504,6 +502,18 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
> > >  	raw_spin_unlock_irq(&sem->wait_lock);
> > >  
> > >  	return ret;
> > > +
> > > +out_nolock:
> > > +	__set_current_state(TASK_RUNNING);
> > > +	raw_spin_lock_irq(&sem->wait_lock);
> > > +	list_del(&waiter.list);
> > > +	if (list_empty(&sem->wait_list))
> > > +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> > > +	else
> > > +		__rwsem_do_wake(sem, RWSEM_WAKE_READERS);
> > > +	raw_spin_unlock_irq(&sem->wait_lock);
> > > +
> > > +	return ERR_PTR(-EINTR);
> > >  }
> > 
> > Looks much better but don't we have to check the count for potentially
> > pending writers?
> 
> Ah, so I was thinking that if we get here, there must still be an owner,
> otherwise we'd have acquired the lock. And if there is an owner, we
> cannot go wake writers. Hence the WAKE_READERS thing.

I was worried about the case where the owner is writer and we would wake
readers but I have missed that this wouldn't happen because of

	if (wake_type != RWSEM_WAKE_READ_OWNED) {
		adjustment = RWSEM_ACTIVE_READ_BIAS;
 try_reader_grant:
		oldcount = rwsem_atomic_update(adjustment, sem) - adjustment;
		if (unlikely(oldcount < RWSEM_WAITING_BIAS)) {
			/* A writer stole the lock. Undo our reader grant. */
			if (rwsem_atomic_update(-adjustment, sem) &
						RWSEM_ACTIVE_MASK)
				goto out;
			/* Last active locker left. Retry waking readers. */
			goto try_reader_grant;
		}
	}

> Then again, WAKE_ANY would not harm, in that if we do wake a pending
> writer it will not proceed if it cannot and it'll go back to sleep
> again.

true

> So yeah, maybe WAKE_ANY is the prudent thing to do.

I guess so.

Care to cook up a full patch?

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1398881

On Wed, May 11, 2016 at 11:31:27AM +0200, Michal Hocko wrote:

> Care to cook up a full patch?

compile tested only, if someone could please test it?

I'll go write a Changelog if it turns out to actually work ;-)

---
 kernel/locking/rwsem-xadd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..09e30c6225e5 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 
 		/* Block until there are no active lockers. */
 		do {
-			if (signal_pending_state(state, current)) {
-				raw_spin_lock_irq(&sem->wait_lock);
-				ret = ERR_PTR(-EINTR);
-				goto out;
-			}
+			if (signal_pending_state(state, current))
+				goto out_nolock;
+
 			schedule();
 			set_current_state(state);
 		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
 
 		raw_spin_lock_irq(&sem->wait_lock);
 	}
-out:
 	__set_current_state(TASK_RUNNING);
 	list_del(&waiter.list);
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	return ret;
+
+out_nolock:
+	__set_current_state(TASK_RUNNING);
+	raw_spin_lock_irq(&sem->wait_lock);
+	list_del(&waiter.list);
+	if (list_empty(&sem->wait_list))
+		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
+	else
+		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
+	raw_spin_unlock_irq(&sem->wait_lock);
+
+	return ERR_PTR(-EINTR);
 }
 
 __visible struct rw_semaphore * __sched

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

#1399102

On Wed 11-05-16 11:41:28, Peter Zijlstra wrote:
> On Wed, May 11, 2016 at 11:31:27AM +0200, Michal Hocko wrote:
> 
> > Care to cook up a full patch?
> 
> compile tested only, if someone could please test it?

I have tried to run the test case from Tetsuo[1] with a small printk to
show the interrupted writer case:
[ 2753.596678] XXX: Writer interrupted. Woken waiters:0
[ 2998.266978] XXX: Writer interrupted. Woken waiters:0

which means rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem) path which is
the problematic case. oom_reaper was always able to succeed so I guess
the patch works as expected. I will leave the test run for longer to be
sure.

[1] http://lkml.kernel.org/r/201605061958.HHG48967.JVFtSLFQOFOOMH@I-love.SAKURA.ne.jp
> 
> I'll go write a Changelog if it turns out to actually work ;-)
> 
> ---
>  kernel/locking/rwsem-xadd.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index df4dcb883b50..09e30c6225e5 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
>  
>  		/* Block until there are no active lockers. */
>  		do {
> -			if (signal_pending_state(state, current)) {
> -				raw_spin_lock_irq(&sem->wait_lock);
> -				ret = ERR_PTR(-EINTR);
> -				goto out;
> -			}
> +			if (signal_pending_state(state, current))
> +				goto out_nolock;
> +
>  			schedule();
>  			set_current_state(state);
>  		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
>  
>  		raw_spin_lock_irq(&sem->wait_lock);
>  	}
> -out:
>  	__set_current_state(TASK_RUNNING);
>  	list_del(&waiter.list);
>  	raw_spin_unlock_irq(&sem->wait_lock);
>  
>  	return ret;
> +
> +out_nolock:
> +	__set_current_state(TASK_RUNNING);
> +	raw_spin_lock_irq(&sem->wait_lock);
> +	list_del(&waiter.list);
> +	if (list_empty(&sem->wait_list))
> +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> +	else
> +		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
> +	raw_spin_unlock_irq(&sem->wait_lock);
> +
> +	return ERR_PTR(-EINTR);
>  }
>  
>  __visible struct rw_semaphore * __sched

-- 
Michal Hocko
SUSE Labs

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

#1399396

On Wed 11-05-16 15:59:38, Michal Hocko wrote:
> On Wed 11-05-16 11:41:28, Peter Zijlstra wrote:
> > On Wed, May 11, 2016 at 11:31:27AM +0200, Michal Hocko wrote:
> > 
> > > Care to cook up a full patch?
> > 
> > compile tested only, if someone could please test it?
> 
> I have tried to run the test case from Tetsuo[1] with a small printk to
> show the interrupted writer case:
> [ 2753.596678] XXX: Writer interrupted. Woken waiters:0
> [ 2998.266978] XXX: Writer interrupted. Woken waiters:0
> 
> which means rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem) path which is
> the problematic case. oom_reaper was always able to succeed so I guess
> the patch works as expected. I will leave the test run for longer to be
> sure.

And just for the reference I am able to reproduce the lockup without the
patch applied and the same test case and a debugging patch

[ 1522.036379] XXX interrupted. list_is_singular:1
[ 1523.040462] oom_reaper: unable to reap pid:3736 (tgid=3736)

I still cannot say I would understand why the pending
RWSEM_WAITING_BIAS matters but I would probably need to look at the code
again with a clean head, __rwsem_wake is quite tricky...
-- 
Michal Hocko
SUSE Labs

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

#1399961 — [PATCH] locking, rwsem: Fix down_write_killable()

On Wed, May 11, 2016 at 08:03:46PM +0200, Michal Hocko wrote:
> On Wed 11-05-16 15:59:38, Michal Hocko wrote:
> > On Wed 11-05-16 11:41:28, Peter Zijlstra wrote:
> > > On Wed, May 11, 2016 at 11:31:27AM +0200, Michal Hocko wrote:
> > > 
> > > > Care to cook up a full patch?
> > > 
> > > compile tested only, if someone could please test it?
> > 
> > I have tried to run the test case from Tetsuo[1] with a small printk to
> > show the interrupted writer case:
> > [ 2753.596678] XXX: Writer interrupted. Woken waiters:0
> > [ 2998.266978] XXX: Writer interrupted. Woken waiters:0
> > 
> > which means rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem) path which is
> > the problematic case. oom_reaper was always able to succeed so I guess
> > the patch works as expected. I will leave the test run for longer to be
> > sure.
> 
> And just for the reference I am able to reproduce the lockup without the
> patch applied and the same test case and a debugging patch
> 
> [ 1522.036379] XXX interrupted. list_is_singular:1
> [ 1523.040462] oom_reaper: unable to reap pid:3736 (tgid=3736)

Here the complete patch. Ingo could you make it appear in the right tip
branch?


---
Subject: locking, rwsem: Fix down_write_killable()
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed, 11 May 2016 11:41:28 +0200

The new signal_pending exit path in __rwsem_down_write_failed_common()
was fingered as breaking his kernel by Tetsuo Handa.

Upon inspection it was found that there are two things wrong with it;

 - it forgets to remove WAITING_BIAS if it leaves the list empty, or
 - it forgets to wake further waiters that were blocked on the now
   removed waiter.

Especially the first issue causes new lock attempts to block and stall
indefinitely, as the code assumes that pending waiters mean there is
an owner that will wake when it releases the lock.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Waiman Long <Waiman.Long@hpe.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Tony Luck <tony.luck@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/locking/rwsem-xadd.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct
 
 		/* Block until there are no active lockers. */
 		do {
-			if (signal_pending_state(state, current)) {
-				raw_spin_lock_irq(&sem->wait_lock);
-				ret = ERR_PTR(-EINTR);
-				goto out;
-			}
+			if (signal_pending_state(state, current))
+				goto out_nolock;
+
 			schedule();
 			set_current_state(state);
 		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
 
 		raw_spin_lock_irq(&sem->wait_lock);
 	}
-out:
 	__set_current_state(TASK_RUNNING);
 	list_del(&waiter.list);
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	return ret;
+
+out_nolock:
+	__set_current_state(TASK_RUNNING);
+	raw_spin_lock_irq(&sem->wait_lock);
+	list_del(&waiter.list);
+	if (list_empty(&sem->wait_list))
+		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
+	else
+		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
+	raw_spin_unlock_irq(&sem->wait_lock);
+
+	return ERR_PTR(-EINTR);
 }
 
 __visible struct rw_semaphore * __sched

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

#1400004 — [tip:locking/rwsem] locking/rwsem: Fix down_write_killable()

Commit-ID:  6d4acadc1ca37a3aca10892d32fb7205bc5b0cfc
Gitweb:     http://git.kernel.org/tip/6d4acadc1ca37a3aca10892d32fb7205bc5b0cfc
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 12 May 2016 13:57:45 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 12 May 2016 14:10:39 +0200

locking/rwsem: Fix down_write_killable()

The new signal_pending exit path in __rwsem_down_write_failed_common()
was fingered as breaking his kernel by Tetsuo Handa.

Upon inspection it was found that there are two things wrong with it;

 - it forgets to remove WAITING_BIAS if it leaves the list empty, or
 - it forgets to wake further waiters that were blocked on the now
   removed waiter.

Especially the first issue causes new lock attempts to block and stall
indefinitely, as the code assumes that pending waiters mean there is
an owner that will wake when it releases the lock.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: Waiman Long <Waiman.Long@hpe.com>
Link: http://lkml.kernel.org/r/20160512115745.GP3192@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/rwsem-xadd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb8..09e30c6 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 
 		/* Block until there are no active lockers. */
 		do {
-			if (signal_pending_state(state, current)) {
-				raw_spin_lock_irq(&sem->wait_lock);
-				ret = ERR_PTR(-EINTR);
-				goto out;
-			}
+			if (signal_pending_state(state, current))
+				goto out_nolock;
+
 			schedule();
 			set_current_state(state);
 		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
 
 		raw_spin_lock_irq(&sem->wait_lock);
 	}
-out:
 	__set_current_state(TASK_RUNNING);
 	list_del(&waiter.list);
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	return ret;
+
+out_nolock:
+	__set_current_state(TASK_RUNNING);
+	raw_spin_lock_irq(&sem->wait_lock);
+	list_del(&waiter.list);
+	if (list_empty(&sem->wait_list))
+		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
+	else
+		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
+	raw_spin_unlock_irq(&sem->wait_lock);
+
+	return ERR_PTR(-EINTR);
 }
 
 __visible struct rw_semaphore * __sched

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

#1400245 — Re: [PATCH] locking, rwsem: Fix down_write_killable()

On Thu 12-05-16 13:57:45, Peter Zijlstra wrote:
[...]
> Subject: locking, rwsem: Fix down_write_killable()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed, 11 May 2016 11:41:28 +0200
> 
> The new signal_pending exit path in __rwsem_down_write_failed_common()
> was fingered as breaking his kernel by Tetsuo Handa.
> 
> Upon inspection it was found that there are two things wrong with it;
> 
>  - it forgets to remove WAITING_BIAS if it leaves the list empty, or
>  - it forgets to wake further waiters that were blocked on the now
>    removed waiter.
> 
> Especially the first issue causes new lock attempts to block and stall
> indefinitely, as the code assumes that pending waiters mean there is
> an owner that will wake when it releases the lock.

Just to prevent from confusion. I think that the patch is doing clearly
the proper thing. Even if the state WAITING_BIAS was OK for some reason
it would be too subtle and it is better to clean up the state when
failing. So the follow up discussion is just to clarify what the heck
was going on here.

> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Waiman Long <Waiman.Long@hpe.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/locking/rwsem-xadd.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct
>  
>  		/* Block until there are no active lockers. */
>  		do {
> -			if (signal_pending_state(state, current)) {
> -				raw_spin_lock_irq(&sem->wait_lock);
> -				ret = ERR_PTR(-EINTR);
> -				goto out;
> -			}
> +			if (signal_pending_state(state, current))
> +				goto out_nolock;
> +
>  			schedule();
>  			set_current_state(state);
>  		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
>  
>  		raw_spin_lock_irq(&sem->wait_lock);
>  	}
> -out:
>  	__set_current_state(TASK_RUNNING);
>  	list_del(&waiter.list);
>  	raw_spin_unlock_irq(&sem->wait_lock);
>  
>  	return ret;
> +
> +out_nolock:
> +	__set_current_state(TASK_RUNNING);
> +	raw_spin_lock_irq(&sem->wait_lock);
> +	list_del(&waiter.list);
> +	if (list_empty(&sem->wait_list))
> +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> +	else
> +		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
> +	raw_spin_unlock_irq(&sem->wait_lock);
> +
> +	return ERR_PTR(-EINTR);
>  }
>  
>  __visible struct rw_semaphore * __sched

-- 
Michal Hocko
SUSE Labs

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

#1401256 — [tip:locking/rwsem] locking/rwsem: Fix down_write_killable()

Commit-ID:  04cafed7fc19a8010771c788708ac97c405fc3de
Gitweb:     http://git.kernel.org/tip/04cafed7fc19a8010771c788708ac97c405fc3de
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 12 May 2016 13:57:45 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 15 May 2016 22:55:00 +0200

locking/rwsem: Fix down_write_killable()

The new signal_pending exit path in __rwsem_down_write_failed_common()
was fingered as breaking his kernel by Tetsuo Handa.

Upon inspection it was found that there are two things wrong with it;

 - it forgets to remove WAITING_BIAS if it leaves the list empty, or
 - it forgets to wake further waiters that were blocked on the now
   removed waiter.

Especially the first issue causes new lock attempts to block and stall
indefinitely, as the code assumes that pending waiters mean there is
an owner that will wake when it releases the lock.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: Waiman Long <Waiman.Long@hpe.com>
Link: http://lkml.kernel.org/r/20160512115745.GP3192@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/rwsem-xadd.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb8..09e30c6 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 
 		/* Block until there are no active lockers. */
 		do {
-			if (signal_pending_state(state, current)) {
-				raw_spin_lock_irq(&sem->wait_lock);
-				ret = ERR_PTR(-EINTR);
-				goto out;
-			}
+			if (signal_pending_state(state, current))
+				goto out_nolock;
+
 			schedule();
 			set_current_state(state);
 		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
 
 		raw_spin_lock_irq(&sem->wait_lock);
 	}
-out:
 	__set_current_state(TASK_RUNNING);
 	list_del(&waiter.list);
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	return ret;
+
+out_nolock:
+	__set_current_state(TASK_RUNNING);
+	raw_spin_lock_irq(&sem->wait_lock);
+	list_del(&waiter.list);
+	if (list_empty(&sem->wait_list))
+		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
+	else
+		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
+	raw_spin_unlock_irq(&sem->wait_lock);
+
+	return ERR_PTR(-EINTR);
 }
 
 __visible struct rw_semaphore * __sched

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

#1400001

On Thu 12-05-16 14:12:04, Peter Zijlstra wrote:
> On Wed, May 11, 2016 at 08:03:46PM +0200, Michal Hocko wrote:
> > I still cannot say I would understand why the pending
> > RWSEM_WAITING_BIAS matters but I would probably need to look at the code
> > again with a clean head, __rwsem_wake is quite tricky...
> 
> Ah, you're asking why an unconditional __rwsem_wake(ANY) isn't enough?
> 
> Because; if at that point there's nobody waiting, we're left with an
> empty list and WAITER_BIAS set. This in turn will make all fast paths
> fail.
> 
> Look at rwsem_down_read_failed() for instance; if we enter that we'll
> unconditionally queue ourself, with nobody left to come wake us.

This is still not clear to me because rwsem_down_read_failed will call
__rwsem_do_wake if the count is RWSEM_WAITING_BIAS so we shouldn't go to
sleep and get the lock. So you are right that we would force everybody
to the slow path which is not great but shouldn't cause incorrect
behavior. I guess I must be missing something obvious here...
-- 
Michal Hocko
SUSE Labs

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

#1400099

On Thu, May 12, 2016 at 02:19:07PM +0200, Michal Hocko wrote:
> On Thu 12-05-16 14:12:04, Peter Zijlstra wrote:
> > On Wed, May 11, 2016 at 08:03:46PM +0200, Michal Hocko wrote:
> > > I still cannot say I would understand why the pending
> > > RWSEM_WAITING_BIAS matters but I would probably need to look at the code
> > > again with a clean head, __rwsem_wake is quite tricky...
> > 
> > Ah, you're asking why an unconditional __rwsem_wake(ANY) isn't enough?
> > 
> > Because; if at that point there's nobody waiting, we're left with an
> > empty list and WAITER_BIAS set. This in turn will make all fast paths
> > fail.
> > 
> > Look at rwsem_down_read_failed() for instance; if we enter that we'll
> > unconditionally queue ourself, with nobody left to come wake us.
> 
> This is still not clear to me because rwsem_down_read_failed will call
> __rwsem_do_wake if the count is RWSEM_WAITING_BIAS so we shouldn't go to
> sleep and get the lock. So you are right that we would force everybody
> to the slow path which is not great but shouldn't cause incorrect
> behavior. I guess I must be missing something obvious here...


Ah me too; I missed the obvious: we do the __rwsem_do_wake() after we
add ourselves to the list, which means we'll also wake ourselves.

I'll have more thinking..

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

#1400313 — Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable

On 05/12/2016 08:19 AM, Michal Hocko wrote:
> On Thu 12-05-16 14:12:04, Peter Zijlstra wrote:
>> On Wed, May 11, 2016 at 08:03:46PM +0200, Michal Hocko wrote:
>>> I still cannot say I would understand why the pending
>>> RWSEM_WAITING_BIAS matters but I would probably need to look at the code
>>> again with a clean head, __rwsem_wake is quite tricky...
>> Ah, you're asking why an unconditional __rwsem_wake(ANY) isn't enough?
>>
>> Because; if at that point there's nobody waiting, we're left with an
>> empty list and WAITER_BIAS set. This in turn will make all fast paths
>> fail.
>>
>> Look at rwsem_down_read_failed() for instance; if we enter that we'll
>> unconditionally queue ourself, with nobody left to come wake us.
> This is still not clear to me because rwsem_down_read_failed will call
> __rwsem_do_wake if the count is RWSEM_WAITING_BIAS so we shouldn't go to
> sleep and get the lock. So you are right that we would force everybody
> to the slow path which is not great but shouldn't cause incorrect
> behavior. I guess I must be missing something obvious here...

Because of writer lock stealing, having a count of RWSEM_WAITING_BIAS 
doesn't mean the reader can surely get the lock even if it is the first 
one in the queue. Calling __rwsem_do_wake() will take care of all the 
locking and queue checking work. Yes, I think it is a bit odd for the 
possibility that a task may wake up itself. Maybe we can add code like:

--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -202,7 +202,8 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum 
rwsem_wake_type wake_type)
           */
          smp_mb();
          waiter->task = NULL;
-        wake_up_process(tsk);
+        if (tsk != current)
+            wake_up_process(tsk);
          put_task_struct(tsk);
      } while (--loop);

Cheers,
Longman

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web