lkml.org 
[lkml]   [2016]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 03/11] locking, rwsem: introduce basis for down_write_killable
    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?

    \
     
     \ /
      Last update: 2016-05-10 15:01    [W:3.277 / U:0.232 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site