Messages in this thread | | | Subject | Re: [PATCH-tip v7 09/20] locking/rwsem: Always release wait_lock before waking up tasks | From | Waiman Long <> | Date | Fri, 3 May 2019 09:56:41 -0400 |
| |
On 5/3/19 9:37 AM, Peter Zijlstra wrote: > On Sun, Apr 28, 2019 at 05:25:46PM -0400, Waiman Long wrote: > >> + /* >> + * This waiter may have become first in the wait >> + * list after re-acquring the wait_lock. The >> + * rwsem_first_waiter() test in the main while >> + * loop below will correctly detect that. We do >> + * need to reload count to perform proper trylock >> + * and avoid missed wakeup. >> + */ >> + count = atomic_long_read(&sem->count); >> + } >> } else { >> count = atomic_long_add_return(RWSEM_FLAG_WAITERS, &sem->count); >> } > I've been eyeing that count usage for the past few patches, and this > here makes me think we should get rid of it. > > --- a/kernel/locking/rwsem.c > +++ b/kernel/locking/rwsem.c > @@ -400,13 +400,14 @@ static void __rwsem_mark_wake(struct rw_ > * If wstate is WRITER_HANDOFF, it will make sure that either the handoff > * bit is set or the lock is acquired with handoff bit cleared. > */ > -static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem, > +static inline bool rwsem_try_write_lock(struct rw_semaphore *sem, > enum writer_wait_state wstate) > { > - long new; > + long count, new; > > lockdep_assert_held(&sem->wait_lock); > > + count = atomic_long_read(&sem->count); > do { > bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF); > > @@ -760,25 +761,16 @@ rwsem_down_write_slowpath(struct rw_sema > wake_up_q(&wake_q); > wake_q_init(&wake_q); /* Used again, reinit */ > raw_spin_lock_irq(&sem->wait_lock); > - /* > - * This waiter may have become first in the wait > - * list after re-acquring the wait_lock. The > - * rwsem_first_waiter() test in the main while > - * loop below will correctly detect that. We do > - * need to reload count to perform proper trylock > - * and avoid missed wakeup. > - */ > - count = atomic_long_read(&sem->count); > } > } else { > - count = atomic_long_add_return(RWSEM_FLAG_WAITERS, &sem->count); > + atomic_long_or(RWSEM_FLAG_WAITERS, &sem->count); > } > > wait: > /* wait until we successfully acquire the lock */ > set_current_state(state); > for (;;) { > - if (rwsem_try_write_lock(count, sem, wstate)) > + if (rwsem_try_write_lock(sem, wstate)) > break; > > raw_spin_unlock_irq(&sem->wait_lock); > @@ -819,7 +811,6 @@ rwsem_down_write_slowpath(struct rw_sema > } > > raw_spin_lock_irq(&sem->wait_lock); > - count = atomic_long_read(&sem->count); > } > __set_current_state(TASK_RUNNING); > list_del(&waiter.list);
Yes, this is an alternative way of doing it.
Cheers, Longman
| |