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
