Re: [PATCH 5.15 1/1] Revert "locking/rwsem: Make handoff bit handling more consistent"
From: John Donnelly
Date: Wed Apr 27 2022 - 19:21:12 EST
On 4/27/22 5:56 AM, john.p.donnelly@xxxxxxxxxx wrote:
--
Hi
Since Waiman followed up with :
[PATCH] locking/rwsem: Allow slowpath writer to ignore handoff bit if
not set by first waiter
today, I am not sure if this is needed if that patch is applied to 5.15.y .
Please advise.
This reverts commit 76723ed1fb8922ee94089e7432b8a262e3a06ed7.
This commit has introduced frequent system hangs running "fio test
suite" on a variety of filesystem types on LVM volumes composed of four
nvme devices.
This issue was first encountered in Linux 5.15.30, and later in Linux
5.17.y.
Each panic includes the rwsem_down_write_slowpath() :
ext4:
PID: 3682 TASK: ffff8f489ae34bc0 CPU: 2 COMMAND: "dio/dm-0"
#0 [fffffe0000083e50] crash_nmi_callback at ffffffff828772b3
#1 [fffffe0000083e58] nmi_handle at ffffffff82840778
#2 [fffffe0000083ea0] default_do_nmi at ffffffff8337a1e2
#3 [fffffe0000083ec8] exc_nmi at ffffffff8337a48d
#4 [fffffe0000083ef0] end_repeat_nmi at ffffffff8340153b
[exception RIP: _raw_spin_lock_irq+23]
RIP: ffffffff8338b2e7 RSP: ffff9c4409b47c78 RFLAGS: 00000046
RAX: 0000000000000000 RBX: ffff8f489ae34bc0 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff8f47f7b90104
RBP: ffff9c4409b47d20 R8: 0000000000000000 R9: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8f47f7b90104
R13: ffff9c4409b47cb0 R14: ffff8f47f7b900f0 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#5 [ffff9c4409b47c78] _raw_spin_lock_irq at ffffffff8338b2e7
#6 [ffff9c4409b47c78] rwsem_down_write_slowpath at ffffffff82925be9
#7 [ffff9c4409b47d28] ext4_map_blocks at ffffffffc11c26dc [ext4]
#8 [ffff9c4409b47d98] ext4_convert_unwritten_extents at
ffffffffc11ad9e0 [ext4]
#9 [ffff9c4409b47df0] ext4_dio_write_end_io at ffffffffc11b22aa [ext4]
xfs:
PID: 3719 TASK: ffff9f81d2d74bc0 CPU: 37 COMMAND: "dio/dm-0"
#0 [fffffe0000894e50] crash_nmi_callback at ffffffffad6772b3
#1 [fffffe0000894e58] nmi_handle at ffffffffad640778
#2 [fffffe0000894ea0] default_do_nmi at ffffffffae17a1e2
#3 [fffffe0000894ec8] exc_nmi at ffffffffae17a48d
#4 [fffffe0000894ef0] end_repeat_nmi at ffffffffae20153b
[exception RIP: _raw_spin_lock_irq+23]
RIP: ffffffffae18b2e7 RSP: ffffbb7ec9637c48 RFLAGS: 00000046
RAX: 0000000000000000 RBX: ffff9f81d2d74bc0 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff9f81c04a918c
RBP: ffffbb7ec9637ce8 R8: 0000000000000000 R9: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff9f81c04a918c
R13: ffffbb7ec9637c80 R14: ffff9f81c04a9178 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
<NMI exception stack> ---
#5 [ffffbb7ec9637c48] _raw_spin_lock_irq at ffffffffae18b2e7
#6 [ffffbb7ec9637c48] rwsem_down_write_slowpath at ffffffffad725be9
#7 [ffffbb7ec9637cf0] xfs_trans_alloc_inode at ffffffffc074f2bd [xfs]
#8 [ffffbb7ec9637d50] xfs_iomap_write_unwritten at ffffffffc073ad15
Reported-by: Jorge Lopez <jorge.jo.lopez@xxxxxxxxxx>
Tested-by: Jorge Lopez <jorge.jo.lopez@xxxxxxxxxx>
Signed-off-by: John Donnelly <john.p.donnelly@xxxxxxxxxx>
Reviewed-by: Jack Vogel <jack.vogel@xxxxxxxxxx>
---
kernel/locking/rwsem.c | 171 +++++++++++++++++++++--------------------
1 file changed, 86 insertions(+), 85 deletions(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index e63f740c2cc8..29eea50a3e67 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -106,9 +106,9 @@
* atomic_long_cmpxchg() will be used to obtain writer lock.
*
* There are three places where the lock handoff bit may be set or
cleared.
- * 1) rwsem_mark_wake() for readers -- set, clear
- * 2) rwsem_try_write_lock() for writers -- set, clear
- * 3) rwsem_del_waiter() -- clear
+ * 1) rwsem_mark_wake() for readers.
+ * 2) rwsem_try_write_lock() for writers.
+ * 3) Error path of rwsem_down_write_slowpath().
*
* For all the above cases, wait_lock will be held. A writer must also
* be the first one in the wait_list to be eligible for setting the
handoff
@@ -335,9 +335,6 @@ struct rwsem_waiter {
struct task_struct *task;
enum rwsem_waiter_type type;
unsigned long timeout;
-
- /* Writer only, not initialized in reader */
- bool handoff_set;
};
#define rwsem_first_waiter(sem) \
list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
@@ -348,6 +345,12 @@ enum rwsem_wake_type {
RWSEM_WAKE_READ_OWNED /* Waker thread holds the read lock */
};
+enum writer_wait_state {
+ WRITER_NOT_FIRST, /* Writer is not first in wait list */
+ WRITER_FIRST, /* Writer is first in wait list */
+ WRITER_HANDOFF /* Writer is first & handoff needed */
+};
+
/*
* The typical HZ value is either 250 or 1000. So set the minimum waiting
* time to at least 4ms or 1 jiffy (if it is higher than 4ms) in the wait
@@ -363,31 +366,6 @@ enum rwsem_wake_type {
*/
#define MAX_READERS_WAKEUP 0x100
-static inline void
-rwsem_add_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
-{
- lockdep_assert_held(&sem->wait_lock);
- list_add_tail(&waiter->list, &sem->wait_list);
- /* caller will set RWSEM_FLAG_WAITERS */
-}
-
-/*
- * Remove a waiter from the wait_list and clear flags.
- *
- * Both rwsem_mark_wake() and rwsem_try_write_lock() contain a full
'copy' of
- * this function. Modify with care.
- */
-static inline void
-rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
-{
- lockdep_assert_held(&sem->wait_lock);
- list_del(&waiter->list);
- if (likely(!list_empty(&sem->wait_list)))
- return;
-
- atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS,
&sem->count);
-}
-
/*
* handle the lock release when processes blocked on it that can now run
* - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must
@@ -399,8 +377,6 @@ rwsem_del_waiter(struct rw_semaphore *sem, struct
rwsem_waiter *waiter)
* preferably when the wait_lock is released
* - woken process blocks are discarded from the list after having
task zeroed
* - writers are only marked woken if downgrading is false
- *
- * Implies rwsem_del_waiter() for all woken readers.
*/
static void rwsem_mark_wake(struct rw_semaphore *sem,
enum rwsem_wake_type wake_type,
@@ -515,25 +491,18 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
adjustment = woken * RWSEM_READER_BIAS - adjustment;
lockevent_cond_inc(rwsem_wake_reader, woken);
-
- oldcount = atomic_long_read(&sem->count);
if (list_empty(&sem->wait_list)) {
- /*
- * Combined with list_move_tail() above, this implies
- * rwsem_del_waiter().
- */
+ /* hit end of list above */
adjustment -= RWSEM_FLAG_WAITERS;
- if (oldcount & RWSEM_FLAG_HANDOFF)
- adjustment -= RWSEM_FLAG_HANDOFF;
- } else if (woken) {
- /*
- * When we've woken a reader, we no longer need to force
- * writers to give up the lock and we can clear HANDOFF.
- */
- if (oldcount & RWSEM_FLAG_HANDOFF)
- adjustment -= RWSEM_FLAG_HANDOFF;
}
+ /*
+ * When we've woken a reader, we no longer need to force writers
+ * to give up the lock and we can clear HANDOFF.
+ */
+ if (woken && (atomic_long_read(&sem->count) & RWSEM_FLAG_HANDOFF))
+ adjustment -= RWSEM_FLAG_HANDOFF;
+
if (adjustment)
atomic_long_add(adjustment, &sem->count);
@@ -564,12 +533,12 @@ static void rwsem_mark_wake(struct rw_semaphore
*sem,
* race conditions between checking the rwsem wait list and setting the
* sem->count accordingly.
*
- * Implies rwsem_del_waiter() on success.
+ * 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(struct rw_semaphore *sem,
- struct rwsem_waiter *waiter)
+ enum writer_wait_state wstate)
{
- bool first = rwsem_first_waiter(sem) == waiter;
long count, new;
lockdep_assert_held(&sem->wait_lock);
@@ -578,19 +547,13 @@ static inline bool rwsem_try_write_lock(struct
rw_semaphore *sem,
do {
bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF);
- if (has_handoff) {
- if (!first)
- return false;
-
- /* First waiter inherits a previously set handoff bit */
- waiter->handoff_set = true;
- }
+ if (has_handoff && wstate == WRITER_NOT_FIRST)
+ return false;
new = count;
if (count & RWSEM_LOCK_MASK) {
- if (has_handoff || (!rt_task(waiter->task) &&
- !time_after(jiffies, waiter->timeout)))
+ if (has_handoff || (wstate != WRITER_HANDOFF))
return false;
new |= RWSEM_FLAG_HANDOFF;
@@ -607,17 +570,9 @@ static inline bool rwsem_try_write_lock(struct
rw_semaphore *sem,
* We have either acquired the lock with handoff bit cleared or
* set the handoff bit.
*/
- if (new & RWSEM_FLAG_HANDOFF) {
- waiter->handoff_set = true;
- lockevent_inc(rwsem_wlock_handoff);
+ if (new & RWSEM_FLAG_HANDOFF)
return false;
- }
- /*
- * Have rwsem_try_write_lock() fully imply rwsem_del_waiter() on
- * success.
- */
- list_del(&waiter->list);
rwsem_set_owner(sem);
return true;
}
@@ -998,7 +953,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem,
long count, unsigned int stat
}
adjustment += RWSEM_FLAG_WAITERS;
}
- rwsem_add_waiter(sem, &waiter);
+ list_add_tail(&waiter.list, &sem->wait_list);
/* we're now waiting on the lock, but no longer actively locking */
count = atomic_long_add_return(adjustment, &sem->count);
@@ -1044,7 +999,11 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem,
long count, unsigned int stat
return sem;
out_nolock:
- rwsem_del_waiter(sem, &waiter);
+ list_del(&waiter.list);
+ if (list_empty(&sem->wait_list)) {
+ atomic_long_andnot(RWSEM_FLAG_WAITERS|RWSEM_FLAG_HANDOFF,
+ &sem->count);
+ }
raw_spin_unlock_irq(&sem->wait_lock);
__set_current_state(TASK_RUNNING);
lockevent_inc(rwsem_rlock_fail);
@@ -1058,7 +1017,9 @@ static struct rw_semaphore *
rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
{
long count;
+ enum writer_wait_state wstate;
struct rwsem_waiter waiter;
+ struct rw_semaphore *ret = sem;
DEFINE_WAKE_Q(wake_q);
/* do optimistic spinning and steal lock if possible */
@@ -1074,13 +1035,16 @@ rwsem_down_write_slowpath(struct rw_semaphore
*sem, int state)
waiter.task = current;
waiter.type = RWSEM_WAITING_FOR_WRITE;
waiter.timeout = jiffies + RWSEM_WAIT_TIMEOUT;
- waiter.handoff_set = false;
raw_spin_lock_irq(&sem->wait_lock);
- rwsem_add_waiter(sem, &waiter);
+
+ /* account for this before adding a new element to the list */
+ wstate = list_empty(&sem->wait_list) ? WRITER_FIRST :
WRITER_NOT_FIRST;
+
+ list_add_tail(&waiter.list, &sem->wait_list);
/* we're now waiting on the lock */
- if (rwsem_first_waiter(sem) != &waiter) {
+ if (wstate == WRITER_NOT_FIRST) {
count = atomic_long_read(&sem->count);
/*
@@ -1116,16 +1080,13 @@ rwsem_down_write_slowpath(struct rw_semaphore
*sem, int state)
/* wait until we successfully acquire the lock */
set_current_state(state);
for (;;) {
- if (rwsem_try_write_lock(sem, &waiter)) {
+ if (rwsem_try_write_lock(sem, wstate)) {
/* rwsem_try_write_lock() implies ACQUIRE on success */
break;
}
raw_spin_unlock_irq(&sem->wait_lock);
- if (signal_pending_state(state, current))
- goto out_nolock;
-
/*
* After setting the handoff bit and failing to acquire
* the lock, attempt to spin on owner to accelerate lock
@@ -1134,7 +1095,7 @@ rwsem_down_write_slowpath(struct rw_semaphore
*sem, int state)
* In this case, we attempt to acquire the lock again
* without sleeping.
*/
- if (waiter.handoff_set) {
+ if (wstate == WRITER_HANDOFF) {
enum owner_state owner_state;
preempt_disable();
@@ -1145,26 +1106,66 @@ rwsem_down_write_slowpath(struct rw_semaphore
*sem, int state)
goto trylock_again;
}
- schedule();
- lockevent_inc(rwsem_sleep_writer);
- set_current_state(state);
+ /* Block until there are no active lockers. */
+ for (;;) {
+ if (signal_pending_state(state, current))
+ goto out_nolock;
+
+ schedule();
+ lockevent_inc(rwsem_sleep_writer);
+ set_current_state(state);
+ /*
+ * If HANDOFF bit is set, unconditionally do
+ * a trylock.
+ */
+ if (wstate == WRITER_HANDOFF)
+ break;
+
+ if ((wstate == WRITER_NOT_FIRST) &&
+ (rwsem_first_waiter(sem) == &waiter))
+ wstate = WRITER_FIRST;
+
+ count = atomic_long_read(&sem->count);
+ if (!(count & RWSEM_LOCK_MASK))
+ break;
+
+ /*
+ * The setting of the handoff bit is deferred
+ * until rwsem_try_write_lock() is called.
+ */
+ if ((wstate == WRITER_FIRST) && (rt_task(current) ||
+ time_after(jiffies, waiter.timeout))) {
+ wstate = WRITER_HANDOFF;
+ lockevent_inc(rwsem_wlock_handoff);
+ break;
+ }
+ }
trylock_again:
raw_spin_lock_irq(&sem->wait_lock);
}
__set_current_state(TASK_RUNNING);
+ list_del(&waiter.list);
raw_spin_unlock_irq(&sem->wait_lock);
lockevent_inc(rwsem_wlock);
- return sem;
+
+ return ret;
out_nolock:
__set_current_state(TASK_RUNNING);
raw_spin_lock_irq(&sem->wait_lock);
- rwsem_del_waiter(sem, &waiter);
- if (!list_empty(&sem->wait_list))
+ list_del(&waiter.list);
+
+ if (unlikely(wstate == WRITER_HANDOFF))
+ atomic_long_add(-RWSEM_FLAG_HANDOFF, &sem->count);
+
+ if (list_empty(&sem->wait_list))
+ atomic_long_andnot(RWSEM_FLAG_WAITERS, &sem->count);
+ else
rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
raw_spin_unlock_irq(&sem->wait_lock);
wake_up_q(&wake_q);
lockevent_inc(rwsem_wlock_fail);
+
return ERR_PTR(-EINTR);
}
-- 2.31.1