The read_seqbegin/need_seqretry/done_seqretry API is cumbersome and
error prone. With the new helper the "typical" code like

	nextseq = 0;
	do {
		seq = nextseq;
		flags = read_seqbegin_or_lock_irqsave(&seqlock, &seq);

		// read-side critical section

		nextseq = 1;
	} while (need_seqretry(&seqlock, seq));
	done_seqretry_irqrestore(&seqlock, seq, flags);

can be rewritten as

	SEQLOCK_READ_SECTION(&seqlock, &flags) {
		// read-side critical section
	}

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/seqlock.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 5ce48eab7a2a..8cd36a7c1638 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -1209,4 +1209,53 @@ done_seqretry_irqrestore(seqlock_t *lock, int seq, unsigned long flags)
 	if (seq & 1)
 		read_sequnlock_excl_irqrestore(lock, flags);
 }
+
+/* internal helper for SEQLOCK_READ_SECTION() */
+static inline int
+seqlock_read_section_retry(seqlock_t *lock, int *seq, unsigned long *flags)
+{
+	int retry = 0;
+
+	if (*seq & 1) {
+		if (flags)
+			read_sequnlock_excl_irqrestore(lock, *flags);
+		else
+			read_sequnlock_excl(lock);
+	} else if (read_seqretry(lock, *seq)) {
+		retry = *seq = 1;
+		if (flags)
+			read_seqlock_excl_irqsave(lock, *flags);
+		else
+			read_seqlock_excl(lock);
+	}
+
+	return retry;
+}
+
+#define __SEQLOCK_READ_SECTION(lock, lockless, seq, flags)	\
+	for (int lockless = 1, seq = read_seqbegin(lock);		\
+	     lockless || seqlock_read_section_retry(lock, &seq, flags);	\
+	     lockless = 0)
+
+/**
+ * SEQLOCK_READ_SECTION(lock, flags) - execute the read side critical section
+ *                                     without manual sequence counter handling
+ *                                     or calls to other helpers
+ * @lock: pointer to the seqlock_t protecting the data
+ * @flags: pointer to unsigned long for irqsave/irqrestore or NULL
+ *
+ * Example:
+ *
+ *	SEQLOCK_READ_SECTION(&lock, &flags) {
+ *		// read-side critical section
+ *	}
+ *
+ * Starts with a lockless pass first. If it fails, restarts the critical section
+ * with the lock held and, if @flags != NULL, with irqs disabled.
+ *
+ * The critical section must not contain control flow that escapes the loop.
+ */
+#define SEQLOCK_READ_SECTION(lock, flags)	\
+	__SEQLOCK_READ_SECTION(lock, __UNIQUE_ID(lockless), __UNIQUE_ID(seq), flags)
+
 #endif /* __LINUX_SEQLOCK_H */
-- 
2.25.1.362.g51ebf55

On Sun, 5 Oct 2025 at 07:51, Oleg Nesterov <oleg@redhat.com> wrote:
>
> +#define __SEQLOCK_READ_SECTION(lock, lockless, seq, flags)     \
> +       for (int lockless = 1, seq = read_seqbegin(lock);               \
> +            lockless || seqlock_read_section_retry(lock, &seq, flags); \
> +            lockless = 0)
> ...
> +#define SEQLOCK_READ_SECTION(lock, flags)      \
> +       __SEQLOCK_READ_SECTION(lock, __UNIQUE_ID(lockless), __UNIQUE_ID(seq), flags)

Ok, I like the helper wrapper, but I don't actually think we need it
to be shouty.

As far as the users are concerned, the result doesn't end up being
really any different from our scoped guards, so I'd actually suggest
you just make this helper look like our scoped_guard() macro does.

And instead of making people pass in a NULL 'flags', just do a
separate version of it, exactly like we do for locking. Even if the
internal implementation then ends up sharing most of the code, please
don't make people pass in NULL just because they don't want the
irqsave version.

So make it two different things:

   scoped_seqlock_read(lock) { .... }

   scoped_seqlock__read_irqsave(lock, flags) { ... }

or something.

(Maybe 'flags' can even be local to that scope?)

Hmm?

                Linus

OK. I'll send v2 but let me ask...

On 10/05, Linus Torvalds wrote:
>
> As far as the users are concerned, the result doesn't end up being
> really any different from our scoped guards, so I'd actually suggest
> you just make this helper look like our scoped_guard() macro does.

I swear, I too thought about scoped_seqlock_xxx ;)

> And instead of making people pass in a NULL 'flags', just do a
> separate version of it, exactly like we do for locking. Even if the
> internal implementation then ends up sharing most of the code, please
> don't make people pass in NULL just because they don't want the
> irqsave version.
>
> So make it two different things:
>
>    scoped_seqlock_read(lock) { .... }
>
>    scoped_seqlock__read_irqsave(lock, flags) { ... }

OK. But if you don't object I'd like to avoid another DEFINE_LOCK_GUARD()
or something like it in this case. To me it won't buy anything.

And I think that the "generic" seqlock_read_section_retry(flags) makes
sense, the "if (flags)" checks should not generate the extra code.

Will you agree?

> (Maybe 'flags' can even be local to that scope?)

The problem is that you can't declare "int lockless/seq" and
"unsigned long flags" inside "for (...)",  but I'll try to think about
it more.

Oleg.

On Sun, 5 Oct 2025 at 09:09, Oleg Nesterov <oleg@redhat.com> wrote:
>
> OK. But if you don't object I'd like to avoid another DEFINE_LOCK_GUARD()
> or something like it in this case. To me it won't buy anything.

Oh, absolutely. I didn't mean that you actually *use* the fancy GUARD
infrastructure we have: this code doesn't do the whole '__cleanup()'
thing at all.

I only mean that as far as users are concerned, this all looks
similar, so please use a similar interface even if from an
implementation standpoint it is very different.

> The problem is that you can't declare "int lockless/seq" and
> "unsigned long flags" inside "for (...)",  but I'll try to think about
> it more.

I agree, it's an annoying limitation of the for-loop variable
declaration thing that you can only declare one type.

There's a somewhat strange solution to it, though: declare a structure
with multiple members of the types you want:

        for (struct { char *a; int b; } c = {NULL, 1}; ... }

and then you use 'c.a' and 'c.b' and friends inside the loop.

It looks pretty odd, but it actually works fine and we should probably
use it more.

[ The *really* hacky solution is to just use one type and pick the
biggest type and then possibly mis-use that type.

  That's effectively what you did with 'lockless': it's declared as an
'int', but it's really effectively used as a 'bool' anyway.

  Of course, in that 'lockless' case it's not as noticeable, since we
often use int/bool rather interchangeably in the kernel for historical
reasons anyway, so you probably didn't think of it as a hacky solution
]

               Linus