[PATCH 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()

Oleg Nesterov posted 4 patches 4 months ago
There is a newer version of this series
[PATCH 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()
Posted by Oleg Nesterov 4 months ago
The read_seqbegin/need_seqretry/done_seqretry API is cumbersome and
error prone. With the new helper the "typical" code like

	int seq, nextseq;
	unsigned long flags;

	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

	scoped_seqlock_read_irqsave (&seqlock) {
		// read-side critical section
	}

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

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 5ce48eab7a2a..9012702fd0a8 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -1209,4 +1209,65 @@ done_seqretry_irqrestore(seqlock_t *lock, int seq, unsigned long flags)
 	if (seq & 1)
 		read_sequnlock_excl_irqrestore(lock, flags);
 }
+
+/* internal helper for scoped_seqlock_read/scoped_seqlock_read_irqsave */
+static inline int
+scoped_seqlock_read_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 __scoped_seqlock_read(lock, lockless, seq)	\
+	for (int lockless = 1, seq = read_seqbegin(lock);		\
+	     lockless || scoped_seqlock_read_retry(lock, &seq, NULL);	\
+	     lockless = 0)
+
+/**
+ * scoped_seqlock_read(lock) - 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
+ *
+ * Example:
+ *
+ *	scoped_seqlock_read(&lock) {
+ *		// read-side critical section
+ *	}
+ *
+ * Starts with a lockless pass first. If it fails, restarts the critical
+ * section with the lock held.
+ *
+ * The critical section must not contain control flow that escapes the loop.
+ */
+#define scoped_seqlock_read(lock)	\
+	__scoped_seqlock_read(lock, __UNIQUE_ID(lockless), __UNIQUE_ID(seq))
+
+#define __scoped_seqlock_read_irqsave(lock, s)	\
+	for (struct { int lockless, seq; ulong flags; } s = { 1, read_seqbegin(lock) }; \
+	     s.lockless || scoped_seqlock_read_retry(lock, &s.seq, &s.flags);		\
+	     s.lockless = 0)
+
+/**
+ * scoped_seqlock_read_irqsave(lock) - same as scoped_seqlock_read() but
+ *                                     disables irqs on a locking pass
+ * @lock: pointer to the seqlock_t protecting the data
+ */
+#define scoped_seqlock_read_irqsave(lock)	\
+	__scoped_seqlock_read_irqsave(lock, __UNIQUE_ID(s))
+
 #endif /* __LINUX_SEQLOCK_H */
-- 
2.25.1.362.g51ebf55
Re: [PATCH 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()
Posted by Waiman Long 4 months ago
On 10/7/25 10:21 AM, Oleg Nesterov wrote:
> The read_seqbegin/need_seqretry/done_seqretry API is cumbersome and
> error prone. With the new helper the "typical" code like
>
> 	int seq, nextseq;
> 	unsigned long flags;
>
> 	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
>
> 	scoped_seqlock_read_irqsave (&seqlock) {
> 		// read-side critical section
> 	}
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   include/linux/seqlock.h | 61 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
>
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index 5ce48eab7a2a..9012702fd0a8 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -1209,4 +1209,65 @@ done_seqretry_irqrestore(seqlock_t *lock, int seq, unsigned long flags)
>   	if (seq & 1)
>   		read_sequnlock_excl_irqrestore(lock, flags);
>   }
> +
> +/* internal helper for scoped_seqlock_read/scoped_seqlock_read_irqsave */
> +static inline int
> +scoped_seqlock_read_retry(seqlock_t *lock, int *seq, unsigned long *flags)
I would suggest adding the "__" prefix to indicate that this is an 
internal helper that shouldn't be called directly.
> +{
> +	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 __scoped_seqlock_read(lock, lockless, seq)	\
> +	for (int lockless = 1, seq = read_seqbegin(lock);		\
> +	     lockless || scoped_seqlock_read_retry(lock, &seq, NULL);	\
> +	     lockless = 0)

I like Linus' suggestion of putting lockless and seq into a struct to 
make it more consistent with __scoped_seqlock_read_irqsave().

> +
> +/**
> + * scoped_seqlock_read(lock) - 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
> + *
> + * Example:
> + *
> + *	scoped_seqlock_read(&lock) {
> + *		// read-side critical section
> + *	}
> + *
> + * Starts with a lockless pass first. If it fails, restarts the critical
> + * section with the lock held.
> + *
> + * The critical section must not contain control flow that escapes the loop.
> + */
> +#define scoped_seqlock_read(lock)	\
> +	__scoped_seqlock_read(lock, __UNIQUE_ID(lockless), __UNIQUE_ID(seq))
> +
> +#define __scoped_seqlock_read_irqsave(lock, s)	\
> +	for (struct { int lockless, seq; ulong flags; } s = { 1, read_seqbegin(lock) }; \
> +	     s.lockless || scoped_seqlock_read_retry(lock, &s.seq, &s.flags);		\
> +	     s.lockless = 0)
> +
> +/**
> + * scoped_seqlock_read_irqsave(lock) - same as scoped_seqlock_read() but
> + *                                     disables irqs on a locking pass
> + * @lock: pointer to the seqlock_t protecting the data
Maybe we should we should add a comment saying that this API is similar 
to scoped_seqlock_read() but with irqs disabled.
> + */
> +#define scoped_seqlock_read_irqsave(lock)	\
> +	__scoped_seqlock_read_irqsave(lock, __UNIQUE_ID(s))
> +
>   #endif /* __LINUX_SEQLOCK_H */

Other than these minor nits, the patch looks good to me.

Cheers,
Longman
Re: [PATCH 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()
Posted by Oleg Nesterov 4 months ago
On 10/07, Waiman Long wrote:
>
> On 10/7/25 10:21 AM, Oleg Nesterov wrote:
> >+
> >+/* internal helper for scoped_seqlock_read/scoped_seqlock_read_irqsave */
> >+static inline int
> >+scoped_seqlock_read_retry(seqlock_t *lock, int *seq, unsigned long *flags)
> I would suggest adding the "__" prefix to indicate that this is an internal
> helper that shouldn't be called directly.

OK, I will add "__", but I thought that "internal helper" makes it clear that
it shouldn't be called directly. Nevermind, will do.

> >+#define __scoped_seqlock_read(lock, lockless, seq)	\
> >+	for (int lockless = 1, seq = read_seqbegin(lock);		\
> >+	     lockless || scoped_seqlock_read_retry(lock, &seq, NULL);	\
> >+	     lockless = 0)
>
> I like Linus' suggestion of putting lockless and seq into a struct to make
> it more consistent with __scoped_seqlock_read_irqsave().

Again, will do. See my reply to Linus.

> >+/**
> >+ * scoped_seqlock_read_irqsave(lock) - same as scoped_seqlock_read() but
> >+ *                                     disables irqs on a locking pass
> >+ * @lock: pointer to the seqlock_t protecting the data
> Maybe we should we should add a comment saying that this API is similar to
> scoped_seqlock_read() but with irqs disabled.

Hmm... This is what the comment above tries to say... Do you think it can
be improved?

Oleg.
Re: [PATCH 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()
Posted by Waiman Long 4 months ago
On 10/7/25 1:18 PM, Oleg Nesterov wrote:
> On 10/07, Waiman Long wrote:
>> On 10/7/25 10:21 AM, Oleg Nesterov wrote:
>>> +
>>> +/* internal helper for scoped_seqlock_read/scoped_seqlock_read_irqsave */
>>> +static inline int
>>> +scoped_seqlock_read_retry(seqlock_t *lock, int *seq, unsigned long *flags)
>> I would suggest adding the "__" prefix to indicate that this is an internal
>> helper that shouldn't be called directly.
> OK, I will add "__", but I thought that "internal helper" makes it clear that
> it shouldn't be called directly. Nevermind, will do.
>
>>> +#define __scoped_seqlock_read(lock, lockless, seq)	\
>>> +	for (int lockless = 1, seq = read_seqbegin(lock);		\
>>> +	     lockless || scoped_seqlock_read_retry(lock, &seq, NULL);	\
>>> +	     lockless = 0)
>> I like Linus' suggestion of putting lockless and seq into a struct to make
>> it more consistent with __scoped_seqlock_read_irqsave().
> Again, will do. See my reply to Linus.
>
>>> +/**
>>> + * scoped_seqlock_read_irqsave(lock) - same as scoped_seqlock_read() but
>>> + *                                     disables irqs on a locking pass
>>> + * @lock: pointer to the seqlock_t protecting the data
>> Maybe we should we should add a comment saying that this API is similar to
>> scoped_seqlock_read() but with irqs disabled.
> Hmm... This is what the comment above tries to say... Do you think it can
> be improved?

Sorry, I missed that. Never mind :-)

Cheers,
Longman