Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 05:58:13PM +0200, Ahmed S. Darwish wrote:
> > I even considered:
> >
> > #define __SEQPROP(name, prop, expr) \
> > static __always_inline __seqprop_##prop##_t \
> > __seqprop##name##_##prop(seqcount##name##_t *s) \
> > { \
> > expr; \
> > }
> >
> > Such that we could write:
> >
> > __SEQPROP(, ptr, return s)
> > __SEQPROP(, preempt, return false)
> > __SEQPROP(, assert, )
> >
> > __SEQPROP(_##locktype, ptr, return >seqcount) \
> > __SEQPROP(_##locktype, preempt, return preempt) \
> > __SEQPROP(_##locktype, assert,
> > __SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember))) \
> >
> > But I figured _that_ might've been one step too far ;-)
>
> Initially I implemented something like this during internal,
> pre-upstream, versions of this patch series. We've decided afterwards
> that the macro compression level is so high that the whole thing is not
> so easily understandable.
I've been reading too much tracing code lately... :-) This is only 2
levels of expansion and fits on a single screen.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 05:58:13PM +0200, Ahmed S. Darwish wrote: > On Wed, Jul 08, 2020 at 05:35:22PM +0200, Peter Zijlstra wrote: > ... > > > > And while the gcc-4.8 code is horrendous crap, the rest should be pretty > > straight forward and concentrates on the pieces where there are > > differences. > > > > Is there any possibility of upgrading the minimum gcc version to 4.9? Is > there any supported architecture that is still stuck on 4.8? Upgrading also got mention here: https://lkml.kernel.org/r/CAHk-=wgd+q+oddtukyc74_cdx5i0ynf0glhune2faaokejj...@mail.gmail.com But it didn't get much response.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 05:35:22PM +0200, Peter Zijlstra wrote:
> But I figured _that_ might've been one step too far ;-)
Damn, now you made me do it... and it's not too horrible. Included the
-rt part.
---
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 8b97204f35a7..cc15a6aaab00 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -1,36 +1,15 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef __LINUX_SEQLOCK_H
#define __LINUX_SEQLOCK_H
+
/*
- * Reader/writer consistent mechanism without starving writers. This type of
- * lock for data where the reader wants a consistent set of information
- * and is willing to retry if the information changes. There are two types
- * of readers:
- * 1. Sequence readers which never block a writer but they may have to retry
- *if a writer is in progress by detecting change in sequence number.
- *Writers do not wait for a sequence reader.
- * 2. Locking readers which will wait if a writer or another locking reader
- *is in progress. A locking reader in progress will also block a writer
- *from going forward. Unlike the regular rwlock, the read lock here is
- *exclusive so that only one locking reader can get it.
- *
- * This is not as cache friendly as brlock. Also, this may not work well
- * for data that contains pointers, because any writer could
- * invalidate a pointer that a reader was following.
- *
- * Expected non-blocking reader usage:
- * do {
- * seq = read_seqbegin();
- * ...
- * } while (read_seqretry(, seq));
+ * seqcount_t / seqlock_t - a reader-writer consistency mechanism with
+ * lockless readers (read-only retry loops), and no writer starvation.
*
+ * See Documentation/locking/seqlock.rst for full description.
*
- * On non-SMP the spin locks disappear but the writer still needs
- * to increment the sequence variables because an interrupt routine could
- * change the state of the data.
- *
- * Based on x86_64 vsyscall gettimeofday
- * by Keith Owens and Andrea Arcangeli
+ * Copyrights:
+ * - Based on x86_64 vsyscall gettimeofday: Keith Owens, Andrea Arcangeli
*/
#include
@@ -41,8 +20,8 @@
#include
/*
- * The seqlock interface does not prescribe a precise sequence of read
- * begin/retry/end. For readers, typically there is a call to
+ * The seqlock seqcount_t interface does not prescribe a precise sequence of
+ * read begin/retry/end. For readers, typically there is a call to
* read_seqcount_begin() and read_seqcount_retry(), however, there are more
* esoteric cases which do not follow this pattern.
*
@@ -56,10 +35,28 @@
#define KCSAN_SEQLOCK_REGION_MAX 1000
/*
- * Version using sequence counter only.
- * This can be used when code has its own mutex protecting the
- * updating starting before the write_seqcountbeqin() and ending
- * after the write_seqcount_end().
+ * Sequence counters (seqcount_t)
+ *
+ * This is the raw counting mechanism, without any writer protection.
+ *
+ * Write side critical sections must be serialized and non-preemptible.
+ *
+ * If readers can be invoked from hardirq or softirq contexts,
+ * interrupts or bottom halves must also be respectively disabled before
+ * entering the write section.
+ *
+ * This mechanism can't be used if the protected data contains pointers,
+ * as the writer can invalidate a pointer that a reader is following.
+ *
+ * If the write serialization mechanism is one of the common kernel
+ * locking primitives, use a sequence counter with associated lock
+ * (seqcount_LOCKTYPE_t) instead.
+ *
+ * If it's desired to automatically handle the sequence counter writer
+ * serialization and non-preemptibility requirements, use a sequential
+ * lock (seqlock_t) instead.
+ *
+ * See Documentation/locking/seqlock.rst
*/
typedef struct seqcount {
unsigned sequence;
@@ -82,6 +79,10 @@ static inline void __seqcount_init(seqcount_t *s, const char
*name,
# define SEQCOUNT_DEP_MAP_INIT(lockname) \
.dep_map = { .name = #lockname } \
+/**
+ * seqcount_init() - runtime initializer for seqcount_t
+ * @s: Pointer to the seqcount_t instance
+ */
# define seqcount_init(s) \
do {\
static struct lock_class_key __key; \
@@ -105,12 +106,139 @@ static inline void seqcount_lockdep_reader_access(const
seqcount_t *s)
# define seqcount_lockdep_reader_access(x)
#endif
-#define SEQCNT_ZERO(lockname) { .sequence = 0, SEQCOUNT_DEP_MAP_INIT(lockname)}
+/**
+ * SEQCNT_ZERO() - static initializer for seqcount_t
+ * @name: Name of the seqcount_t instance
+ */
+#define SEQCNT_ZERO(name) { .sequence = 0, SEQCOUNT_DEP_MAP_INIT(name) }
+
+/*
+ * Sequence counters with associated locks (seqcount_LOCKTYPE_t)
+ *
+ * A sequence counter which associates the lock used for writer
+ * serialization at initialization time. This enables lockdep to validate
+ * that the write side critical section is properly
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 05:35:22PM +0200, Peter Zijlstra wrote:
...
>
> And while the gcc-4.8 code is horrendous crap, the rest should be pretty
> straight forward and concentrates on the pieces where there are
> differences.
>
Is there any possibility of upgrading the minimum gcc version to 4.9? Is
there any supported architecture that is still stuck on 4.8?
...
> I even considered:
>
> #define __SEQPROP(name, prop, expr) \
> static __always_inline __seqprop_##prop##_t \
> __seqprop##name##_##prop(seqcount##name##_t *s) \
> { \
> expr; \
> }
>
> Such that we could write:
>
> __SEQPROP(, ptr, return s)
> __SEQPROP(, preempt, return false)
> __SEQPROP(, assert, )
>
> __SEQPROP(_##locktype, ptr, return >seqcount) \
> __SEQPROP(_##locktype, preempt, return preempt) \
> __SEQPROP(_##locktype, assert,
> __SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember))) \
>
> But I figured _that_ might've been one step too far ;-)
Initially I implemented something like this during internal,
pre-upstream, versions of this patch series. We've decided afterwards
that the macro compression level is so high that the whole thing is not
so easily understandable.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 05:09:30PM +0200, Ahmed S. Darwish wrote:
> On Wed, Jul 08, 2020 at 02:29:38PM +0200, Peter Zijlstra wrote:
> > How about something disguisting like this then?
> >
> ...
> > #define __SEQ_RTIS_BUILTIN(CONFIG_PREEMPT_RT)
> >
> > SEQCOUNT_LOCKTYPE(raw_spinlock, raw_spinlock_t, false, lock)
> > SEQCOUNT_LOCKTYPE(spinlock, spinlock_t, __SEQ_RT, lock)
> > SEQCOUNT_LOCKTYPE(rwlock, rwlock_t, __SEQ_RT, lock)
> > SEQCOUNT_LOCKTYPE(mutex,struct mutex, true, lock)
> > SEQCOUNT_LOCKTYPE(ww_mutex, struct ww_mutex,true,
> > lock->base)
> >
> > #if (defined(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < 40900) ||
> > defined(__CHECKER__)
> >
> > #define __seqprop_pick(const_expr, s, locktype, prop, otherwise)\
> > __builtin_choose_expr(const_expr, \
> > __seqprop_##locktype##_##prop((void *)(s)), \
> > otherwise)
> >
> > extern void __seqprop_invalid(void);
> >
> > #define __seqprop(s, prop)
> > \
> > __seqprop_pick(__same_type(*(s), seqcount_t), (s), seqcount, prop,
> > \
> > __seqprop_pick(__same_type(*(s), seqcount_raw_spinlock_t), (s),
> > raw_spinlock, prop, \
> > __seqprop_pick(__same_type(*(s), seqcount_spinlock_t), (s),
> > spinlock, prop, \
> > __seqprop_pick(__same_type(*(s), seqcount_rwlock_t), (s), rwlock,
> > prop, \
> > __seqprop_pick(__same_type(*(s), seqcount_mutex_t), (s), mutex,
> > prop, \
> > __seqprop_pick(__same_type(*(s), seqcount_ww_mutex_t), (s),
> > ww_mutex, prop, \
> > __seqprop_invalid()))
> >
> > #else
> >
> > #define __seqprop_case(s, locktype, prop) \
> > seqcount_##locktype##_t: __seqprop_##locktype##_##prop((void *)s)
> >
> > #define __seqprop(s, prop) \
> > _Generic(*(s), \
> > seqcount_t: __seqprop_seqcount_##prop((void*)s),\
> > __seqprop_case((s), raw_spinlock, prop), \
> > __seqprop_case((s), spinlock, prop), \
> > __seqprop_case((s), rwlock, prop), \
> > __seqprop_case((s), mutex, prop), \
> > __seqprop_case((s), ww_mutex, prop))
> >
> > #endif
> >
> > #define __to_seqcount_t(s) __seqprop(s, ptr)
> > #define __associated_lock_is_preemptible(s) __seqprop(s, preempt)
> > #define __assert_associated_lock_held(s)__seqprop(s, assert)
>
> Hmm, I'll prototype the whole thing (along with PREEMPT_RT associated
> lock()/unlock() as you've mentioned in the other e-mail), and come back.
>
> Honestly, I have a first impression that this is heading into too much
> complexity and compaction, but let's finish the whole thing first.
So the thing I pasted compiles kernel/sched/cputime.o, but that only
uses the old seqcount_t thing, not any of the fancy new stuff, still the
compiler groks it all.
And while the gcc-4.8 code is horrendous crap, the rest should be pretty
straight forward and concentrates on the pieces where there are
differences.
I even considered:
#define __SEQPROP(name, prop, expr) \
static __always_inline __seqprop_##prop##_t \
__seqprop##name##_##prop(seqcount##name##_t *s) \
{ \
expr; \
}
Such that we could write:
__SEQPROP(, ptr, return s)
__SEQPROP(, preempt, return false)
__SEQPROP(, assert, )
__SEQPROP(_##locktype, ptr, return >seqcount) \
__SEQPROP(_##locktype, preempt, return preempt) \
__SEQPROP(_##locktype, assert,
__SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember))) \
But I figured _that_ might've been one step too far ;-)
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 02:29:38PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 08, 2020 at 12:33:14PM +0200, Ahmed S. Darwish wrote:
>
> > > +#define read_seqcount_begin(s)
> > > do_read_seqcount_begin(__to_seqcount_t(s))
> > > +
> > > +static inline unsigned do_read_seqcount_begin(const seqcount_t *s)
> > > +{
> > ...
> >
> > Hmm, the __to_seqcount_t(s) force cast is not good. It will break the
> > arguments type-safety of seqcount macros that do not have either:
> >
> > __associated_lock_is_preemptible() or
> > __assert_associated_lock_held()
> >
> > in their path. This basically includes all the read path macros, and
> > even some others (e.g. write_seqcount_invalidate()).
> >
> > With the suggested force cast above, I can literally *pass anything* to
> > read_seqcount_begin() and friends, and the compiler won't say a thing.
> >
> > So, I'll restore __to_seqcount_t(s) that to its original implementation:
>
> Right, I figured that the write side would be enough to catch glaring
> abuse. But sure.
>
> It's a bummer we didn't upgrade the minimum compiler version to 4.9,
> that would've given us _Generic(), which allows one to write this
> slightly less verbose I think.
>
Looking at 5429ef62bcf3 ("compiler/gcc: Raise minimum GCC version for
kernel builds to 4.8"), it seems that the decision of picking gcc 4.8
vs. 4.9 was kinda arbitrary.
Anyway, please continue below.
> How about something disguisting like this then?
>
...
> #define __SEQ_RT IS_BUILTIN(CONFIG_PREEMPT_RT)
>
> SEQCOUNT_LOCKTYPE(raw_spinlock, raw_spinlock_t, false, lock)
> SEQCOUNT_LOCKTYPE(spinlock, spinlock_t, __SEQ_RT, lock)
> SEQCOUNT_LOCKTYPE(rwlock, rwlock_t, __SEQ_RT, lock)
> SEQCOUNT_LOCKTYPE(mutex, struct mutex, true, lock)
> SEQCOUNT_LOCKTYPE(ww_mutex, struct ww_mutex,true,
> lock->base)
>
> #if (defined(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < 40900) ||
> defined(__CHECKER__)
>
> #define __seqprop_pick(const_expr, s, locktype, prop, otherwise) \
> __builtin_choose_expr(const_expr, \
> __seqprop_##locktype##_##prop((void *)(s)), \
> otherwise)
>
> extern void __seqprop_invalid(void);
>
> #define __seqprop(s, prop)
> \
> __seqprop_pick(__same_type(*(s), seqcount_t), (s), seqcount, prop,
> \
> __seqprop_pick(__same_type(*(s), seqcount_raw_spinlock_t), (s),
> raw_spinlock, prop, \
> __seqprop_pick(__same_type(*(s), seqcount_spinlock_t), (s),
> spinlock, prop, \
> __seqprop_pick(__same_type(*(s), seqcount_rwlock_t), (s), rwlock,
> prop, \
> __seqprop_pick(__same_type(*(s), seqcount_mutex_t), (s), mutex,
> prop, \
> __seqprop_pick(__same_type(*(s), seqcount_ww_mutex_t), (s),
> ww_mutex, prop, \
> __seqprop_invalid()))
>
> #else
>
> #define __seqprop_case(s, locktype, prop) \
> seqcount_##locktype##_t: __seqprop_##locktype##_##prop((void *)s)
>
> #define __seqprop(s, prop)\
> _Generic(*(s), \
>seqcount_t: __seqprop_seqcount_##prop((void*)s),\
>__seqprop_case((s), raw_spinlock, prop), \
>__seqprop_case((s), spinlock, prop), \
>__seqprop_case((s), rwlock, prop), \
>__seqprop_case((s), mutex, prop), \
>__seqprop_case((s), ww_mutex, prop))
>
> #endif
>
> #define __to_seqcount_t(s)__seqprop(s, ptr)
> #define __associated_lock_is_preemptible(s) __seqprop(s, preempt)
> #define __assert_associated_lock_held(s) __seqprop(s, assert)
Hmm, I'll prototype the whole thing (along with PREEMPT_RT associated
lock()/unlock() as you've mentioned in the other e-mail), and come back.
Honestly, I have a first impression that this is heading into too much
complexity and compaction, but let's finish the whole thing first.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 04:13:32PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 08, 2020 at 02:29:38PM +0200, Peter Zijlstra wrote:
>
> > #define SEQCOUNT_LOCKTYPE(name, locktype, preempt, lockmember)
> > \
> > typedef struct seqcount_##name {\
> > seqcount_t seqcount; \
> > __SEQCOUNT_LOCKDEP(locktype *lock); \
> > } seqcount_##name##_t;
> > \
> > \
> > static __always_inline void \
> > seqcount_##name##_init(seqcount_##name##_t *s, locktype *l) \
> > { \
> > seqcount_init(>seqcount);\
> > __SEQCOUNT_LOCKDEP(s->lock = l);\
> > } \
> > \
> > static __always_inline __seqprop_ptr_t
> > \
> > __seqprop_##name##_ptr(seqcount_##name##_t *s)
> > \
> > { \
> > return >seqcount;\
> > } \
> > \
> > static __always_inline __seqprop_preempt_t \
> > __seqprop_##name##_preempt(seqcount_##name##_t *s) \
> > { \
> > return preempt; \
> > } \
> > \
> > static __always_inline __seqprop_assert_t \
> > __seqprop_##name##_assert(seqcount_##name##_t *s) \
> > { \
> > __SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember)); \
> > }
>
> For PREEMPT_RT's magic thing, you can add:
>
> static __always_inline void \
> __seqprop_##name##_lock(seqcount_##name##_t *s)
> \
> { \
> if (!__SEQ_RT || !preempt) \
> return; \
> \
> lockbase##_lock(>lock); \
> lockbase##_unlock(>lock);\
> }
>
> and:
>
> #define __rt_lock_unlock_associated_sleeping_lock(s) __seqprop(s, lock)
Or possible have it like:
if (!__SEQ_RT || !preempt)
return smp_cond_load_relaxed(>seqcount->sequence, !(VAL &
1));
lockbase##_lock(>lock);
lockbase##_unlock(>lock);
return READ_ONCE(s->seqcount->sequence);
and then replace most of __read_seqcount_begin() with it.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 02:29:38PM +0200, Peter Zijlstra wrote:
> #define SEQCOUNT_LOCKTYPE(name, locktype, preempt, lockmember)
> \
> typedef struct seqcount_##name { \
> seqcount_t seqcount; \
> __SEQCOUNT_LOCKDEP(locktype *lock); \
> } seqcount_##name##_t;
> \
> \
> static __always_inline void \
> seqcount_##name##_init(seqcount_##name##_t *s, locktype *l) \
> { \
> seqcount_init(>seqcount);\
> __SEQCOUNT_LOCKDEP(s->lock = l);\
> } \
> \
> static __always_inline __seqprop_ptr_t
> \
> __seqprop_##name##_ptr(seqcount_##name##_t *s)
> \
> { \
> return >seqcount;\
> } \
> \
> static __always_inline __seqprop_preempt_t\
> __seqprop_##name##_preempt(seqcount_##name##_t *s)\
> { \
> return preempt; \
> } \
> \
> static __always_inline __seqprop_assert_t \
> __seqprop_##name##_assert(seqcount_##name##_t *s) \
> { \
> __SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember)); \
> }
For PREEMPT_RT's magic thing, you can add:
static __always_inline void \
__seqprop_##name##_lock(seqcount_##name##_t *s) \
{ \
if (!__SEQ_RT || !preempt) \
return; \
\
lockbase##_lock(>lock); \
lockbase##_unlock(>lock);\
}
and:
#define __rt_lock_unlock_associated_sleeping_lock(s) __seqprop(s, lock)
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 12:33:14PM +0200, Ahmed S. Darwish wrote:
> > +#define read_seqcount_begin(s)
> > do_read_seqcount_begin(__to_seqcount_t(s))
> > +
> > +static inline unsigned do_read_seqcount_begin(const seqcount_t *s)
> > +{
> ...
>
> Hmm, the __to_seqcount_t(s) force cast is not good. It will break the
> arguments type-safety of seqcount macros that do not have either:
>
> __associated_lock_is_preemptible() or
> __assert_associated_lock_held()
>
> in their path. This basically includes all the read path macros, and
> even some others (e.g. write_seqcount_invalidate()).
>
> With the suggested force cast above, I can literally *pass anything* to
> read_seqcount_begin() and friends, and the compiler won't say a thing.
>
> So, I'll restore __to_seqcount_t(s) that to its original implementation:
Right, I figured that the write side would be enough to catch glaring
abuse. But sure.
It's a bummer we didn't upgrade the minimum compiler version to 4.9,
that would've given us _Generic(), which allows one to write this
slightly less verbose I think.
How about something disguisting like this then?
#ifdef CONFIG_LOCKDEP
#define __SEQCOUNT_LOCKDEP(expr)expr
#else
#define __SEQCOUNT_LOCKDEP(expr)
#endif
typedef seqcount_t * __seqprop_ptr_t;
typedef bool __seqprop_preempt_t;
typedef void __seqprop_assert_t;
static __always_inline __seqprop_ptr_t
__seqprop_seqcount_ptr(seqcount_t *s)
{
return s;
}
static __always_inline __seqprop_preempt_t
__seqprop_seqcount_preempt(seqcount_t *s)
{
return false;
}
static __always_inline __seqprop_assert_t
__seqprop_seqcount_assert(seqcount_t *s)
{
}
#define SEQCOUNT_LOCKTYPE(name, locktype, preempt, lockmember) \
typedef struct seqcount_##name {\
seqcount_t seqcount; \
__SEQCOUNT_LOCKDEP(locktype *lock); \
} seqcount_##name##_t; \
\
static __always_inline void \
seqcount_##name##_init(seqcount_##name##_t *s, locktype *l) \
{ \
seqcount_init(>seqcount);\
__SEQCOUNT_LOCKDEP(s->lock = l);\
} \
\
static __always_inline __seqprop_ptr_t \
__seqprop_##name##_ptr(seqcount_##name##_t *s) \
{ \
return >seqcount;\
} \
\
static __always_inline __seqprop_preempt_t \
__seqprop_##name##_preempt(seqcount_##name##_t *s) \
{ \
return preempt; \
} \
\
static __always_inline __seqprop_assert_t \
__seqprop_##name##_assert(seqcount_##name##_t *s) \
{ \
__SEQCOUNT_LOCKDEP(lockdep_assert_held(s->lockmember)); \
}
#define SEQCNT_LOCKTYPE_ZERO(_name, _lock) {\
.seqcount = SEQCNT_ZERO(_name.seqcount),\
__SEQCOUNT_LOCKDEP(.lock = (_lock)) \
}
#include
#include
#define __SEQ_RTIS_BUILTIN(CONFIG_PREEMPT_RT)
SEQCOUNT_LOCKTYPE(raw_spinlock, raw_spinlock_t, false, lock)
SEQCOUNT_LOCKTYPE(spinlock, spinlock_t, __SEQ_RT, lock)
SEQCOUNT_LOCKTYPE(rwlock, rwlock_t, __SEQ_RT, lock)
SEQCOUNT_LOCKTYPE(mutex,struct mutex, true, lock)
SEQCOUNT_LOCKTYPE(ww_mutex, struct ww_mutex,true, lock->base)
#if (defined(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < 40900) ||
defined(__CHECKER__)
#define __seqprop_pick(const_expr, s, locktype, prop, otherwise)\
__builtin_choose_expr(const_expr, \
__seqprop_##locktype##_##prop((void *)(s)), \
otherwise)
extern void __seqprop_invalid(void);
#define __seqprop(s, prop)
\
__seqprop_pick(__same_type(*(s),
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Wed, Jul 08, 2020 at 11:12:01AM +0200, Peter Zijlstra wrote: > On Tue, Jul 07, 2020 at 04:37:26PM +0200, Peter Zijlstra wrote: > > + * Use the ``_irqsave`` and ``_bh`` variants instead if the read side > > + * ``_bh`` variant of write_seqlock(). Use only if the read side section > > + * ``_irq`` variant of write_sequnlock(). The write side section of > > + * ``_irqsave`` variant of write_seqlock(). Use if the read section of > > + * ``_irqrestore`` variant of write_sequnlock(). The write section of > > + * ``_bh`` variant of read_seqlock_excl(). Use this variant if the > > + * ``_bh`` variant of read_sequnlock_excl(). The closed section must've > > + * ``_irq`` variant of read_seqlock_excl(). Use this only if the > > + * ``_irq`` variant of read_sequnlock_excl(). The closed section must've > > + * ``_irqsave`` variant of read_seqlock_excl(). Use this only if the > > + * ``_irqrestore`` variant of read_sequnlock_excl(). The closed section > > + * This is the ``_irqsave`` variant of read_seqbegin_or_lock(). Use if > > + * This is the ``_irqrestore`` variant of done_seqretry(). The read > > Can we also get rid of that '' nonsense, the gods of ASCII gifted us " > for this. Hehe, why not. I welcome back our ASCII overlords.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Tue, Jul 07, 2020 at 04:37:26PM +0200, Peter Zijlstra wrote:
>
> How's this? it removes a level of indirection and a bunch of repetition.
ACK, for the extra level of indirection removed.
> It's also more than 200 lines shorter.
...
> +#define __to_seqcount_t(s) (seqcount_t *)(s)
...
> +#define read_seqcount_begin(s)
> do_read_seqcount_begin(__to_seqcount_t(s))
> +
> +static inline unsigned do_read_seqcount_begin(const seqcount_t *s)
> +{
...
Hmm, the __to_seqcount_t(s) force cast is not good. It will break the
arguments type-safety of seqcount macros that do not have either:
__associated_lock_is_preemptible() or
__assert_associated_lock_held()
in their path. This basically includes all the read path macros, and
even some others (e.g. write_seqcount_invalidate()).
With the suggested force cast above, I can literally *pass anything* to
read_seqcount_begin() and friends, and the compiler won't say a thing.
So, I'll restore __to_seqcount_t(s) that to its original implementation:
/*
* @s: pointer to seqcount_t or any of the seqcount_locktype_t variants
*/
#define __to_seqcount_t(s) \
({ \
seqcount_t *seq;\
\
if (__same_type(*(s), seqcount_t)) \
seq = (seqcount_t *)(s);\
else if (__same_type(*(s), seqcount_spinlock_t))\
seq = &((seqcount_spinlock_t *)(s))->seqcount; \
else if (__same_type(*(s), seqcount_raw_spinlock_t))\
seq = &((seqcount_raw_spinlock_t *)(s))->seqcount; \
else if (__same_type(*(s), seqcount_rwlock_t)) \
seq = &((seqcount_rwlock_t *)(s))->seqcount;\
else if (__same_type(*(s), seqcount_mutex_t)) \
seq = &((seqcount_mutex_t *)(s))->seqcount; \
else if (__same_type(*(s), seqcount_ww_mutex_t))\
seq = &((seqcount_ww_mutex_t *)(s))->seqcount; \
else\
BUILD_BUG_ON_MSG(1, "Unknown seqcount type"); \
\
seq;\
})
Yes, I know, it's not the prettiest thing in the world, but I'd take
ugly over breaking the compiler type checks any day.
>
> It doesn't provide SEQCNT_LOCKTYPE_ZERO() for each LOCKTYPE, but you can
> use this one macro for any LOCKTYPE.
>
>From call-sites perspectives, SEQCNT_SPINLOCK_ZERO() is much more
readable than SEQCNT_LOCKTYPE_ZERO() and so on. It only costs us 5 lines
*for all* the seqcount lock types. IMHO it's worth the trade-off.
>
> So I applied it all yesterday and tried to make sense of the resulting
> indirections and couldn't find stuff -- because it was hidding in
> another file.
>
> Specifically I disliked that *seqcount_t* naming and didn't see how it
> all connected.
>
Hmm, the idea was that write_seqcount_t_begin() and friends applies only
to plain old "seqcount_t". But, yes, I agree, it's confusing as hell.
The way you've organized the macros is much more readable, so thanks a
lot, I'll incorporate that in v4.
Kind regards,
--
Ahmed S. Darwish
Linutronix GmbH
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Tue, Jul 07, 2020 at 04:37:26PM +0200, Peter Zijlstra wrote: > + * Use the ``_irqsave`` and ``_bh`` variants instead if the read side > + * ``_bh`` variant of write_seqlock(). Use only if the read side section > + * ``_irq`` variant of write_sequnlock(). The write side section of > + * ``_irqsave`` variant of write_seqlock(). Use if the read section of > + * ``_irqrestore`` variant of write_sequnlock(). The write section of > + * ``_bh`` variant of read_seqlock_excl(). Use this variant if the > + * ``_bh`` variant of read_sequnlock_excl(). The closed section must've > + * ``_irq`` variant of read_seqlock_excl(). Use this only if the > + * ``_irq`` variant of read_sequnlock_excl(). The closed section must've > + * ``_irqsave`` variant of read_seqlock_excl(). Use this only if the > + * ``_irqrestore`` variant of read_sequnlock_excl(). The closed section > + * This is the ``_irqsave`` variant of read_seqbegin_or_lock(). Use if > + * This is the ``_irqrestore`` variant of done_seqretry(). The read Can we also get rid of that '' nonsense, the gods of ASCII gifted us " for this.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Tue, Jul 07, 2020 at 03:04:10PM +0200, Peter Zijlstra wrote:
> Anyway, let me muck around with that a bit.
How's this? it removes a level of indirection and a bunch of repetition.
It doesn't provide SEQCNT_LOCKTYPE_ZERO() for each LOCKTYPE, but you can
use this one macro for any LOCKTYPE.
It's also more than 200 lines shorter.
---
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 8b97204f35a7..2b599cf03db4 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -1,36 +1,15 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef __LINUX_SEQLOCK_H
#define __LINUX_SEQLOCK_H
+
/*
- * Reader/writer consistent mechanism without starving writers. This type of
- * lock for data where the reader wants a consistent set of information
- * and is willing to retry if the information changes. There are two types
- * of readers:
- * 1. Sequence readers which never block a writer but they may have to retry
- *if a writer is in progress by detecting change in sequence number.
- *Writers do not wait for a sequence reader.
- * 2. Locking readers which will wait if a writer or another locking reader
- *is in progress. A locking reader in progress will also block a writer
- *from going forward. Unlike the regular rwlock, the read lock here is
- *exclusive so that only one locking reader can get it.
- *
- * This is not as cache friendly as brlock. Also, this may not work well
- * for data that contains pointers, because any writer could
- * invalidate a pointer that a reader was following.
- *
- * Expected non-blocking reader usage:
- * do {
- * seq = read_seqbegin();
- * ...
- * } while (read_seqretry(, seq));
+ * seqcount_t / seqlock_t - a reader-writer consistency mechanism with
+ * lockless readers (read-only retry loops), and no writer starvation.
*
+ * See Documentation/locking/seqlock.rst for full description.
*
- * On non-SMP the spin locks disappear but the writer still needs
- * to increment the sequence variables because an interrupt routine could
- * change the state of the data.
- *
- * Based on x86_64 vsyscall gettimeofday
- * by Keith Owens and Andrea Arcangeli
+ * Copyrights:
+ * - Based on x86_64 vsyscall gettimeofday: Keith Owens, Andrea Arcangeli
*/
#include
@@ -41,8 +20,8 @@
#include
/*
- * The seqlock interface does not prescribe a precise sequence of read
- * begin/retry/end. For readers, typically there is a call to
+ * The seqlock seqcount_t interface does not prescribe a precise sequence of
+ * read begin/retry/end. For readers, typically there is a call to
* read_seqcount_begin() and read_seqcount_retry(), however, there are more
* esoteric cases which do not follow this pattern.
*
@@ -56,10 +35,28 @@
#define KCSAN_SEQLOCK_REGION_MAX 1000
/*
- * Version using sequence counter only.
- * This can be used when code has its own mutex protecting the
- * updating starting before the write_seqcountbeqin() and ending
- * after the write_seqcount_end().
+ * Sequence counters (seqcount_t)
+ *
+ * This is the raw counting mechanism, without any writer protection.
+ *
+ * Write side critical sections must be serialized and non-preemptible.
+ *
+ * If readers can be invoked from hardirq or softirq contexts,
+ * interrupts or bottom halves must also be respectively disabled before
+ * entering the write section.
+ *
+ * This mechanism can't be used if the protected data contains pointers,
+ * as the writer can invalidate a pointer that a reader is following.
+ *
+ * If the write serialization mechanism is one of the common kernel
+ * locking primitives, use a sequence counter with associated lock
+ * (seqcount_LOCKTYPE_t) instead.
+ *
+ * If it's desired to automatically handle the sequence counter writer
+ * serialization and non-preemptibility requirements, use a sequential
+ * lock (seqlock_t) instead.
+ *
+ * See Documentation/locking/seqlock.rst
*/
typedef struct seqcount {
unsigned sequence;
@@ -82,6 +79,10 @@ static inline void __seqcount_init(seqcount_t *s, const char
*name,
# define SEQCOUNT_DEP_MAP_INIT(lockname) \
.dep_map = { .name = #lockname } \
+/**
+ * seqcount_init() - runtime initializer for seqcount_t
+ * @s: Pointer to the seqcount_t instance
+ */
# define seqcount_init(s) \
do {\
static struct lock_class_key __key; \
@@ -105,12 +106,122 @@ static inline void seqcount_lockdep_reader_access(const
seqcount_t *s)
# define seqcount_lockdep_reader_access(x)
#endif
-#define SEQCNT_ZERO(lockname) { .sequence = 0, SEQCOUNT_DEP_MAP_INIT(lockname)}
+/**
+ * SEQCNT_ZERO() - static initializer for seqcount_t
+ * @name: Name of the seqcount_t instance
+ */
+#define SEQCNT_ZERO(name) { .sequence = 0, SEQCOUNT_DEP_MAP_INIT(name) }
+
+/*
+ * Sequence counters with associated locks (seqcount_LOCKTYPE_t)
+ *
+ * A sequence counter which associates the lock used for writer
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Tue, Jul 07, 2020 at 10:40:24AM +0200, Ahmed S. Darwish wrote: > On Mon, Jul 06, 2020 at 11:21:48PM +0200, Peter Zijlstra wrote: > > On Tue, Jun 30, 2020 at 07:44:38AM +0200, Ahmed S. Darwish wrote: > > > +#include > > > > Why? why not include those lines directly here? Having it in a separate > > file actually makes it harder to read. > > > > The seqlock_types_internal.h file contains mostly a heavy sequence of > typeof() branching macros, which is not related to the core logic of > sequence counters or sequential locks: > IMHO it makes sense to isolate these "not core logic" parts. Adding all > of this to plain seqlock.h makes it almost unreadable. So I applied it all yesterday and tried to make sense of the resulting indirections and couldn't find stuff -- because it was hidding in another file. Specifically I disliked that *seqcount_t* naming and didn't see how it all connected. And that other file is less than 200 lines, which resulted in me wondering why it needed to be hidden away like that. Anyway, let me muck around with that a bit.
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Mon, Jul 06, 2020 at 11:21:48PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 30, 2020 at 07:44:38AM +0200, Ahmed S. Darwish wrote:
> > +#include
>
> Why? why not include those lines directly here? Having it in a separate
> file actually makes it harder to read.
>
The seqlock_types_internal.h file contains mostly a heavy sequence of
typeof() branching macros, which is not related to the core logic of
sequence counters or sequential locks:
#define __to_seqcount_t(s)
({
seqcount_t *seq;
if (__same_type(*(s), seqcount_t))
seq = (seqcount_t *)(s);
else if (__same_type(*(s), seqcount_spinlock_t))
seq = &((seqcount_spinlock_t *)(s))->seqcount;
else if (__same_type(*(s), seqcount_raw_spinlock_t))
seq = &((seqcount_raw_spinlock_t *)(s))->seqcount;
else if (__same_type(*(s), seqcount_rwlock_t))
seq = &((seqcount_rwlock_t *)(s))->seqcount;
else if (__same_type(*(s), seqcount_mutex_t))
seq = &((seqcount_mutex_t *)(s))->seqcount;
else if (__same_type(*(s), seqcount_ww_mutex_t))
seq = &((seqcount_ww_mutex_t *)(s))->seqcount;
else
BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
seq;
})
#define __associated_lock_exists_and_is_preemptible(s)
({
bool ret;
/* No associated lock for these */
if (__same_type(*(s), seqcount_t) ||
__same_type(*(s), seqcount_irq_t)) {
ret = false;
} else if (__same_type(*(s), seqcount_spinlock_t) ||
__same_type(*(s), seqcount_raw_spinlock_t) ||
__same_type(*(s), seqcount_rwlock_t)) {
ret = false;
} else if (__same_type(*(s), seqcount_mutex_t) ||
__same_type(*(s), seqcount_ww_mutex_t)) {
ret = true;
} else
BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
ret;
})
#define __assert_seqcount_write_section_is_protected(s)
do {
/* Can't assert anything with plain seqcount_t */
if (__same_type(*(s), seqcount_t))
break;
if (__same_type(*(s), seqcount_spinlock_t))
lockdep_assert_held(((seqcount_spinlock_t *)(s))->lock);
else if (__same_type(*(s), seqcount_raw_spinlock_t))
lockdep_assert_held(((seqcount_raw_spinlock_t *)(s))->lock);
else if (__same_type(*(s), seqcount_rwlock_t))
lockdep_assert_held_write(((seqcount_rwlock_t *)(s))->lock);
else if (__same_type(*(s), seqcount_mutex_t))
lockdep_assert_held(((seqcount_mutex_t *)(s))->lock);
else if (__same_type(*(s), seqcount_ww_mutex_t))
lockdep_assert_held(&((seqcount_ww_mutex_t *)(s))->lock->base);
else if (__same_type(*(s), seqcount_irq_t))
lockdep_assert_irqs_disabled();
else
BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
} while (0)
and so on and so forth. With the final patch that enables PREEMPT_RT
support (not yet submitted upstream), this file gets even more fugly:
#define __rt_lock_unlock_associated_sleeping_lock(s)
do {
if (__same_type(*(s), seqcount_t) ||
__same_type(*(s), seqcount_raw_spinlock_t)) {
break;
}
if (__same_type(*(s), seqcount_spinlock_t)) {
spin_lock(((seqcount_spinlock_t *) s)->lock);
spin_unlock(((seqcount_spinlock_t *) s)->lock);
} else if (__same_type(*(s), seqcount_rwlock_t)) {
read_lock(((seqcount_rwlock_t *) s)->lock);
read_unlock(((seqcount_rwlock_t *) s)->lock);
} else if (__same_type(*(s), seqcount_mutex_t)) {
mutex_lock(((seqcount_mutex_t *) s)->lock);
mutex_unlock(((seqcount_mutex_t *) s)->lock);
} else if (__same_type(*(s), seqcount_ww_mutex_t)) {
ww_mutex_lock(((seqcount_ww_mutex_t *) s)->lock, NULL);
ww_mutex_unlock(((seqcount_ww_mutex_t *) s)->lock);
} else
BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
} while (0)
and even more macros not included here for brevity.
IMHO it makes sense to isolate these "not core logic" parts. Adding all
of this to plain seqlock.h makes it almost unreadable.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
On Tue, Jun 30, 2020 at 07:44:38AM +0200, Ahmed S. Darwish wrote: > +#include Why? why not include those lines directly here? Having it in a separate file actually makes it harder to read.
[PATCH v3 06/20] seqlock: Extend seqcount API with associated locks
A sequence counter write side critical section must be protected by some
form of locking to serialize writers. If the serialization primitive is
not disabling preemption implicitly, preemption has to be explicitly
disabled before entering the write side critical section.
There is no built-in debugging mechanism to verify that the lock used
for writer serialization is held and preemption is disabled. Some usage
sites like dma-buf have explicit lockdep checks for the writer-side
lock, but this covers only a small portion of the sequence counter usage
in the kernel.
Add new sequence counter types which allows to associate a lock to the
sequence counter at initialization time. The seqcount API functions are
extended to provide appropriate lockdep assertions depending on the
seqcount/lock type.
For sequence counters with associated locks that do not implicitly
disable preemption, preemption protection is enforced in the sequence
counter write side functions. This removes the need to explicitly add
preempt_disable/enable() around the write side critical sections: the
write_begin/end() functions for these new sequence counter types
automatically do this.
Introduce the following seqcount types with associated locks:
seqcount_spinlock_t
seqcount_raw_spinlock_t
seqcount_rwlock_t
seqcount_mutex_t
seqcount_ww_mutex_t
Extend the seqcount read and write functions to branch out to the
specific seqcount_LOCKTYPE_t implementation at compile-time. This avoids
kernel API explosion per each new seqcount_LOCKTYPE_t added. Add such
compile-time type detection logic into a new, internal, seqlock header.
Document the proper seqcount_LOCKTYPE_t usage, and rationale, at
Documentation/locking/seqlock.rst.
If lockdep is disabled, this lock association is compiled out and has
neither storage size nor runtime overhead.
Signed-off-by: Ahmed S. Darwish
---
Documentation/locking/seqlock.rst | 64 -
MAINTAINERS| 2 +-
include/linux/seqlock.h| 372 -
include/linux/seqlock_types_internal.h | 186 +
4 files changed, 558 insertions(+), 66 deletions(-)
create mode 100644 include/linux/seqlock_types_internal.h
diff --git a/Documentation/locking/seqlock.rst
b/Documentation/locking/seqlock.rst
index c9916efe038e..2d526dc95408 100644
--- a/Documentation/locking/seqlock.rst
+++ b/Documentation/locking/seqlock.rst
@@ -48,9 +48,11 @@ write side section. If the read section can be invoked from
hardirq or
softirq contexts, interrupts or bottom halves must also be respectively
disabled before entering the write section.
-If it's desired to automatically handle the sequence counter
-requirements of writer serialization and non-preemptibility, use a
-:ref:`sequential lock ` instead.
+If the write serialization mechanism is one of the common kernel locking
+primitives, use :ref:`sequence counters with associated locks
+` instead. If it's desired to automatically handle
+the sequence counter writer serialization and non-preemptibility
+requirements, use a :ref:`sequential lock `.
Initialization:
@@ -70,6 +72,7 @@ Initialization:
Write path:
+.. _seqcount_write_ops:
.. code-block:: c
/* Serialized context with disabled preemption */
@@ -82,6 +85,7 @@ Write path:
Read path:
+.. _seqcount_read_ops:
.. code-block:: c
do {
@@ -91,6 +95,60 @@ Read path:
} while (read_seqcount_retry(_seqcount, seq));
+.. _seqcount_locktype_t:
+
+Sequence counters with associated locks (:c:type:`seqcount_LOCKTYPE_t`)
+---
+
+As :ref:`earlier discussed `, seqcount write side critical
+sections must be serialized and non-preemptible. This variant of
+sequence counters associate the lock used for writer serialization at
+the seqcount initialization time. This enables lockdep to validate that
+the write side critical section is properly serialized.
+
+This lock association is a NOOP if lockdep is disabled and has neither
+storage nor runtime overhead. If lockdep is enabled, the lock pointer is
+stored in struct seqcount and lockdep's "lock is held" assertions are
+injected at the beginning of the write side critical section to validate
+that it is properly protected.
+
+For lock types which do not implicitly disable preemption, preemption
+protection is enforced in the write side function.
+
+The following seqcounts with associated locks are defined:
+
+ - :c:type:`seqcount_spinlock_t`
+ - :c:type:`seqcount_raw_spinlock_t`
+ - :c:type:`seqcount_rwlock_t`
+ - :c:type:`seqcount_mutex_t`
+ - :c:type:`seqcount_ww_mutex_t`
+
+The plain seqcount read and write APIs branch out to the specific
+seqcount_LOCKTYPE_t implementation at compile-time. This avoids kernel
+API explosion per each new seqcount LOCKTYPE.
+
+Initialization (replace "LOCKTYPE" with one of the supported locks):
+
+.. code-block:: c
+
+ /* dynamic */
+
