[PATCH 1/2] seqlock: fix the wrong read_seqbegin_or_lock/need_seqretry documentation

[PATCH 1/2] seqlock: fix the wrong read_seqbegin_or_lock/need_seqretry documentation

[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.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));

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 Documentation/locking/seqlock.rst | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/locking/seqlock.rst b/Documentation/locking/seqlock.rst
index bfda1a5fecad..4bdf8d4ed2a2 100644
--- a/Documentation/locking/seqlock.rst
+++ b/Documentation/locking/seqlock.rst
@@ -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]] ... */
-- 
2.25.1.362.g51ebf55

[RFC PATCH 2/2] seqlock: introduce need_seqretry_xxx()

[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(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index d0050c889f26..9b3bc4ce3332 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -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
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index af7952f12e6c..45704a84baec 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -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

Re: [RFC PATCH 2/2] seqlock: introduce need_seqretry_xxx()

[Oleg Nesterov]

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.


On 10/24, Oleg Nesterov wrote:
>
> 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(-)
>
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index d0050c889f26..9b3bc4ce3332 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -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
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index af7952f12e6c..45704a84baec 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -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
>

Re: [PATCH 1/2] seqlock: fix the wrong read_seqbegin_or_lock/need_seqretry documentation

[Oleg Nesterov]

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?

On 10/24, Oleg Nesterov wrote:
>
> 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));
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  Documentation/locking/seqlock.rst | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/locking/seqlock.rst b/Documentation/locking/seqlock.rst
> index bfda1a5fecad..4bdf8d4ed2a2 100644
> --- a/Documentation/locking/seqlock.rst
> +++ b/Documentation/locking/seqlock.rst
> @@ -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]] ... */
> -- 
> 2.25.1.362.g51ebf55
>