Skip to content

Commit 5c1ec49

Browse files
Waiman-LongIngo Molnar
authored andcommitted
locking/rwsem: Remove rwsem_wake() wakeup optimization
After the following commit: 59aabfc ("locking/rwsem: Reduce spinlock contention in wakeup after up_read()/up_write()") the rwsem_wake() forgoes doing a wakeup if the wait_lock cannot be directly acquired and an optimistic spinning locker is present. This can help performance by avoiding spinning on the wait_lock when it is contended. With the later commit: 133e89e ("locking/rwsem: Enable lockless waiter wakeup(s)") the performance advantage of the above optimization diminishes as the average wait_lock hold time become much shorter. With a later patch that supports rwsem lock handoff, we can no longer relies on the fact that the presence of an optimistic spinning locker will ensure that the lock will be acquired by a task soon and rwsem_wake() will be called later on to wake up waiters. This can lead to missed wakeup and application hang. So the original 59aabfc commit has to be reverted. Signed-off-by: Waiman Long <[email protected]> Signed-off-by: Peter Zijlstra (Intel) <[email protected]> Cc: Borislav Petkov <[email protected]> Cc: Davidlohr Bueso <[email protected]> Cc: H. Peter Anvin <[email protected]> Cc: Linus Torvalds <[email protected]> Cc: Peter Zijlstra <[email protected]> Cc: Thomas Gleixner <[email protected]> Cc: Tim Chen <[email protected]> Cc: Will Deacon <[email protected]> Cc: huang ying <[email protected]> Link: https://lkml.kernel.org/r/[email protected] Signed-off-by: Ingo Molnar <[email protected]>
1 parent c71fd89 commit 5c1ec49

File tree

1 file changed

+0
-72
lines changed

1 file changed

+0
-72
lines changed

kernel/locking/rwsem-xadd.c

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -411,25 +411,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
411411
lockevent_cond_inc(rwsem_opt_fail, !taken);
412412
return taken;
413413
}
414-
415-
/*
416-
* Return true if the rwsem has active spinner
417-
*/
418-
static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
419-
{
420-
return osq_is_locked(&sem->osq);
421-
}
422-
423414
#else
424415
static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
425416
{
426417
return false;
427418
}
428-
429-
static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
430-
{
431-
return false;
432-
}
433419
#endif
434420

435421
/*
@@ -651,65 +637,7 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
651637
unsigned long flags;
652638
DEFINE_WAKE_Q(wake_q);
653639

654-
/*
655-
* __rwsem_down_write_failed_common(sem)
656-
* rwsem_optimistic_spin(sem)
657-
* osq_unlock(sem->osq)
658-
* ...
659-
* atomic_long_add_return(&sem->count)
660-
*
661-
* - VS -
662-
*
663-
* __up_write()
664-
* if (atomic_long_sub_return_release(&sem->count) < 0)
665-
* rwsem_wake(sem)
666-
* osq_is_locked(&sem->osq)
667-
*
668-
* And __up_write() must observe !osq_is_locked() when it observes the
669-
* atomic_long_add_return() in order to not miss a wakeup.
670-
*
671-
* This boils down to:
672-
*
673-
* [S.rel] X = 1 [RmW] r0 = (Y += 0)
674-
* MB RMB
675-
* [RmW] Y += 1 [L] r1 = X
676-
*
677-
* exists (r0=1 /\ r1=0)
678-
*/
679-
smp_rmb();
680-
681-
/*
682-
* If a spinner is present, it is not necessary to do the wakeup.
683-
* Try to do wakeup only if the trylock succeeds to minimize
684-
* spinlock contention which may introduce too much delay in the
685-
* unlock operation.
686-
*
687-
* spinning writer up_write/up_read caller
688-
* --------------- -----------------------
689-
* [S] osq_unlock() [L] osq
690-
* MB RMB
691-
* [RmW] rwsem_try_write_lock() [RmW] spin_trylock(wait_lock)
692-
*
693-
* Here, it is important to make sure that there won't be a missed
694-
* wakeup while the rwsem is free and the only spinning writer goes
695-
* to sleep without taking the rwsem. Even when the spinning writer
696-
* is just going to break out of the waiting loop, it will still do
697-
* a trylock in rwsem_down_write_failed() before sleeping. IOW, if
698-
* rwsem_has_spinner() is true, it will guarantee at least one
699-
* trylock attempt on the rwsem later on.
700-
*/
701-
if (rwsem_has_spinner(sem)) {
702-
/*
703-
* The smp_rmb() here is to make sure that the spinner
704-
* state is consulted before reading the wait_lock.
705-
*/
706-
smp_rmb();
707-
if (!raw_spin_trylock_irqsave(&sem->wait_lock, flags))
708-
return sem;
709-
goto locked;
710-
}
711640
raw_spin_lock_irqsave(&sem->wait_lock, flags);
712-
locked:
713641

714642
if (!list_empty(&sem->wait_list))
715643
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);

0 commit comments

Comments
 (0)