On Thu 12-05-16 13:57:45, Peter Zijlstra wrote: [...] > Subject: locking, rwsem: Fix down_write_killable() > From: Peter Zijlstra <[email protected]> > 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" <[email protected]> > Cc: Waiman Long <[email protected]> > Cc: Chris Zankel <[email protected]> > Cc: Ingo Molnar <[email protected]> > Cc: Andrew Morton <[email protected]> > Cc: Thomas Gleixner <[email protected]> > Cc: Davidlohr Bueso <[email protected]> > Cc: Tony Luck <[email protected]> > Cc: "H. Peter Anvin" <[email protected]> > Cc: Max Filippov <[email protected]> > Reported-by: Tetsuo Handa <[email protected]> > Tested-by: Tetsuo Handa <[email protected]> > Tested-by: Michal Hocko <[email protected]> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]> > --- > 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
