From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752256AbcEJN6x (ORCPT ); Tue, 10 May 2016 09:58:53 -0400 Received: from www262.sakura.ne.jp ([202.181.97.72]:47096 "EHLO www262.sakura.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751692AbcEJN6w (ORCPT ); Tue, 10 May 2016 09:58:52 -0400 To: peterz@infradead.org, mhocko@kernel.org Cc: linux-kernel@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de, hpa@zytor.com, davem@davemloft.net, tony.luck@intel.com, akpm@linux-foundation.org, chris@zankel.net, jcmvbkbc@gmail.com, dave@stgolabs.net, Waiman.Long@hpe.com Subject: Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable From: Tetsuo Handa References: <1459508695-14915-1-git-send-email-mhocko@kernel.org> <1459508695-14915-4-git-send-email-mhocko@kernel.org> <8bd03bdc-0373-a3bb-da12-045322efb797@I-love.SAKURA.ne.jp> <20160510115320.GJ23576@dhcp22.suse.cz> <20160510123806.GB3193@twins.programming.kicks-ass.net> In-Reply-To: <20160510123806.GB3193@twins.programming.kicks-ass.net> Message-Id: <201605102257.HBE12981.LOOJFQtOSMFFHV@I-love.SAKURA.ne.jp> X-Mailer: Winbiff [Version 2.51 PL2] X-Accept-Language: ja,en,zh Date: Tue, 10 May 2016 22:57:34 +0900 Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? >