Messages in this thread | | | Date | Thu, 29 Sep 2022 21:09:58 -0400 | Subject | Re: [PATCH] locking/rwsem: Prevent non-first waiter from spinning in down_write() slowpath | From | Waiman Long <> |
| |
On 9/29/22 17:15, John Donnelly wrote: > On 9/29/22 13:04, Waiman Long wrote: >> A non-first waiter can potentially spin in the for loop of >> rwsem_down_write_slowpath() without sleeping but fail to acquire the >> lock even if the rwsem is free if the following sequence happens: >> >> Non-first waiter First waiter Lock holder >> ---------------- ------------ ----------- >> Acquire wait_lock >> rwsem_try_write_lock(): >> Set handoff bit if RT or >> wait too long >> Set waiter->handoff_set >> Release wait_lock >> Acquire wait_lock >> Inherit waiter->handoff_set >> Release wait_lock >> Clear owner >> Release lock >> if (waiter.handoff_set) { >> rwsem_spin_on_owner((); >> if (OWNER_NULL) >> goto trylock_again; >> } >> trylock_again: >> Acquire wait_lock >> rwsem_try_write_lock(): >> if (first->handoff_set && (waiter != first)) >> return false; >> Release wait_lock >> >> It is especially problematic if the non-first waiter is an RT task and >> it is running on the same CPU as the first waiter as this can lead to >> live lock. >> >> Fixes: d257cc8cb8d5 ("locking/rwsem: Make handoff bit handling more >> consistent") >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> kernel/locking/rwsem.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c >> index 65f0262f635e..ad676e99e0b3 100644 >> --- a/kernel/locking/rwsem.c >> +++ b/kernel/locking/rwsem.c >> @@ -628,6 +628,11 @@ static inline bool rwsem_try_write_lock(struct >> rw_semaphore *sem, >> new = count; >> if (count & RWSEM_LOCK_MASK) { >> + /* >> + * A waiter (first or not) can set the handoff bit >> + * if it is an RT task or wait in the wait queue >> + * for too long. >> + */ >> if (has_handoff || (!rt_task(waiter->task) && >> !time_after(jiffies, waiter->timeout))) >> return false; >> @@ -643,11 +648,13 @@ static inline bool rwsem_try_write_lock(struct >> rw_semaphore *sem, >> } while (!atomic_long_try_cmpxchg_acquire(&sem->count, &count, >> new)); >> /* >> - * We have either acquired the lock with handoff bit cleared or >> - * set the handoff bit. >> + * We have either acquired the lock with handoff bit cleared or set >> + * the handoff bit. Only the first waiter can have its handoff_set >> + * set here to enable optimistic spinning in slowpath loop. >> */ >> if (new & RWSEM_FLAG_HANDOFF) { >> - waiter->handoff_set = true; >> + if (waiter == first) >> + waiter->handoff_set = true; >> lockevent_inc(rwsem_wlock_handoff); >> return false; >> } > Hi,. > > Are you missing > > > [PATCH 5.18 87/88] locking/rwsem: Allow slowpath writer to ignore > handoff bit if not set by first waiter > > > > Fixes: d257cc8cb8d5 ("locking/rwsem: Make handoff bit handling more > consistent") > > Or is this another regression ? > It is another regression which.
Cheers, Longman
| |