* [PATCH -tip v1 0/3] seqlock: assorted cleanups
@ 2020-12-06 16:21 Ahmed S. Darwish
2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Ahmed S. Darwish @ 2020-12-06 16:21 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon
Cc: Linus Torvalds, Thomas Gleixner, Paul E. McKenney,
Steven Rostedt, Jonathan Corbet, Jason Gunthorpe, John Hubbard,
LKML, Sebastian A. Siewior, Ahmed S. Darwish
Hi,
When the seqcount_LOCKNAME_t group of data types were introduced, two
classes of seqlock.h sequence counter macros were added:
- An external public API which can either take a plain seqcount_t or
any of the seqcount_LOCKNAME_t variants.
- An internal API which takes only a plain seqcount_t.
To distinguish between the two groups, the "*_seqcount_t_*" pattern was
used for the latter. This confused a number of mm/ call-site developers,
and Linus also commented that this was not the standard practice for
marking kernel internal APIs. [1]
Distinguish the latter group of macros by prefixing a "do_".
A number of call-site developers also complained that the automatic
preemption disable/enable for the write side macros was not obvious, or
documented. [2] Linus also suggested adding few comments explaining that
behavior. [3] Fix it by completing the seqcount write side kernel-doc
annotations.
Finally, fix a minor naming inconsistency w.r.t. seqlock.h
vs. Documentation/locking/seqlock.rst.
This series does not change the output "allyesconfig" kernel binary:
text data bss ... filename
247616963 289662125 81498728 ... ../build-x86-64/vmlinux.old
247616963 289662125 81498728 ... ../build-x86-64/vmlinux
145054028 78270273 18435468 ... ../build-arm/vmlinux.old
145054028 78270273 18435468 ... ../build-arm/vmlinux
Note: based over -tip locking/core, instead of latest -rc, due to -tip
ab440b2c604b ("seqlock: Rename __seqprop() users").
References:
[1] https://lkml.kernel.org/r/CAHk-=wgB8nyOQufpn0o6a5BpJCJPnXvH+kRxApujhsgG+7qAwQ@mail.gmail.com
[2] https://lkml.kernel.org/r/20201030235121.GQ2620339@nvidia.com
[3] https://lkml.kernel.org/r/CAHk-=wikhGExmprXgaW+MVXG1zsGpztBbVwOb23vetk41EtTBQ@mail.gmail.com
8<--------------
Ahmed S. Darwish (3):
Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g
seqlock: Prefix internal seqcount_t-only macros with a "do_"
seqlock: kernel-doc: Specify when preemption is automatically altered
Documentation/locking/seqlock.rst | 21 ++++----
include/linux/seqlock.h | 83 ++++++++++++++++---------------
2 files changed, 54 insertions(+), 50 deletions(-)
base-commit: 97d62caa32d6d79dadae3f8d19af5c92ea9a589a
--
2.29.2
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g 2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish @ 2020-12-06 16:21 ` Ahmed S. Darwish 2020-12-09 18:38 ` [tip: locking/core] " tip-bot2 for Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" Ahmed S. Darwish ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Ahmed S. Darwish @ 2020-12-06 16:21 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Will Deacon Cc: Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, Jason Gunthorpe, John Hubbard, LKML, Sebastian A. Siewior, Ahmed S. Darwish Sequence counters with an associated write serialization lock are called seqcount_LOCKNAME_t. Fix the documentation accordingly. While at it, remove a paragraph that inappropriately discussed a seqlock.h implementation detail. Fixes: 6dd699b13d53 ("seqlock: seqcount_LOCKNAME_t: Standardize naming convention") Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> Cc: stable@vger.kernel.org Cc: Jonathan Corbet <corbet@lwn.net> --- Documentation/locking/seqlock.rst | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/locking/seqlock.rst b/Documentation/locking/seqlock.rst index a334b584f2b3..64405e5da63e 100644 --- a/Documentation/locking/seqlock.rst +++ b/Documentation/locking/seqlock.rst @@ -89,7 +89,7 @@ Read path:: .. _seqcount_locktype_t: -Sequence counters with associated locks (``seqcount_LOCKTYPE_t``) +Sequence counters with associated locks (``seqcount_LOCKNAME_t``) ----------------------------------------------------------------- As discussed at :ref:`seqcount_t`, sequence count write side critical @@ -115,27 +115,26 @@ The following sequence counters with associated locks are defined: - ``seqcount_mutex_t`` - ``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. +The sequence counter read and write APIs can take either a plain +seqcount_t or any of the seqcount_LOCKNAME_t variants above. -Initialization (replace "LOCKTYPE" with one of the supported locks):: +Initialization (replace "LOCKNAME" with one of the supported locks):: /* dynamic */ - seqcount_LOCKTYPE_t foo_seqcount; - seqcount_LOCKTYPE_init(&foo_seqcount, &lock); + seqcount_LOCKNAME_t foo_seqcount; + seqcount_LOCKNAME_init(&foo_seqcount, &lock); /* static */ - static seqcount_LOCKTYPE_t foo_seqcount = - SEQCNT_LOCKTYPE_ZERO(foo_seqcount, &lock); + static seqcount_LOCKNAME_t foo_seqcount = + SEQCNT_LOCKNAME_ZERO(foo_seqcount, &lock); /* C99 struct init */ struct { - .seq = SEQCNT_LOCKTYPE_ZERO(foo.seq, &lock), + .seq = SEQCNT_LOCKNAME_ZERO(foo.seq, &lock), } foo; Write path: same as in :ref:`seqcount_t`, while running from a context -with the associated LOCKTYPE lock acquired. +with the associated write serialization lock acquired. Read path: same as in :ref:`seqcount_t`. -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [tip: locking/core] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g 2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish @ 2020-12-09 18:38 ` tip-bot2 for Ahmed S. Darwish 0 siblings, 0 replies; 10+ messages in thread From: tip-bot2 for Ahmed S. Darwish @ 2020-12-09 18:38 UTC (permalink / raw) To: linux-tip-commits Cc: Ahmed S. Darwish, Peter Zijlstra (Intel), stable, x86, linux-kernel The following commit has been merged into the locking/core branch of tip: Commit-ID: cf48647243cc28d15280600292db5777592606c5 Gitweb: https://git.kernel.org/tip/cf48647243cc28d15280600292db5777592606c5 Author: Ahmed S. Darwish <a.darwish@linutronix.de> AuthorDate: Sun, 06 Dec 2020 17:21:41 +01:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Wed, 09 Dec 2020 17:08:49 +01:00 Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Sequence counters with an associated write serialization lock are called seqcount_LOCKNAME_t. Fix the documentation accordingly. While at it, remove a paragraph that inappropriately discussed a seqlock.h implementation detail. Fixes: 6dd699b13d53 ("seqlock: seqcount_LOCKNAME_t: Standardize naming convention") Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: stable@vger.kernel.org Link: https://lkml.kernel.org/r/20201206162143.14387-2-a.darwish@linutronix.de --- Documentation/locking/seqlock.rst | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/locking/seqlock.rst b/Documentation/locking/seqlock.rst index a334b58..64405e5 100644 --- a/Documentation/locking/seqlock.rst +++ b/Documentation/locking/seqlock.rst @@ -89,7 +89,7 @@ Read path:: .. _seqcount_locktype_t: -Sequence counters with associated locks (``seqcount_LOCKTYPE_t``) +Sequence counters with associated locks (``seqcount_LOCKNAME_t``) ----------------------------------------------------------------- As discussed at :ref:`seqcount_t`, sequence count write side critical @@ -115,27 +115,26 @@ The following sequence counters with associated locks are defined: - ``seqcount_mutex_t`` - ``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. +The sequence counter read and write APIs can take either a plain +seqcount_t or any of the seqcount_LOCKNAME_t variants above. -Initialization (replace "LOCKTYPE" with one of the supported locks):: +Initialization (replace "LOCKNAME" with one of the supported locks):: /* dynamic */ - seqcount_LOCKTYPE_t foo_seqcount; - seqcount_LOCKTYPE_init(&foo_seqcount, &lock); + seqcount_LOCKNAME_t foo_seqcount; + seqcount_LOCKNAME_init(&foo_seqcount, &lock); /* static */ - static seqcount_LOCKTYPE_t foo_seqcount = - SEQCNT_LOCKTYPE_ZERO(foo_seqcount, &lock); + static seqcount_LOCKNAME_t foo_seqcount = + SEQCNT_LOCKNAME_ZERO(foo_seqcount, &lock); /* C99 struct init */ struct { - .seq = SEQCNT_LOCKTYPE_ZERO(foo.seq, &lock), + .seq = SEQCNT_LOCKNAME_ZERO(foo.seq, &lock), } foo; Write path: same as in :ref:`seqcount_t`, while running from a context -with the associated LOCKTYPE lock acquired. +with the associated write serialization lock acquired. Read path: same as in :ref:`seqcount_t`. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" 2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish @ 2020-12-06 16:21 ` Ahmed S. Darwish 2020-12-07 20:39 ` Jason Gunthorpe 2020-12-06 16:21 ` [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered Ahmed S. Darwish 2020-12-07 14:07 ` [PATCH -tip v1 0/3] seqlock: assorted cleanups Peter Zijlstra 3 siblings, 1 reply; 10+ messages in thread From: Ahmed S. Darwish @ 2020-12-06 16:21 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Will Deacon Cc: Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, Jason Gunthorpe, John Hubbard, LKML, Sebastian A. Siewior, Ahmed S. Darwish When the seqcount_LOCKNAME_t group of data types were introduced, two classes of seqlock.h sequence counter macros were added: - An external public API which can either take a plain seqcount_t or any of the seqcount_LOCKNAME_t variants. - An internal API which takes only a plain seqcount_t. To distinguish between the two groups, the "*_seqcount_t_*" pattern was used for the latter. This confused a number of mm/ call-site developers, and Linus also commented that it was not a standard practice for marking seqlock.h internal APIs. Distinguish the latter group of macros by prefixing a "do_". Link: https://lkml.kernel.org/r/CAHk-=wgB8nyOQufpn0o6a5BpJCJPnXvH+kRxApujhsgG+7qAwQ@mail.gmail.com Link: https://lkml.kernel.org/r/CAHk-=wikhGExmprXgaW+MVXG1zsGpztBbVwOb23vetk41EtTBQ@mail.gmail.com Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> --- include/linux/seqlock.h | 66 ++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index d89134c74fba..235cbc65fd71 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -425,9 +425,9 @@ SEQCOUNT_LOCKNAME(ww_mutex, struct ww_mutex, true, &s->lock->base, ww_mu * Return: true if a read section retry is required, else false */ #define __read_seqcount_retry(s, start) \ - __read_seqcount_t_retry(seqprop_ptr(s), start) + do___read_seqcount_retry(seqprop_ptr(s), start) -static inline int __read_seqcount_t_retry(const seqcount_t *s, unsigned start) +static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start) { kcsan_atomic_next(0); return unlikely(READ_ONCE(s->sequence) != start); @@ -445,12 +445,12 @@ static inline int __read_seqcount_t_retry(const seqcount_t *s, unsigned start) * Return: true if a read section retry is required, else false */ #define read_seqcount_retry(s, start) \ - read_seqcount_t_retry(seqprop_ptr(s), start) + do_read_seqcount_retry(seqprop_ptr(s), start) -static inline int read_seqcount_t_retry(const seqcount_t *s, unsigned start) +static inline int do_read_seqcount_retry(const seqcount_t *s, unsigned start) { smp_rmb(); - return __read_seqcount_t_retry(s, start); + return do___read_seqcount_retry(s, start); } /** @@ -462,10 +462,10 @@ do { \ if (seqprop_preemptible(s)) \ preempt_disable(); \ \ - raw_write_seqcount_t_begin(seqprop_ptr(s)); \ + do_raw_write_seqcount_begin(seqprop_ptr(s)); \ } while (0) -static inline void raw_write_seqcount_t_begin(seqcount_t *s) +static inline void do_raw_write_seqcount_begin(seqcount_t *s) { kcsan_nestable_atomic_begin(); s->sequence++; @@ -478,13 +478,13 @@ static inline void raw_write_seqcount_t_begin(seqcount_t *s) */ #define raw_write_seqcount_end(s) \ do { \ - raw_write_seqcount_t_end(seqprop_ptr(s)); \ + do_raw_write_seqcount_end(seqprop_ptr(s)); \ \ if (seqprop_preemptible(s)) \ preempt_enable(); \ } while (0) -static inline void raw_write_seqcount_t_end(seqcount_t *s) +static inline void do_raw_write_seqcount_end(seqcount_t *s) { smp_wmb(); s->sequence++; @@ -506,12 +506,12 @@ do { \ if (seqprop_preemptible(s)) \ preempt_disable(); \ \ - write_seqcount_t_begin_nested(seqprop_ptr(s), subclass); \ + do_write_seqcount_begin_nested(seqprop_ptr(s), subclass); \ } while (0) -static inline void write_seqcount_t_begin_nested(seqcount_t *s, int subclass) +static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass) { - raw_write_seqcount_t_begin(s); + do_raw_write_seqcount_begin(s); seqcount_acquire(&s->dep_map, subclass, 0, _RET_IP_); } @@ -533,12 +533,12 @@ do { \ if (seqprop_preemptible(s)) \ preempt_disable(); \ \ - write_seqcount_t_begin(seqprop_ptr(s)); \ + do_write_seqcount_begin(seqprop_ptr(s)); \ } while (0) -static inline void write_seqcount_t_begin(seqcount_t *s) +static inline void do_write_seqcount_begin(seqcount_t *s) { - write_seqcount_t_begin_nested(s, 0); + do_write_seqcount_begin_nested(s, 0); } /** @@ -549,16 +549,16 @@ static inline void write_seqcount_t_begin(seqcount_t *s) */ #define write_seqcount_end(s) \ do { \ - write_seqcount_t_end(seqprop_ptr(s)); \ + do_write_seqcount_end(seqprop_ptr(s)); \ \ if (seqprop_preemptible(s)) \ preempt_enable(); \ } while (0) -static inline void write_seqcount_t_end(seqcount_t *s) +static inline void do_write_seqcount_end(seqcount_t *s) { seqcount_release(&s->dep_map, _RET_IP_); - raw_write_seqcount_t_end(s); + do_raw_write_seqcount_end(s); } /** @@ -603,9 +603,9 @@ static inline void write_seqcount_t_end(seqcount_t *s) * } */ #define raw_write_seqcount_barrier(s) \ - raw_write_seqcount_t_barrier(seqprop_ptr(s)) + do_raw_write_seqcount_barrier(seqprop_ptr(s)) -static inline void raw_write_seqcount_t_barrier(seqcount_t *s) +static inline void do_raw_write_seqcount_barrier(seqcount_t *s) { kcsan_nestable_atomic_begin(); s->sequence++; @@ -623,9 +623,9 @@ static inline void raw_write_seqcount_t_barrier(seqcount_t *s) * will complete successfully and see data older than this. */ #define write_seqcount_invalidate(s) \ - write_seqcount_t_invalidate(seqprop_ptr(s)) + do_write_seqcount_invalidate(seqprop_ptr(s)) -static inline void write_seqcount_t_invalidate(seqcount_t *s) +static inline void do_write_seqcount_invalidate(seqcount_t *s) { smp_wmb(); kcsan_nestable_atomic_begin(); @@ -865,9 +865,9 @@ static inline unsigned read_seqretry(const seqlock_t *sl, unsigned start) } /* - * For all seqlock_t write side functions, use write_seqcount_*t*_begin() - * instead of the generic write_seqcount_begin(). This way, no redundant - * lockdep_assert_held() checks are added. + * For all seqlock_t write side functions, use the the internal + * do_write_seqcount_begin() instead of generic write_seqcount_begin(). + * This way, no redundant lockdep_assert_held() checks are added. */ /** @@ -886,7 +886,7 @@ static inline unsigned read_seqretry(const seqlock_t *sl, unsigned start) static inline void write_seqlock(seqlock_t *sl) { spin_lock(&sl->lock); - write_seqcount_t_begin(&sl->seqcount.seqcount); + do_write_seqcount_begin(&sl->seqcount.seqcount); } /** @@ -898,7 +898,7 @@ static inline void write_seqlock(seqlock_t *sl) */ static inline void write_sequnlock(seqlock_t *sl) { - write_seqcount_t_end(&sl->seqcount.seqcount); + do_write_seqcount_end(&sl->seqcount.seqcount); spin_unlock(&sl->lock); } @@ -912,7 +912,7 @@ static inline void write_sequnlock(seqlock_t *sl) static inline void write_seqlock_bh(seqlock_t *sl) { spin_lock_bh(&sl->lock); - write_seqcount_t_begin(&sl->seqcount.seqcount); + do_write_seqcount_begin(&sl->seqcount.seqcount); } /** @@ -925,7 +925,7 @@ static inline void write_seqlock_bh(seqlock_t *sl) */ static inline void write_sequnlock_bh(seqlock_t *sl) { - write_seqcount_t_end(&sl->seqcount.seqcount); + do_write_seqcount_end(&sl->seqcount.seqcount); spin_unlock_bh(&sl->lock); } @@ -939,7 +939,7 @@ static inline void write_sequnlock_bh(seqlock_t *sl) static inline void write_seqlock_irq(seqlock_t *sl) { spin_lock_irq(&sl->lock); - write_seqcount_t_begin(&sl->seqcount.seqcount); + do_write_seqcount_begin(&sl->seqcount.seqcount); } /** @@ -951,7 +951,7 @@ static inline void write_seqlock_irq(seqlock_t *sl) */ static inline void write_sequnlock_irq(seqlock_t *sl) { - write_seqcount_t_end(&sl->seqcount.seqcount); + do_write_seqcount_end(&sl->seqcount.seqcount); spin_unlock_irq(&sl->lock); } @@ -960,7 +960,7 @@ static inline unsigned long __write_seqlock_irqsave(seqlock_t *sl) unsigned long flags; spin_lock_irqsave(&sl->lock, flags); - write_seqcount_t_begin(&sl->seqcount.seqcount); + do_write_seqcount_begin(&sl->seqcount.seqcount); return flags; } @@ -989,7 +989,7 @@ static inline unsigned long __write_seqlock_irqsave(seqlock_t *sl) static inline void write_sequnlock_irqrestore(seqlock_t *sl, unsigned long flags) { - write_seqcount_t_end(&sl->seqcount.seqcount); + do_write_seqcount_end(&sl->seqcount.seqcount); spin_unlock_irqrestore(&sl->lock, flags); } -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" 2020-12-06 16:21 ` [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" Ahmed S. Darwish @ 2020-12-07 20:39 ` Jason Gunthorpe 0 siblings, 0 replies; 10+ messages in thread From: Jason Gunthorpe @ 2020-12-07 20:39 UTC (permalink / raw) To: Ahmed S. Darwish Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, John Hubbard, LKML, Sebastian A. Siewior On Sun, Dec 06, 2020 at 05:21:42PM +0100, Ahmed S. Darwish wrote: > When the seqcount_LOCKNAME_t group of data types were introduced, two > classes of seqlock.h sequence counter macros were added: > > - An external public API which can either take a plain seqcount_t or > any of the seqcount_LOCKNAME_t variants. > > - An internal API which takes only a plain seqcount_t. > > To distinguish between the two groups, the "*_seqcount_t_*" pattern was > used for the latter. This confused a number of mm/ call-site developers, > and Linus also commented that it was not a standard practice for marking > seqlock.h internal APIs. > > Distinguish the latter group of macros by prefixing a "do_". > > Link: https://lkml.kernel.org/r/CAHk-=wgB8nyOQufpn0o6a5BpJCJPnXvH+kRxApujhsgG+7qAwQ@mail.gmail.com > Link: https://lkml.kernel.org/r/CAHk-=wikhGExmprXgaW+MVXG1zsGpztBbVwOb23vetk41EtTBQ@mail.gmail.com > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > --- > include/linux/seqlock.h | 66 ++++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 33 deletions(-) It is clearer, thank you Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered 2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" Ahmed S. Darwish @ 2020-12-06 16:21 ` Ahmed S. Darwish 2020-12-07 20:43 ` Jason Gunthorpe 2020-12-07 14:07 ` [PATCH -tip v1 0/3] seqlock: assorted cleanups Peter Zijlstra 3 siblings, 1 reply; 10+ messages in thread From: Ahmed S. Darwish @ 2020-12-06 16:21 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Will Deacon Cc: Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, Jason Gunthorpe, John Hubbard, LKML, Sebastian A. Siewior, Ahmed S. Darwish The kernel-doc annotations for sequence counters write side functions are incomplete: they do not specify when preemption is automatically disabled and re-enabled. This has confused a number of call-site developers. Fix it. Link: https://lkml.kernel.org/r/20201030235121.GQ2620339@nvidia.com Link: https://lkml.kernel.org/r/CAHk-=wikhGExmprXgaW+MVXG1zsGpztBbVwOb23vetk41EtTBQ@mail.gmail.com Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> Cc: Jonathan Corbet <corbet@lwn.net> --- include/linux/seqlock.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index 235cbc65fd71..2f7bb92b4c9e 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -456,6 +456,8 @@ static inline int do_read_seqcount_retry(const seqcount_t *s, unsigned start) /** * raw_write_seqcount_begin() - start a seqcount_t write section w/o lockdep * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants + * + * Context: check write_seqcount_begin() */ #define raw_write_seqcount_begin(s) \ do { \ @@ -475,6 +477,8 @@ static inline void do_raw_write_seqcount_begin(seqcount_t *s) /** * raw_write_seqcount_end() - end a seqcount_t write section w/o lockdep * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants + * + * Context: check write_seqcount_end() */ #define raw_write_seqcount_end(s) \ do { \ @@ -498,6 +502,7 @@ static inline void do_raw_write_seqcount_end(seqcount_t *s) * @subclass: lockdep nesting level * * See Documentation/locking/lockdep-design.rst + * Context: check write_seqcount_begin() */ #define write_seqcount_begin_nested(s, subclass) \ do { \ @@ -519,11 +524,10 @@ static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass) * write_seqcount_begin() - start a seqcount_t write side critical section * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants * - * write_seqcount_begin opens a write side critical section of the given - * seqcount_t. - * - * Context: seqcount_t write side critical sections must be serialized and - * non-preemptible. If readers can be invoked from hardirq or softirq + * Context: sequence counter write side sections must be serialized and + * non-preemptible. Preemption will be automatically disabled if and + * only if the seqcount write serialization lock is associated, and + * preemptible. If readers can be invoked from hardirq or softirq * context, interrupts or bottom halves must be respectively disabled. */ #define write_seqcount_begin(s) \ @@ -545,7 +549,8 @@ static inline void do_write_seqcount_begin(seqcount_t *s) * write_seqcount_end() - end a seqcount_t write side critical section * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants * - * The write section must've been opened with write_seqcount_begin(). + * Context: Preemption will be automatically re-enabled if and only if + * the seqcount write serialization lock is associated, and preemptible. */ #define write_seqcount_end(s) \ do { \ -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered 2020-12-06 16:21 ` [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered Ahmed S. Darwish @ 2020-12-07 20:43 ` Jason Gunthorpe 2020-12-08 14:31 ` Ahmed S. Darwish 0 siblings, 1 reply; 10+ messages in thread From: Jason Gunthorpe @ 2020-12-07 20:43 UTC (permalink / raw) To: Ahmed S. Darwish Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, John Hubbard, LKML, Sebastian A. Siewior On Sun, Dec 06, 2020 at 05:21:43PM +0100, Ahmed S. Darwish wrote: > @@ -519,11 +524,10 @@ static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass) > * write_seqcount_begin() - start a seqcount_t write side critical section > * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants > * > - * write_seqcount_begin opens a write side critical section of the given > - * seqcount_t. > - * > - * Context: seqcount_t write side critical sections must be serialized and > - * non-preemptible. If readers can be invoked from hardirq or softirq > + * Context: sequence counter write side sections must be serialized and > + * non-preemptible. Preemption will be automatically disabled if and > + * only if the seqcount write serialization lock is associated, and > + * preemptible. If readers can be invoked from hardirq or softirq > * context, interrupts or bottom halves must be respectively disabled. > */ The thing that was confusing is if it was appropriate to use a seqcount in case where write side preemption was not disabled - which is safe only if the read side doesn't spin. We seem to have only two places that do this, but since this comment reads like it is absolutely forbidden, it is still confusing.. To make it clear a read side API to work with the seqlock for non-premption cases would be nice, then the language could be 'must be non-premeptible if using read_seqcount_retry(), but if using NEWTHING then it is not required' Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered 2020-12-07 20:43 ` Jason Gunthorpe @ 2020-12-08 14:31 ` Ahmed S. Darwish 2020-12-08 15:46 ` Jason Gunthorpe 0 siblings, 1 reply; 10+ messages in thread From: Ahmed S. Darwish @ 2020-12-08 14:31 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, John Hubbard, LKML, Sebastian A. Siewior Hi Jason, On Mon, Dec 07, 2020 at 04:43:16PM -0400, Jason Gunthorpe wrote: ... > > The thing that was confusing is if it was appropriate to use a > seqcount in case where write side preemption was not disabled - which > is safe only if the read side doesn't spin. > No, that's not correct. What was confusing was that *everyone* in that mm/ thread, including yourself, thought that write_seqcount_begin() will automatically disable preemption for plain seqcount_t: https://lkml.kernel.org/r/20201030235121.GQ2620339@nvidia.com Quoting Peter Xu: "My understanding is that we used raw_write_seqcount_t_begin() because we're with spin lock so assuming we disabled preemption already." Quoting you: "write_seqcount_begin - Enforces preemption off". And that's why this patch explicitly adds to the kernel-doc: "Preemption will be automatically disabled if and only if the seqcount write serialization lock is associated, and preemptible." Honestly, I should've added that statement anyway in my original "sequence counters with associated locks" submission. I take the blame for the resulting confusion, and I'm sorry... ~~~~~~~~~~~~~~~~~~~~ Now regarding the other point, where the seqcount_t write side does not need to disable preemption if the reader does not spin. We've already discussed that (at length) last time. There comes a point where you decide what level of documentation to add, and what level to skip. Because in the end, you don't want to confuse "Joe, the generic driver developer" with too much details that's not relevant to their task at hand. You want to keep the general case simple, but the special case do-able. And you also want to encourage people to use the standard write side critical section entry and exit functions, as much as possible. Because, oh, look at that: [PATCH v2 0/6] seqlock: seqcount_t call sites bugfixes https://lkml.kernel.org/r/20200603144949.1122421-1-a.darwish@linutronix.de Sequence counters are already hard to get right. And driver developers get things wrong, a lot -- you don't want to confuse them even further. For developers who're advanced enough to know the difference, they don't need the kernel-doc anyway. And that's why I've kindly asked to add the following to your mm/ patch (which you did, thanks): /* * Disabling preemption is not needed for the write side, as * the read side does not spin, but goes to mmap_lock. * ... */ And IMHO, that should be enough. Developers of such special cases are already assumed to know what they're doing. Thanks a lot, -- Ahmed S. Darwish Linutronix GmbH ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered 2020-12-08 14:31 ` Ahmed S. Darwish @ 2020-12-08 15:46 ` Jason Gunthorpe 0 siblings, 0 replies; 10+ messages in thread From: Jason Gunthorpe @ 2020-12-08 15:46 UTC (permalink / raw) To: Ahmed S. Darwish Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, John Hubbard, LKML, Sebastian A. Siewior On Tue, Dec 08, 2020 at 03:31:39PM +0100, Ahmed S. Darwish wrote: > Hi Jason, > > On Mon, Dec 07, 2020 at 04:43:16PM -0400, Jason Gunthorpe wrote: > ... > > > > The thing that was confusing is if it was appropriate to use a > > seqcount in case where write side preemption was not disabled - which > > is safe only if the read side doesn't spin. > > > > No, that's not correct. Well, that is where I started from.. seqcount in normal pre-emption disabled cases was well understood, I needed a no-pre-emption disable case. > For developers who're advanced enough to know the difference, they don't > need the kernel-doc anyway. And that's why I've kindly asked to add the > following to your mm/ patch (which you did, thanks): That is probably over stating things quite a lot. If there are valid locking patterns then I think we should document them, otherwise people simply do something crazy and get it wrong. It was not entirely easy to figure out why preemption disable is necessary here, though in hindsight it is obvious.. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -tip v1 0/3] seqlock: assorted cleanups 2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish ` (2 preceding siblings ...) 2020-12-06 16:21 ` [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered Ahmed S. Darwish @ 2020-12-07 14:07 ` Peter Zijlstra 3 siblings, 0 replies; 10+ messages in thread From: Peter Zijlstra @ 2020-12-07 14:07 UTC (permalink / raw) To: Ahmed S. Darwish Cc: Ingo Molnar, Will Deacon, Linus Torvalds, Thomas Gleixner, Paul E. McKenney, Steven Rostedt, Jonathan Corbet, Jason Gunthorpe, John Hubbard, LKML, Sebastian A. Siewior On Sun, Dec 06, 2020 at 05:21:40PM +0100, Ahmed S. Darwish wrote: > Ahmed S. Darwish (3): > Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g > seqlock: Prefix internal seqcount_t-only macros with a "do_" > seqlock: kernel-doc: Specify when preemption is automatically altered Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-09 18:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-06 16:21 [PATCH -tip v1 0/3] seqlock: assorted cleanups Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 1/3] Documentation: seqlock: s/LOCKTYPE/LOCKNAME/g Ahmed S. Darwish 2020-12-09 18:38 ` [tip: locking/core] " tip-bot2 for Ahmed S. Darwish 2020-12-06 16:21 ` [PATCH -tip v1 2/3] seqlock: Prefix internal seqcount_t-only macros with a "do_" Ahmed S. Darwish 2020-12-07 20:39 ` Jason Gunthorpe 2020-12-06 16:21 ` [PATCH -tip v1 3/3] seqlock: kernel-doc: Specify when preemption is automatically altered Ahmed S. Darwish 2020-12-07 20:43 ` Jason Gunthorpe 2020-12-08 14:31 ` Ahmed S. Darwish 2020-12-08 15:46 ` Jason Gunthorpe 2020-12-07 14:07 ` [PATCH -tip v1 0/3] seqlock: assorted cleanups Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).