seqlock: fix the wrong read_seqbegin_or_lock/need_seqretry documentation
Subsystems (3)
PATCH 1/2 seqlock: fix the wrong read_seqbegin_or_lock/need_seqretry documentation
Half of the read_seqbegin_or_lock's users are buggy (I'll send the fixes), and I guess this is because the documentation and the pseudo code in Documentation/locking/seqlock.rst are wrong.
Pseudo code:
int seq = 0;
do {
read_seqbegin_or_lock(&foo_seqlock, &seq);
/* ... [[read-side critical section]] ... */
} while (need_seqretry(&foo_seqlock, seq));read_seqbegin_or_lock() returns with the even seq, need_seqretry() doesn't change this counter. This means that seq is always even and thus the locking pass is simply impossible.
IOW, "_or_lock" has no effect and this code doesn't differ from
do {
seq = read_seqbegin(&foo_seqlock);
/* ... [[read-side critical section]] ... */
} while (read_seqretry(&foo_seqlock, seq));--- Documentation/locking/seqlock.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Documentation/locking/seqlock.rst +5 -4
@@ -218,13 +218,14 @@ Read path, three categories: according to a passed marker. This is used to avoid lockless readers starvation (too much retry loops) in case of a sharp spike in write activity. First, a lockless read is tried (even marker passed). If - that trial fails (odd sequence counter is returned, which is used as - the next iteration marker), the lockless read is transformed to a - full locking read and no retry loop is necessary:: + that trial fails (sequence counter doesn't match), make the marker + odd for the next iteration, the lockless read is transformed to a + full locking read and no retry loop is necessary, for example:: /* marker; even initialization */ - int seq = 0; + int seq = 1; do { + seq++; /* 2 on the 1st/lockless path, otherwise odd */ read_seqbegin_or_lock(&foo_seqlock, &seq); /* ... [[read-side critical section]] ... */
Signature · 1 line
2.25.1.362.g51ebf55
RFC PATCH 2/2 seqlock: introduce need_seqretry_xxx()
Not for inclusion, just for discussion...
Modulo naming, do you think the new need_seqretry_xxx() makes sense?
Simpler to use and less error prone. thread_group_cputime() is changed as an example.
--- include/linux/seqlock.h | 10 ++++++++++ kernel/sched/cputime.c | 9 +++------ 2 files changed, 13 insertions(+), 6 deletions(-)
include/linux/seqlock.h +10 -0
@@ -1165,6 +1165,16 @@ static inline int need_seqretry(seqlock_t *lock, int seq)return !(seq &1 ) &&read_seqretry (lock, seq); } +static inline int need_seqretry_xxx (seqlock_t *lock,int *seq) +{ +int ret = !(*seq &1 ) &&read_seqretry (lock, *seq); + +if (ret) + ++*seq;/* make this counter odd */ + +return ret; +} +/** * done_seqretry() - end seqlock_t "locking or lockless" reader section * @lock: Pointer to seqlock_t
kernel/sched/cputime.c +3 -6
@@ -314,7 +314,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)struct signal_struct *sig = tsk->signal;u64 utime, stime;struct task_struct *t; -unsigned int seq, nextseq; +unsigned int seq;unsigned long flags;/*
@@ -330,9 +330,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)rcu_read_lock ();/* Attempt a lockless read on the first round. */ - nextseq =0 ; + seq =0 ; do { - seq = nextseq; flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq); times->utime = sig->utime; times->stime = sig->stime;
@@ -344,9 +343,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) times->stime += stime; times->sum_exec_runtime +=read_sum_exec_runtime (t); } -/* If lockless access failed, take the lock. */ - nextseq =1 ; - }while (need_seqretry (&sig->stats_lock, seq)); + }while (need_seqretry_xxx (&sig->stats_lock, &seq));done_seqretry_irqrestore (&sig->stats_lock, seq, flags);rcu_read_unlock (); }
Signature · 1 line
2.25.1.362.g51ebf55
Re: [RFC PATCH 2/2] seqlock: introduce need_seqretry_xxx()
Or perhaps even something like
static inline int xxx(seqlock_t *lock, int *seq, int lockless)
{
if (lockless) {
*seq = read_seqbegin(lock);
return 1;
}
if (*seq & 1) {
read_sequnlock_excl(lock);
return 0;
}
if (read_seqretry(lock, *seq)) {
read_seqlock_excl(lock);
*seq = 1;
return 1;
}
return 0;
}
#define __XXX(lock, seq, lockless) \
for (int lockless = 1, seq; xxx(lock, &seq, lockless); lockless = 0)
#define XXX(lock) \
__XXX(lock, __UNIQUE_ID(seq), __UNIQUE_ID(lockless))? This way one can do
seqlock_t sl;
void func(void)
{
XXX(&sl) {
... read-side critical section ...
}
}using the single XXX() helper, no need to declare/initialize seq, no need to call need_seqretry/done_seqretry.
What do you think?
Oleg.
Quoted 71 lines · Oleg Nesterov
Not for inclusion, just for discussion...
Modulo naming, do you think the new
need_seqretry_xxx()makes sense?Simpler to use and less error prone.
thread_group_cputime()is changed as an example.--- include/linux/seqlock.h | 10 ++++++++++ kernel/sched/cputime.c | 9 +++------ 2 files changed, 13 insertions(+), 6 deletions(-)include/linux/seqlock.h +10
-0@@ -1165,6 +1165,16 @@ static inline int need_seqretry(seqlock_t *lock, int seq)return !(seq &1 ) &&read_seqretry (lock, seq); } +static inline int need_seqretry_xxx (seqlock_t *lock,int *seq) +{ +int ret = !(*seq &1 ) &&read_seqretry (lock, *seq); + +if (ret) + ++*seq;/* make this counter odd */ + +return ret; +} +/** * done_seqretry() - end seqlock_t "locking or lockless" reader section * @lock: Pointer to seqlock_t kernel/sched/cputime.c +3
-7@@ -314,7 +314,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)struct signal_struct *sig = tsk->signal;u64 utime, stime;struct task_struct *t; -unsigned int seq, nextseq; +unsigned int seq;unsigned long flags;/* @@ -330,9 +330,8 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)rcu_read_lock ();/* Attempt a lockless read on the first round. */ - nextseq =0 ; + seq =0 ; do { - seq = nextseq; flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq); times->utime = sig->utime; times->stime = sig->stime;@@ -344,9 +343,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) times->stime += stime; times->sum_exec_runtime +=read_sum_exec_runtime (t); } -/* If lockless access failed, take the lock. */ - nextseq =1 ; - }while (need_seqretry (&sig->stats_lock, seq)); + }while (need_seqretry_xxx (&sig->stats_lock, &seq));done_seqretry_irqrestore (&sig->stats_lock, seq, flags);rcu_read_unlock (); } --2.25.1.362.g51ebf55
Ping.
Please ignore 2/2 for now (it obviously wasn't for inclusion), but the wrong documentation confuses the users.
fs/afs, rxrpc_find_service_conn_rcu, nfsd_copy_write_verifier use read_seqbegin_or_lock/need_seqretry according to this doc and they are wrong.
I am discussing the necessary changes in the code paths above, but can't we fix the documentation?
Quoted 59 lines · Oleg Nesterov
Half of the read_seqbegin_or_lock's users are buggy (I'll send the fixes), and I guess this is because the documentation and the pseudo code in
Documentation/locking/seqlock.rstare wrong.Pseudo code:
int seq = 0; do { read_seqbegin_or_lock(&foo_seqlock, &seq); /* ... [[read-side critical section]] ... */ } while (need_seqretry(&foo_seqlock, seq));
read_seqbegin_or_lock()returns with the even seq,need_seqretry()doesn't change this counter. This means that seq is always even and thus the locking pass is simply impossible.IOW, "
_or_lock" has no effect and this code doesn't differ fromdo { seq = read_seqbegin(&foo_seqlock); /* ... [[read-side critical section]] ... */ } while (read_seqretry(&foo_seqlock, seq));Signed-off-by Oleg Nesterov <[email protected]>--- Documentation/locking/seqlock.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)Documentation/locking/seqlock.rst +5
-4@@ -218,13 +218,14 @@ Read path, three categories: according to a passed marker. This is used to avoid lockless readers starvation (too much retry loops) in case of a sharp spike in write activity. First, a lockless read is tried (even marker passed). If - that trial fails (odd sequence counter is returned, which is used as - the next iteration marker), the lockless read is transformed to a - full locking read and no retry loop is necessary:: + that trial fails (sequence counter doesn't match), make the marker + odd for the next iteration, the lockless read is transformed to a + full locking read and no retry loop is necessary, for example:: /* marker; even initialization */ - int seq = 0; + int seq = 1; do { + seq++; /* 2 on the 1st/lockless path, otherwise odd */ read_seqbegin_or_lock(&foo_seqlock, &seq); /* ... [[read-side critical section]] ... */Signature · 1 line
2.25.1.362.g51ebf55