linux-kernel - Re: [PATCH 1/1] documentation: seqlock: fix the wrong documentation of read_seqbegin_or_lock/need

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251002110145.GB32506@redhat.com>
Date: Thu, 2 Oct 2025 13:01:46 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Waiman Long <llong@...hat.com>
Cc: Boqun Feng <boqun.feng@...il.com>, David Howells <dhowells@...hat.com>,
	Ingo Molnar <mingo@...hat.com>, Li RongQing <lirongqing@...du.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] documentation: seqlock: fix the wrong documentation
 of read_seqbegin_or_lock/need_seqretry

Hi Waiman,

This is probably my fault, but I can't understand your emails. So let me start
from the very beginning and write another reply to this email.

I don't think that we can change include/linux/seqlock.h so that this change
will make the documentation correct without changing/breaking the existing code.
But perhaps I misunderstood you...

And just in case... of course need_seqretry_xxx() and/or XXX() must be renamed.

On 10/01, Waiman Long wrote:
>
> On 9/28/25 12:20 PM, Oleg Nesterov wrote:
> >-	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]] ... */
>
> It is kind of odd to initialize the sequence to 1 and add an sequence
> increment inside the loop.

Sure. That is why I am proposing the new helper which can be used instead
need_seqretry(). You named it need_seqretry_once() below.

Now, from Documentation/locking/seqlock.rst before this patch:

	/* marker; even initialization */
	int seq = 0;
	do {
		read_seqbegin_or_lock(&foo_seqlock, &seq);

		/* ... [[read-side critical section]] ... */

	} while (need_seqretry(&foo_seqlock, seq));
	done_seqretry(&foo_seqlock, seq);

> Perhaps we can do something like:
>
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -1126,6 +1126,9 @@ read_sequnlock_excl_irqrestore(seqlock_t *sl, unsigned
> long flags)
>   */
>  static inline void read_seqbegin_or_lock(seqlock_t *lock, int *seq)
>  {
> +       if (!(*seq & 1))        /* Reread sequence # if even */
> +               *seq = seqprop_sequence(&lock->seqcount);
> +

Why? I don't understand this change...

> +static inline int need_seqretry_once(seqlock_t *lock, int *seq)
> +{
> +       int ret = !(*seq & 1) && read_seqretry(lock, *seq);
> +
> +       if (ret)
> +               *seq = 1;       /* Enforce locking in next iteration */
> +       return ret;
> +}
>
> With this, the current document should be good.

No? How can this change make the pseudo code above correct? It will never
take the lock. OK, unless CONFIG_PREEMPT_RT=y but this is another story.
And the documentation is still wrong in this respect.

> Users have the option of
> using need_seqretry_once() to enforce at most 1 iteration.

So we need to change the pseudo code above

	-	} while (need_seqretry(&foo_seqlock, seq));
	+	} while (need_seqretry_once(&foo_seqlock, &seq));

and this is exactly what I am trying to suggest in "RFC 2/1".

So I think we should fix the docs, and the new helper(s), and update the
current users one-by-one.

Oleg.