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

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

#1398787

On Wed, May 11, 2016 at 09:23:57AM +0200, Michal Hocko wrote:
> On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:

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

Right; at this point we're neutral wrt bias.

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

Subtle; yes.

So if you look at rwsem_try_write_lock() -- traditionally the only way
to exit this wait loop, you see it does:

	if (count == RWSEM_WAITING_BIAS &&
	    cmpxchg_acquire(&sem->count, RWSEM_WAITING_BIAS,
		    RWSEM_ACTIVE_WRITE_BIAS) == RWSEM_WAITING_BIAS) {
		if (!list_is_singular(&sem->wait_list))
			rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
		rwsem_set_owner(sem);
		return true;
	}

Which ends up clearing RWSEM_WAITING_BIAS is we were the only waiter --
or rather, it always clear WAITING, but then tests the list and re-sets
it if there's more than one waiters on.

Now, the signal break doesn't clear WAITING if we were the only waiter
on the list; which means any further down_read() will block (I didn't
look at what a subsequent down_write() would do).

So I think we needs something like this, to clear WAITING if we leave
the list empty.

Does that make sense?

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..7011dd1c286c 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -489,6 +489,8 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 		do {
 			if (signal_pending_state(state, current)) {
 				raw_spin_lock_irq(&sem->wait_lock);
+				if (list_singular(&sem->wait_list))
+					rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
 				ret = ERR_PTR(-EINTR);
 				goto out;
 			}

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

#1398805

On Wed 11-05-16 10:35:12, Peter Zijlstra wrote:
> On Wed, May 11, 2016 at 09:23:57AM +0200, Michal Hocko wrote:
> > On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:
> 
> > > 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.
> 
> Right; at this point we're neutral wrt bias.
> 
> > 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..
> 
> Subtle; yes.
> 
> So if you look at rwsem_try_write_lock() -- traditionally the only way
> to exit this wait loop, you see it does:
> 
> 	if (count == RWSEM_WAITING_BIAS &&
> 	    cmpxchg_acquire(&sem->count, RWSEM_WAITING_BIAS,
> 		    RWSEM_ACTIVE_WRITE_BIAS) == RWSEM_WAITING_BIAS) {
> 		if (!list_is_singular(&sem->wait_list))
> 			rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
> 		rwsem_set_owner(sem);
> 		return true;
> 	}
> 
> Which ends up clearing RWSEM_WAITING_BIAS is we were the only waiter --
> or rather, it always clear WAITING, but then tests the list and re-sets
> it if there's more than one waiters on.
> 
> Now, the signal break doesn't clear WAITING if we were the only waiter
> on the list; which means any further down_read() will block (I didn't
> look at what a subsequent down_write() would do).

I was staring at this part as well but then I convinced myself that this
is OK because rwsem_down_read_failed does:

	if (list_empty(&sem->wait_list))
		adjustment += RWSEM_WAITING_BIAS;
	list_add_tail(&waiter.list, &sem->wait_list);

	/* we're now waiting on the lock, but no longer actively locking */
	count = rwsem_atomic_update(adjustment, sem);

	/* If there are no active locks, wake the front queued process(es).
	 *
	 * If there are no writers and we are first in the queue,
	 * wake our own waiter to join the existing active readers !
	 */
	if (count == RWSEM_WAITING_BIAS ||
	    (count > RWSEM_WAITING_BIAS &&
	     adjustment != -RWSEM_ACTIVE_READ_BIAS))
		sem = __rwsem_do_wake(sem, RWSEM_WAKE_ANY);

__rwsem_do_wake should then see all the readers (including the current
one) and wake them up and set waiter->task to NULL to allow the current
one to break out of the loop as well.

> So I think we needs something like this, to clear WAITING if we leave
> the list empty.

But maybe this is the correct way to go.

> Does that make sense?

I do not see an immediate problem with this. Maybe Tetsuo can try this
out and see if it really makes a difference.

> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index df4dcb883b50..7011dd1c286c 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -489,6 +489,8 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
>  		do {
>  			if (signal_pending_state(state, current)) {
>  				raw_spin_lock_irq(&sem->wait_lock);
> +				if (list_singular(&sem->wait_list))
> +					rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
>  				ret = ERR_PTR(-EINTR);
>  				goto out;
>  			}

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web