Messages in this thread Patch in this message | | | Date | Fri, 13 Oct 2023 10:46:25 +0200 | From | Ingo Molnar <> | Subject | [PATCH] locking/seqlock: Propagate 'const' pointers within read-only methods, remove forced type casts |
| |
* Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, 12 Oct 2023 at 11:21, Ingo Molnar <mingo@kernel.org> wrote: > > > > Okay, so dropping 'const' makes sense in terms of staying bug-compatible > > with the previous API and not build-breaking the world - but could we > > perhaps follow this up with fixups of the type misuse and then a removal > > of the forced type casts from these APIs? > > No. The use of 'const' here is *not* a bug. > > The thing is, 'const' doesn't mean what you seem to think it means. A > 'const' pointer in C in no way means that the target is constant - it > means that *THIS* use of the pointer will not write to the target!
Yeah, that is absolutely what I too think 'const' means - and sorry, I didn't expand: I meant something like the patch below, which clearly separates the 'const' from the non-const pointer uses within <linux/seqlock.h> and removes the two forced type casts I was unhappy about.
The 'bug' was that the __seqprop_*ptr() wrapper was used with both const and non-const pointers, and we forced a type and lost a tiny bit of potential const propagation. The code was fine and I should not have called it a 'bug', but I consider the dropping of 'const' a bad pattern, and I sometimes exaggerate problems to trick^W convince developers to continue working along a given path...
In hindsight my "break the world" expectation was overblown too: our const propagation through these methods was almost complete already, and the fixes stayed within <linux/seqlock.h>.
This patch could probably be split into two patches. Lightly tested only.
Does this work for you?
Thanks,
Ingo
===================> From: Ingo Molnar <mingo@kernel.org> Date: Fri, 13 Oct 2023 10:15:46 +0200 Subject: [PATCH] locking/seqlock: Propagate 'const' pointers within read-only methods, remove forced type casts
Currently __seqprop_ptr() is an inline function that must chose to either use 'const' or non-const seqcount related pointers - but this results in the undesirable loss of 'const' propagation, via a forced type cast.
The easiest solution would be to turn the pointer wrappers into macros that pass through whatever type is passed to them - but the clever maze of seqlock API instantiation macros relies on the GCC CPP '##' macro extension, which isn't recursive, so inline functions must be used here.
So create two wrapper variants instead: 'ptr' and 'const_ptr', and pick the right one for the codepaths that are const: read_seqcount_begin() and read_seqcount_retry().
This cleans up type handling and allows the removal of all type forcing.
No change in functionality.
Signed-off-by: Ingo Molnar <mingo@kernel.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Waiman Long <longman@redhat.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Paul E. McKenney <paulmck@kernel.org> --- include/linux/seqlock.h | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index 4b8dcd3a0d93..80f21d2ca2aa 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -200,9 +200,15 @@ typedef struct seqcount_##lockname { \ } seqcount_##lockname##_t; \ \ static __always_inline seqcount_t * \ -__seqprop_##lockname##_ptr(const seqcount_##lockname##_t *s) \ +__seqprop_##lockname##_ptr(seqcount_##lockname##_t *s) \ { \ - return (void *)&s->seqcount; /* drop const */ \ + return &s->seqcount; \ +} \ + \ +static __always_inline const seqcount_t * \ +__seqprop_##lockname##_const_ptr(const seqcount_##lockname##_t *s) \ +{ \ + return &s->seqcount; \ } \ \ static __always_inline unsigned \ @@ -247,9 +253,14 @@ __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s) \ * __seqprop() for seqcount_t */ -static inline seqcount_t *__seqprop_ptr(const seqcount_t *s) +static inline seqcount_t *__seqprop_ptr(seqcount_t *s) { - return (void *)s; /* drop const */ + return s; +} + +static inline const seqcount_t *__seqprop_const_ptr(const seqcount_t *s) +{ + return s; } static inline unsigned __seqprop_sequence(const seqcount_t *s) @@ -302,6 +313,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex) __seqprop_case((s), mutex, prop)) #define seqprop_ptr(s) __seqprop(s, ptr)(s) +#define seqprop_const_ptr(s) __seqprop(s, const_ptr)(s) #define seqprop_sequence(s) __seqprop(s, sequence)(s) #define seqprop_preemptible(s) __seqprop(s, preemptible)(s) #define seqprop_assert(s) __seqprop(s, assert)(s) @@ -353,7 +365,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex) */ #define read_seqcount_begin(s) \ ({ \ - seqcount_lockdep_reader_access(seqprop_ptr(s)); \ + seqcount_lockdep_reader_access(seqprop_const_ptr(s)); \ raw_read_seqcount_begin(s); \ }) @@ -419,7 +431,7 @@ SEQCOUNT_LOCKNAME(mutex, struct mutex, true, mutex) * Return: true if a read section retry is required, else false */ #define __read_seqcount_retry(s, start) \ - do___read_seqcount_retry(seqprop_ptr(s), start) + do___read_seqcount_retry(seqprop_const_ptr(s), start) static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start) { @@ -439,7 +451,7 @@ static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start) * Return: true if a read section retry is required, else false */ #define read_seqcount_retry(s, start) \ - do_read_seqcount_retry(seqprop_ptr(s), start) + do_read_seqcount_retry(seqprop_const_ptr(s), start) static inline int do_read_seqcount_retry(const seqcount_t *s, unsigned start) {
| |