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/[email protected] ), 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/[email protected] ) 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/[email protected] ), but you said that showing all locks held by !TASK_RUNNING threads would be useful ( http://lkml.kernel.org/r/[email protected] ). 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? >
