[PATCH 0/5] turn signal_struct.stats_lock into seqcount_rwlock_t

[PATCH 0/5] turn signal_struct.stats_lock into seqcount_rwlock_t

[Oleg Nesterov]

Hello,

RFC, not for inclusion yet. Please review, at least the intent.

In particular, can you look at the changelog from the last patch?
Am I right that currently thread_group_cputime() is not RT friendly?


During the (ongoing) s/while_each_thread/for_each_thread/ conversion
I noticed that some of these users can use signal->stats_lock instead
of lock_task_sighand(). And if we change them, we can try to avoid
stats_lock under siglock in wait_task_zombie() at least.

However, signal->stats_lock is seqlock_t but I think seqcount_rwlock_t
make more sense. So let me try to turn it into seqcount_rwlock_t first.

OTOH... I am not sure I understand the value of signal->stats_lock.
I mean, do we have any numbers which prove that seqlock_t is really
"better" than the plain rwlock_t ?

So far only compile tested, and I need to re-read these changes with
a clear head. In any case, it is not that I myself like this series
very much, and most probably I did something wrong...

Oleg.
---
 include/linux/sched/signal.h |   4 +-
 include/linux/seqlock.h      | 105 +++++++++++++++++++++++++++++++++++--------
 kernel/exit.c                |  12 +++--
 kernel/fork.c                |   3 +-
 kernel/sched/cputime.c       |  10 +++--
 5 files changed, 106 insertions(+), 28 deletions(-)

[PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Oleg Nesterov]

1. Kill the "lockmember" argument. It is always s->lock plus
   __seqprop_##lockname##_sequence() already uses s->lock and
   ignores "lockmember".

2. Kill the "lock_acquire" argument. __seqprop_##lockname##_sequence()
   can use the same "lockbase" prefix for _lock and _unlock.

Apart from line numbers, gcc -E outputs the same code.

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

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 987a59d977c5..ac6631bd5706 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -191,11 +191,9 @@ static inline void seqcount_lockdep_reader_access(const seqcount_t *s)
  * @lockname:		"LOCKNAME" part of seqcount_LOCKNAME_t
  * @locktype:		LOCKNAME canonical C data type
  * @preemptible:	preemptibility of above locktype
- * @lockmember:		argument for lockdep_assert_held()
- * @lockbase:		associated lock release function (prefix only)
- * @lock_acquire:	associated lock acquisition function (full call)
+ * @lockbase:		prefix for associated lock/unlock
  */
-#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockmember, lockbase, lock_acquire) \
+#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockbase)	\
 typedef struct seqcount_##lockname {					\
 	seqcount_t		seqcount;				\
 	__SEQ_LOCK(locktype	*lock);					\
@@ -216,7 +214,7 @@ __seqprop_##lockname##_sequence(const seqcount_##lockname##_t *s)	\
 		return seq;						\
 									\
 	if (preemptible && unlikely(seq & 1)) {				\
-		__SEQ_LOCK(lock_acquire);				\
+		__SEQ_LOCK(lockbase##_lock(s->lock));			\
 		__SEQ_LOCK(lockbase##_unlock(s->lock));			\
 									\
 		/*							\
@@ -242,7 +240,7 @@ __seqprop_##lockname##_preemptible(const seqcount_##lockname##_t *s)	\
 static __always_inline void						\
 __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s)		\
 {									\
-	__SEQ_LOCK(lockdep_assert_held(lockmember));			\
+	__SEQ_LOCK(lockdep_assert_held(s->lock));			\
 }
 
 /*
@@ -271,10 +269,10 @@ static inline void __seqprop_assert(const seqcount_t *s)
 
 #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
 
-SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    s->lock,        raw_spin, raw_spin_lock(s->lock))
-SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, s->lock,        spin,     spin_lock(s->lock))
-SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, s->lock,        read,     read_lock(s->lock))
-SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     s->lock,        mutex,    mutex_lock(s->lock))
+SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    raw_spin)
+SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, spin)
+SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, read)
+SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
 
 /*
  * SEQCNT_LOCKNAME_ZERO - static initializer for seqcount_LOCKNAME_t
-- 
2.25.1.362.g51ebf55

Re: [PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Alexey Gladkov]

On Wed, Sep 13, 2023 at 05:49:53PM +0200, Oleg Nesterov wrote:
> 1. Kill the "lockmember" argument. It is always s->lock plus
>    __seqprop_##lockname##_sequence() already uses s->lock and
>    ignores "lockmember".

typedef struct seqcount_##lockname {					\
	seqcount_t		seqcount;				\
	__SEQ_LOCK(locktype	*lock);					\
                                 ^^^^

and also lockmember was not used in the definition! :)
Thanks for bugfix!

Reviewed-by: Alexey Gladkov <legion@kernel.org>

> 2. Kill the "lock_acquire" argument. __seqprop_##lockname##_sequence()
>    can use the same "lockbase" prefix for _lock and _unlock.
> 
> Apart from line numbers, gcc -E outputs the same code.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/seqlock.h | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index 987a59d977c5..ac6631bd5706 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -191,11 +191,9 @@ static inline void seqcount_lockdep_reader_access(const seqcount_t *s)
>   * @lockname:		"LOCKNAME" part of seqcount_LOCKNAME_t
>   * @locktype:		LOCKNAME canonical C data type
>   * @preemptible:	preemptibility of above locktype
> - * @lockmember:		argument for lockdep_assert_held()
> - * @lockbase:		associated lock release function (prefix only)
> - * @lock_acquire:	associated lock acquisition function (full call)
> + * @lockbase:		prefix for associated lock/unlock
>   */
> -#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockmember, lockbase, lock_acquire) \
> +#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockbase)	\
>  typedef struct seqcount_##lockname {					\
>  	seqcount_t		seqcount;				\
>  	__SEQ_LOCK(locktype	*lock);					\
> @@ -216,7 +214,7 @@ __seqprop_##lockname##_sequence(const seqcount_##lockname##_t *s)	\
>  		return seq;						\
>  									\
>  	if (preemptible && unlikely(seq & 1)) {				\
> -		__SEQ_LOCK(lock_acquire);				\
> +		__SEQ_LOCK(lockbase##_lock(s->lock));			\
>  		__SEQ_LOCK(lockbase##_unlock(s->lock));			\
>  									\
>  		/*							\
> @@ -242,7 +240,7 @@ __seqprop_##lockname##_preemptible(const seqcount_##lockname##_t *s)	\
>  static __always_inline void						\
>  __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s)		\
>  {									\
> -	__SEQ_LOCK(lockdep_assert_held(lockmember));			\
> +	__SEQ_LOCK(lockdep_assert_held(s->lock));			\
>  }
>  
>  /*
> @@ -271,10 +269,10 @@ static inline void __seqprop_assert(const seqcount_t *s)
>  
>  #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
>  
> -SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    s->lock,        raw_spin, raw_spin_lock(s->lock))
> -SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, s->lock,        spin,     spin_lock(s->lock))
> -SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, s->lock,        read,     read_lock(s->lock))
> -SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     s->lock,        mutex,    mutex_lock(s->lock))
> +SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    raw_spin)
> +SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, spin)
> +SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, read)
> +SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
>  
>  /*
>   * SEQCNT_LOCKNAME_ZERO - static initializer for seqcount_LOCKNAME_t
> -- 
> 2.25.1.362.g51ebf55
> 

-- 
Rgrds, legion

Re: [PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Peter Zijlstra]

On Wed, Sep 13, 2023 at 05:49:53PM +0200, Oleg Nesterov wrote:
> 1. Kill the "lockmember" argument. It is always s->lock plus
>    __seqprop_##lockname##_sequence() already uses s->lock and
>    ignores "lockmember".
> 
> 2. Kill the "lock_acquire" argument. __seqprop_##lockname##_sequence()
>    can use the same "lockbase" prefix for _lock and _unlock.
> 
> Apart from line numbers, gcc -E outputs the same code.

With seqlock_ww_mutex gone, yes this is a nice cleanup.

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/seqlock.h | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index 987a59d977c5..ac6631bd5706 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -191,11 +191,9 @@ static inline void seqcount_lockdep_reader_access(const seqcount_t *s)
>   * @lockname:		"LOCKNAME" part of seqcount_LOCKNAME_t
>   * @locktype:		LOCKNAME canonical C data type
>   * @preemptible:	preemptibility of above locktype
> - * @lockmember:		argument for lockdep_assert_held()
> - * @lockbase:		associated lock release function (prefix only)
> - * @lock_acquire:	associated lock acquisition function (full call)
> + * @lockbase:		prefix for associated lock/unlock
>   */
> -#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockmember, lockbase, lock_acquire) \
> +#define SEQCOUNT_LOCKNAME(lockname, locktype, preemptible, lockbase)	\
>  typedef struct seqcount_##lockname {					\
>  	seqcount_t		seqcount;				\
>  	__SEQ_LOCK(locktype	*lock);					\
> @@ -216,7 +214,7 @@ __seqprop_##lockname##_sequence(const seqcount_##lockname##_t *s)	\
>  		return seq;						\
>  									\
>  	if (preemptible && unlikely(seq & 1)) {				\
> -		__SEQ_LOCK(lock_acquire);				\
> +		__SEQ_LOCK(lockbase##_lock(s->lock));			\
>  		__SEQ_LOCK(lockbase##_unlock(s->lock));			\
>  									\
>  		/*							\
> @@ -242,7 +240,7 @@ __seqprop_##lockname##_preemptible(const seqcount_##lockname##_t *s)	\
>  static __always_inline void						\
>  __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s)		\
>  {									\
> -	__SEQ_LOCK(lockdep_assert_held(lockmember));			\
> +	__SEQ_LOCK(lockdep_assert_held(s->lock));			\
>  }
>  
>  /*
> @@ -271,10 +269,10 @@ static inline void __seqprop_assert(const seqcount_t *s)
>  
>  #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
>  
> -SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    s->lock,        raw_spin, raw_spin_lock(s->lock))
> -SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, s->lock,        spin,     spin_lock(s->lock))
> -SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, s->lock,        read,     read_lock(s->lock))
> -SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     s->lock,        mutex,    mutex_lock(s->lock))
> +SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    raw_spin)
> +SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, spin)
> +SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, read)
> +SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
>  
>  /*
>   * SEQCNT_LOCKNAME_ZERO - static initializer for seqcount_LOCKNAME_t
> -- 
> 2.25.1.362.g51ebf55
> 

Re: [PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Oleg Nesterov]

On 09/16, Peter Zijlstra wrote:
>
> On Wed, Sep 13, 2023 at 05:49:53PM +0200, Oleg Nesterov wrote:
> > 1. Kill the "lockmember" argument. It is always s->lock plus
> >    __seqprop_##lockname##_sequence() already uses s->lock and
> >    ignores "lockmember".
> >
> > 2. Kill the "lock_acquire" argument. __seqprop_##lockname##_sequence()
> >    can use the same "lockbase" prefix for _lock and _unlock.
> >
> > Apart from line numbers, gcc -E outputs the same code.
>
> With seqlock_ww_mutex gone, yes this is a nice cleanup.

Thanks.

Can you look at 2/5? To me it looks like a good cleanup too.
I am going to resend 1/5 and 2/5, as no one is interested in
stats_lock change.

Oleg.

Re: [PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Peter Zijlstra]

On Thu, Sep 21, 2023 at 01:48:26PM +0200, Oleg Nesterov wrote:
> On 09/16, Peter Zijlstra wrote:
> >
> > On Wed, Sep 13, 2023 at 05:49:53PM +0200, Oleg Nesterov wrote:
> > > 1. Kill the "lockmember" argument. It is always s->lock plus
> > >    __seqprop_##lockname##_sequence() already uses s->lock and
> > >    ignores "lockmember".
> > >
> > > 2. Kill the "lock_acquire" argument. __seqprop_##lockname##_sequence()
> > >    can use the same "lockbase" prefix for _lock and _unlock.
> > >
> > > Apart from line numbers, gcc -E outputs the same code.
> >
> > With seqlock_ww_mutex gone, yes this is a nice cleanup.
> 
> Thanks.
> 
> Can you look at 2/5? To me it looks like a good cleanup too.
> I am going to resend 1/5 and 2/5, as no one is interested in
> stats_lock change.

2 seems okay. Will need a new changelog without the rest thouhg. Perhaps
talk about how it perserves the constness instead?

Re: [PATCH 1/5] seqlock: simplify SEQCOUNT_LOCKNAME()

[Oleg Nesterov]

On 09/21, Peter Zijlstra wrote:
>
> On Thu, Sep 21, 2023 at 01:48:26PM +0200, Oleg Nesterov wrote:
> >
> > Can you look at 2/5? To me it looks like a good cleanup too.
> > I am going to resend 1/5 and 2/5, as no one is interested in
> > stats_lock change.
>
> 2 seems okay. Will need a new changelog without the rest thouhg. Perhaps
> talk about how it perserves the constness instead?

OK, will do, thanks.

Oleg.

[PATCH 2/5] seqlock: change __seqprop() to return the function pointer

[Oleg Nesterov]

Preparation, see the next patch.

This way it is trivial to add the new seqprop's with 2 or more args,
and we do not loose the type info.

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

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index ac6631bd5706..41e36f8afad4 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -200,9 +200,9 @@ typedef struct seqcount_##lockname {					\
 } seqcount_##lockname##_t;						\
 									\
 static __always_inline seqcount_t *					\
-__seqprop_##lockname##_ptr(seqcount_##lockname##_t *s)			\
+__seqprop_##lockname##_ptr(const seqcount_##lockname##_t *s)		\
 {									\
-	return &s->seqcount;						\
+	return (void *)&s->seqcount; /* drop const */			\
 }									\
 									\
 static __always_inline unsigned						\
@@ -292,19 +292,19 @@ SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
 #define SEQCNT_WW_MUTEX_ZERO(name, lock) 	SEQCOUNT_LOCKNAME_ZERO(name, lock)
 
 #define __seqprop_case(s, lockname, prop)				\
-	seqcount_##lockname##_t: __seqprop_##lockname##_##prop((void *)(s))
+	seqcount_##lockname##_t: __seqprop_##lockname##_##prop
 
 #define __seqprop(s, prop) _Generic(*(s),				\
-	seqcount_t:		__seqprop_##prop((void *)(s)),		\
+	seqcount_t:		__seqprop_##prop,			\
 	__seqprop_case((s),	raw_spinlock,	prop),			\
 	__seqprop_case((s),	spinlock,	prop),			\
 	__seqprop_case((s),	rwlock,		prop),			\
 	__seqprop_case((s),	mutex,		prop))
 
-#define seqprop_ptr(s)			__seqprop(s, ptr)
-#define seqprop_sequence(s)		__seqprop(s, sequence)
-#define seqprop_preemptible(s)		__seqprop(s, preemptible)
-#define seqprop_assert(s)		__seqprop(s, assert)
+#define seqprop_ptr(s)			__seqprop(s, 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)
 
 /**
  * __read_seqcount_begin() - begin a seqcount_t read section w/o barrier
-- 
2.25.1.362.g51ebf55

Re: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer

[kernel test robot]

Hi Oleg,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/locking/core]
[also build test WARNING on tip/sched/core linus/master v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleg-Nesterov/seqlock-simplify-SEQCOUNT_LOCKNAME/20230913-235245
base:   tip/locking/core
patch link:    https://lore.kernel.org/r/20230913154956.GA26245%40redhat.com
patch subject: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230914/202309140117.J5VH7p4z-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309140117.J5VH7p4z-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309140117.J5VH7p4z-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/rbtree_latch.h:37,
                    from include/linux/bpf.h:14,
                    from include/linux/filter.h:9,
                    from kernel/bpf/core.c:21:
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
>> include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/bpf/core.c: At top level:
   kernel/bpf/core.c:1638:12: warning: no previous prototype for 'bpf_probe_read_kernel' [-Wmissing-prototypes]
    1638 | u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
         |            ^~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/rbtree_latch.h:37,
                    from include/linux/bpf.h:14,
                    from kernel/bpf/syscall.c:4:
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
>> include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
--
   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from include/linux/ptrace.h:6,
                    from include/uapi/asm-generic/bpf_perf_event.h:4,
                    from ./arch/m68k/include/generated/uapi/asm/bpf_perf_event.h:1,
                    from include/uapi/linux/bpf_perf_event.h:11,
                    from kernel/bpf/btf.c:6:
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
>> include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/bpf/btf.c: In function 'btf_seq_show':
   kernel/bpf/btf.c:7075:29: warning: function 'btf_seq_show' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    7075 |         seq_vprintf((struct seq_file *)show->target, fmt, args);
         |                             ^~~~~~~~
   kernel/bpf/btf.c: In function 'btf_snprintf_show':
   kernel/bpf/btf.c:7112:9: warning: function 'btf_snprintf_show' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    7112 |         len = vsnprintf(show->target, ssnprintf->len_left, fmt, args);
         |         ^~~


vim +176 include/linux/u64_stats_sync.h

68107df5f2cb5d Frederic Weisbecker 2016-09-26  172  
68107df5f2cb5d Frederic Weisbecker 2016-09-26  173  static inline bool __u64_stats_fetch_retry(const struct u64_stats_sync *syncp,
16b8a4761cbe50 Eric Dumazet        2010-06-22  174  					   unsigned int start)
16b8a4761cbe50 Eric Dumazet        2010-06-22  175  {
16b8a4761cbe50 Eric Dumazet        2010-06-22 @176  	return read_seqcount_retry(&syncp->seq, start);
44b0c2957adc62 Thomas Gleixner     2022-08-25  177  }
44b0c2957adc62 Thomas Gleixner     2022-08-25  178  #endif /* !64 bit */
44b0c2957adc62 Thomas Gleixner     2022-08-25  179  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer

[Oleg Nesterov]

On 09/14, kernel test robot wrote:
>
> Hi Oleg,

Hi kernel test robot, thank you very much.

> >> include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]

Yes, a lot of seqcount_t helpers pass the "const seqcount_t *s" to
__seqprop_ptr(s) and this is wrong.

Before this patch __seqprop(s) did "(void *)(s)" and this masked the
problem.

I've updated __seqprop_##lockname##_ptr() for this reason, but didn't
notice that __seqprop_ptr() should be changed too.

Oleg.

Re: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer

[kernel test robot]

Hi Oleg,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/locking/core]
[also build test WARNING on tip/sched/core linus/master v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleg-Nesterov/seqlock-simplify-SEQCOUNT_LOCKNAME/20230913-235245
base:   tip/locking/core
patch link:    https://lore.kernel.org/r/20230913154956.GA26245%40redhat.com
patch subject: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230914/202309140156.adz3K06E-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309140156.adz3K06E-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309140156.adz3K06E-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from arch/parisc/kernel/asm-offsets.c:18:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
--
   In file included from include/linux/mmzone.h:17,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from kernel/fork.c:16:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_begin':
   include/linux/u64_stats_sync.h:170:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     170 |         return read_seqcount_begin(&syncp->seq);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:170:16: note: in expansion of macro 'read_seqcount_begin'
     170 |         return read_seqcount_begin(&syncp->seq);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
   include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
--
   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from include/linux/hardirq.h:9,
                    from include/linux/interrupt.h:11,
                    from kernel/panic.c:14:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/panic.c: In function '__warn':
   kernel/panic.c:666:17: warning: function '__warn' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
     666 |                 vprintk(args->fmt, args->args);
         |                 ^~~~~~~
--
   In file included from include/linux/mmzone.h:17,
                    from include/linux/gfp.h:7,
                    from include/linux/mm.h:7,
                    from kernel/audit.c:38:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_begin':
   include/linux/u64_stats_sync.h:170:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     170 |         return read_seqcount_begin(&syncp->seq);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:170:16: note: in expansion of macro 'read_seqcount_begin'
     170 |         return read_seqcount_begin(&syncp->seq);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
   include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/audit.c: In function 'audit_log_vformat':
   kernel/audit.c:1963:9: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1963 |         len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args);
         |         ^~~
   kernel/audit.c:1972:17: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1972 |                 len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args2);
         |                 ^~~
--
   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from include/linux/hardirq.h:9,
                    from include/linux/interrupt.h:11,
                    from include/linux/trace_recursion.h:5,
                    from include/linux/ftrace.h:10,
                    from include/linux/kprobes.h:28,
                    from kernel/kprobes.c:23:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_begin':
   include/linux/u64_stats_sync.h:170:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     170 |         return read_seqcount_begin(&syncp->seq);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:170:16: note: in expansion of macro 'read_seqcount_begin'
     170 |         return read_seqcount_begin(&syncp->seq);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
   include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/kprobes.c: At top level:
   kernel/kprobes.c:1853:12: warning: no previous prototype for 'kprobe_exceptions_notify' [-Wmissing-prototypes]
    1853 | int __weak kprobe_exceptions_notify(struct notifier_block *self,
         |            ^~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from kernel/iomem.c:2:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/iomem.c: At top level:
   kernel/iomem.c:9:22: warning: no previous prototype for 'ioremap_cache' [-Wmissing-prototypes]
       9 | __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size)
         |                      ^~~~~~~~~~~~~
--
   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from kernel/time/hrtimer.c:25:
   include/linux/fs.h: In function 'i_size_read':
>> include/linux/fs.h:867:43: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                                           ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:867:23: note: in expansion of macro 'read_seqcount_begin'
     867 |                 seq = read_seqcount_begin(&inode->i_size_seqcount);
         |                       ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/fs.h:869:38: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/fs.h:869:18: note: in expansion of macro 'read_seqcount_retry'
     869 |         } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
         |                  ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_begin':
   include/linux/u64_stats_sync.h:170:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     170 |         return read_seqcount_begin(&syncp->seq);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:170:16: note: in expansion of macro 'read_seqcount_begin'
     170 |         return read_seqcount_begin(&syncp->seq);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
   include/linux/u64_stats_sync.h:176:36: warning: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   kernel/time/hrtimer.c: At top level:
   kernel/time/hrtimer.c:120:35: warning: initialized field overwritten [-Woverride-init]
     120 |         [CLOCK_REALTIME]        = HRTIMER_BASE_REALTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:120:35: note: (near initialization for 'hrtimer_clock_to_base_table[0]')
   kernel/time/hrtimer.c:121:35: warning: initialized field overwritten [-Woverride-init]
     121 |         [CLOCK_MONOTONIC]       = HRTIMER_BASE_MONOTONIC,
         |                                   ^~~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:121:35: note: (near initialization for 'hrtimer_clock_to_base_table[1]')
   kernel/time/hrtimer.c:122:35: warning: initialized field overwritten [-Woverride-init]
     122 |         [CLOCK_BOOTTIME]        = HRTIMER_BASE_BOOTTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:122:35: note: (near initialization for 'hrtimer_clock_to_base_table[7]')
   kernel/time/hrtimer.c:123:35: warning: initialized field overwritten [-Woverride-init]
     123 |         [CLOCK_TAI]             = HRTIMER_BASE_TAI,
         |                                   ^~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:123:35: note: (near initialization for 'hrtimer_clock_to_base_table[11]')
..


vim +867 include/linux/fs.h

375e289ea85166 J. Bruce Fields 2012-04-18  843  
7506ae6a7033f6 Jan Kara        2021-05-24  844  void filemap_invalidate_lock_two(struct address_space *mapping1,
7506ae6a7033f6 Jan Kara        2021-05-24  845  				 struct address_space *mapping2);
7506ae6a7033f6 Jan Kara        2021-05-24  846  void filemap_invalidate_unlock_two(struct address_space *mapping1,
7506ae6a7033f6 Jan Kara        2021-05-24  847  				   struct address_space *mapping2);
7506ae6a7033f6 Jan Kara        2021-05-24  848  
7506ae6a7033f6 Jan Kara        2021-05-24  849  
^1da177e4c3f41 Linus Torvalds  2005-04-16  850  /*
^1da177e4c3f41 Linus Torvalds  2005-04-16  851   * NOTE: in a 32bit arch with a preemptable kernel and
^1da177e4c3f41 Linus Torvalds  2005-04-16  852   * an UP compile the i_size_read/write must be atomic
^1da177e4c3f41 Linus Torvalds  2005-04-16  853   * with respect to the local cpu (unlike with preempt disabled),
^1da177e4c3f41 Linus Torvalds  2005-04-16  854   * but they don't need to be atomic with respect to other cpus like in
^1da177e4c3f41 Linus Torvalds  2005-04-16  855   * true SMP (so they need either to either locally disable irq around
^1da177e4c3f41 Linus Torvalds  2005-04-16  856   * the read or for example on x86 they can be still implemented as a
^1da177e4c3f41 Linus Torvalds  2005-04-16  857   * cmpxchg8b without the need of the lock prefix). For SMP compiles
^1da177e4c3f41 Linus Torvalds  2005-04-16  858   * and 64bit archs it makes no difference if preempt is enabled or not.
^1da177e4c3f41 Linus Torvalds  2005-04-16  859   */
48ed214d10ae3c Jan Engelhardt  2006-12-06  860  static inline loff_t i_size_read(const struct inode *inode)
^1da177e4c3f41 Linus Torvalds  2005-04-16  861  {
^1da177e4c3f41 Linus Torvalds  2005-04-16  862  #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
^1da177e4c3f41 Linus Torvalds  2005-04-16  863  	loff_t i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  864  	unsigned int seq;
^1da177e4c3f41 Linus Torvalds  2005-04-16  865  
^1da177e4c3f41 Linus Torvalds  2005-04-16  866  	do {
^1da177e4c3f41 Linus Torvalds  2005-04-16 @867  		seq = read_seqcount_begin(&inode->i_size_seqcount);
^1da177e4c3f41 Linus Torvalds  2005-04-16  868  		i_size = inode->i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  869  	} while (read_seqcount_retry(&inode->i_size_seqcount, seq));
^1da177e4c3f41 Linus Torvalds  2005-04-16  870  	return i_size;
2496396fcb4440 Thomas Gleixner 2019-10-15  871  #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
^1da177e4c3f41 Linus Torvalds  2005-04-16  872  	loff_t i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  873  
^1da177e4c3f41 Linus Torvalds  2005-04-16  874  	preempt_disable();
^1da177e4c3f41 Linus Torvalds  2005-04-16  875  	i_size = inode->i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  876  	preempt_enable();
^1da177e4c3f41 Linus Torvalds  2005-04-16  877  	return i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  878  #else
^1da177e4c3f41 Linus Torvalds  2005-04-16  879  	return inode->i_size;
^1da177e4c3f41 Linus Torvalds  2005-04-16  880  #endif
^1da177e4c3f41 Linus Torvalds  2005-04-16  881  }
^1da177e4c3f41 Linus Torvalds  2005-04-16  882  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer

[kernel test robot]

Hi Oleg,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/locking/core]
[also build test ERROR on tip/sched/core linus/master v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleg-Nesterov/seqlock-simplify-SEQCOUNT_LOCKNAME/20230913-235245
base:   tip/locking/core
patch link:    https://lore.kernel.org/r/20230913154956.GA26245%40redhat.com
patch subject: [PATCH 2/5] seqlock: change __seqprop() to return the function pointer
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20230914/202309140355.Sglwk06B-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309140355.Sglwk06B-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309140355.Sglwk06B-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/hrtimer.h:20,
                    from include/linux/sched.h:20,
                    from arch/powerpc/kernel/syscalls.c:18:
   include/linux/u64_stats_sync.h: In function '__u64_stats_fetch_retry':
>> include/linux/u64_stats_sync.h:176:36: error: passing argument 1 of '__seqprop_ptr' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                                    ^~~~~~~~~~~
   include/linux/seqlock.h:304:59: note: in definition of macro 'seqprop_ptr'
     304 | #define seqprop_ptr(s)                  __seqprop(s, ptr)(s)
         |                                                           ^
   include/linux/u64_stats_sync.h:176:16: note: in expansion of macro 'read_seqcount_retry'
     176 |         return read_seqcount_retry(&syncp->seq, start);
         |                ^~~~~~~~~~~~~~~~~~~
   include/linux/seqlock.h:250:53: note: expected 'seqcount_t *' {aka 'struct seqcount *'} but argument is of type 'const seqcount_t *' {aka 'const struct seqcount *'}
     250 | static inline seqcount_t *__seqprop_ptr(seqcount_t *s)
         |                                         ~~~~~~~~~~~~^
   cc1: all warnings being treated as errors


vim +176 include/linux/u64_stats_sync.h

68107df5f2cb5d Frederic Weisbecker 2016-09-26  172  
68107df5f2cb5d Frederic Weisbecker 2016-09-26  173  static inline bool __u64_stats_fetch_retry(const struct u64_stats_sync *syncp,
16b8a4761cbe50 Eric Dumazet        2010-06-22  174  					   unsigned int start)
16b8a4761cbe50 Eric Dumazet        2010-06-22  175  {
16b8a4761cbe50 Eric Dumazet        2010-06-22 @176  	return read_seqcount_retry(&syncp->seq, start);
44b0c2957adc62 Thomas Gleixner     2022-08-25  177  }
44b0c2957adc62 Thomas Gleixner     2022-08-25  178  #endif /* !64 bit */
44b0c2957adc62 Thomas Gleixner     2022-08-25  179  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/5] seqlock: introduce seqprop_lock/unlock

[Oleg Nesterov]

which can be used to take/release the corresponding lock.

Thanks to the previous patch, it is trivial to pass 2 arguments to
the new __seqprop_##lockname##_lock/unlock "methods", plus we do not
loose the type info and thus the new seqprop's are "type safe".

So for example

	void func(seqcount_rwlock_t *s, rwlock_t *l)
	{
		seqprop_lock(s, l);
	}

happily compiles, but this one

	void func(seqcount_rwlock_t *s, spinlock_t *l)
	{
		seqprop_lock(s, l);
	}

doesn't.

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

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 41e36f8afad4..9831683a0102 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -241,6 +241,21 @@ static __always_inline void						\
 __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s)		\
 {									\
 	__SEQ_LOCK(lockdep_assert_held(s->lock));			\
+}									\
+									\
+static __always_inline void						\
+__seqprop_##lockname##_lock(seqcount_##lockname##_t *s,			\
+				locktype *lock)				\
+{									\
+	__SEQ_LOCK(WARN_ON_ONCE(s->lock != lock));			\
+	lockbase##_lock(lock);						\
+}									\
+									\
+static __always_inline void						\
+__seqprop_##lockname##_unlock(seqcount_##lockname##_t *s,		\
+				locktype *lock)				\
+{									\
+	lockbase##_unlock(lock); 					\
 }
 
 /*
@@ -306,6 +321,12 @@ SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
 #define seqprop_preemptible(s)		__seqprop(s, preemptible)(s)
 #define seqprop_assert(s)		__seqprop(s, assert)(s)
 
+/* seqcount_t doesn't have these methods */
+static inline void __seqprop_lock   (seqcount_t *s, void *l) { BUILD_BUG(); }
+static inline void __seqprop_unlock (seqcount_t *s, void *l) { BUILD_BUG(); }
+#define seqprop_lock(s, l)		__seqprop(s, lock)(s, l)
+#define seqprop_unlock(s, l)		__seqprop(s, unlock)(s, l)
+
 /**
  * __read_seqcount_begin() - begin a seqcount_t read section w/o barrier
  * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants
-- 
2.25.1.362.g51ebf55

Re: [PATCH 3/5] seqlock: introduce seqprop_lock/unlock

[Alexey Gladkov]

On Wed, Sep 13, 2023 at 05:50:00PM +0200, Oleg Nesterov wrote:
> which can be used to take/release the corresponding lock.
> 
> Thanks to the previous patch, it is trivial to pass 2 arguments to
> the new __seqprop_##lockname##_lock/unlock "methods", plus we do not
> loose the type info and thus the new seqprop's are "type safe".
> 
> So for example
> 
> 	void func(seqcount_rwlock_t *s, rwlock_t *l)
> 	{
> 		seqprop_lock(s, l);
> 	}
> 
> happily compiles, but this one
> 
> 	void func(seqcount_rwlock_t *s, spinlock_t *l)
> 	{
> 		seqprop_lock(s, l);
> 	}
> 
> doesn't.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/seqlock.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index 41e36f8afad4..9831683a0102 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -241,6 +241,21 @@ static __always_inline void						\
>  __seqprop_##lockname##_assert(const seqcount_##lockname##_t *s)		\
>  {									\
>  	__SEQ_LOCK(lockdep_assert_held(s->lock));			\
> +}									\
> +									\
> +static __always_inline void						\
> +__seqprop_##lockname##_lock(seqcount_##lockname##_t *s,			\
> +				locktype *lock)				\
> +{									\
> +	__SEQ_LOCK(WARN_ON_ONCE(s->lock != lock));			\
> +	lockbase##_lock(lock);						\
> +}									\
> +									\
> +static __always_inline void						\
> +__seqprop_##lockname##_unlock(seqcount_##lockname##_t *s,		\
> +				locktype *lock)				\
> +{									\
> +	lockbase##_unlock(lock); 					\
>  }

Why are you creating a new method with an unused argument s ?

>  
>  /*
> @@ -306,6 +321,12 @@ SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)
>  #define seqprop_preemptible(s)		__seqprop(s, preemptible)(s)
>  #define seqprop_assert(s)		__seqprop(s, assert)(s)
>  
> +/* seqcount_t doesn't have these methods */
> +static inline void __seqprop_lock   (seqcount_t *s, void *l) { BUILD_BUG(); }
> +static inline void __seqprop_unlock (seqcount_t *s, void *l) { BUILD_BUG(); }
> +#define seqprop_lock(s, l)		__seqprop(s, lock)(s, l)
> +#define seqprop_unlock(s, l)		__seqprop(s, unlock)(s, l)
> +
>  /**
>   * __read_seqcount_begin() - begin a seqcount_t read section w/o barrier
>   * @s: Pointer to seqcount_t or any of the seqcount_LOCKNAME_t variants
> -- 
> 2.25.1.362.g51ebf55
> 

-- 
Rgrds, legion

Re: [PATCH 3/5] seqlock: introduce seqprop_lock/unlock

[Oleg Nesterov]

On 09/15, Alexey Gladkov wrote:
>
> On Wed, Sep 13, 2023 at 05:50:00PM +0200, Oleg Nesterov wrote:
> > +									\
> > +static __always_inline void						\
> > +__seqprop_##lockname##_lock(seqcount_##lockname##_t *s,			\
> > +				locktype *lock)				\
> > +{									\
> > +	__SEQ_LOCK(WARN_ON_ONCE(s->lock != lock));			\
> > +	lockbase##_lock(lock);						\
> > +}									\
> > +									\
> > +static __always_inline void						\
> > +__seqprop_##lockname##_unlock(seqcount_##lockname##_t *s,		\
> > +				locktype *lock)				\
> > +{									\
> > +	lockbase##_unlock(lock); 					\
> >  }
>
> Why are you creating a new method with an unused argument s ?

To make it consistent/symmetrical with _lock() which does
__SEQ_LOCK(WARN_ON_ONCE(s->lock != lock)). _unlock() could do the
same check as well, but somehow I decided it would be too much.

And with other "methods". Say, __seqprop_##lockname##_preemptible(s)
doesn't use 's' too.

Otherwise they both do not need the 1st seqcount_##lockname##_t *s
argument.

Oleg.

[PATCH 4/5] seqlock: introduce read_seqcount_begin_or_lock() and friends

[Oleg Nesterov]

See the comment in the patch.

NOTE: currently __seqprop_##lockname##_sequence() takes and drops s->lock
if preemptible && CONFIG_PREEMPT_RT. With the previous changes it is simple
to change this behaviour for the read_seqcount_begin_or_lock() case, iiuc
it makes more sense to return with s->lock held and "(seq & 1) == 1".

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

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 9831683a0102..503813b3bab6 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -1239,4 +1239,54 @@ done_seqretry_irqrestore(seqlock_t *lock, int seq, unsigned long flags)
 	if (seq & 1)
 		read_sequnlock_excl_irqrestore(lock, flags);
 }
+
+/*
+ * Like read_seqbegin_or_lock/need_seqretry/done_seqretry above
+ * but for seqcount_LOCKNAME_t.
+ */
+
+#define read_seqcount_begin_or_lock(s, lock, seq)		\
+do {								\
+	if (!(*(seq) & 1))					\
+		*(seq) = read_seqcount_begin(s);		\
+	else							\
+		seqprop_lock((s), (lock));			\
+} while (0)
+
+#define need_seqcount_retry(s, seq)				\
+({								\
+	!((seq) & 1) && read_seqcount_retry((s), (seq));	\
+})
+
+#define done_seqcount_retry(s, lock, seq)			\
+do {								\
+	if ((seq) & 1)						\
+		seqprop_unlock((s), (lock));			\
+} while (0)
+
+
+#define read_seqcount_begin_or_lock_irqsave(s, lock, seq)	\
+({								\
+	unsigned long flags = 0;				\
+								\
+	if (!(*(seq) & 1))					\
+		*(seq) = read_seqcount_begin(s);		\
+	else {							\
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))		\
+			local_irq_save(flags);			\
+		seqprop_lock((s), (lock));			\
+	}							\
+								\
+	flags;							\
+})
+
+#define done_seqcount_retry_irqrestore(s, lock, seq, flags)	\
+do {								\
+	if ((seq) & 1) {					\
+		seqprop_unlock((s), (lock));			\
+		if (!IS_ENABLED(CONFIG_PREEMPT_RT))		\
+			local_irq_restore((flags));		\
+	}							\
+} while (0)
+
 #endif /* __LINUX_SEQLOCK_H */
-- 
2.25.1.362.g51ebf55

[PATCH 5/5] time,signal: turn signal_struct.stats_lock into seqcount_rwlock_t

[Oleg Nesterov]

This way thread_group_cputime() doesn't exclude other readers on the
2nd pass.

thread_group_cputime() still needs to disable irqs because stats_lock
nests inside siglock. But once we change the getrusage()-like users to
rely on stats_lock we can remove this dependency, and after that there
will be no need for _irqsave.

And IIUC, this is the bugfix for CONFIG_PREEMPT_RT? Before this patch
read_seqbegin_or_lock() can spin in __read_seqcount_begin() while the
write_seqlock(stats_lock) section was preempted.

While at it, change the main loop to use __for_each_thread(sig, t).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/sched/signal.h |  4 +++-
 kernel/exit.c                | 12 ++++++++----
 kernel/fork.c                |  3 ++-
 kernel/sched/cputime.c       | 10 ++++++----
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index d7fa3ca2fa53..c7c0928b877d 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -182,7 +182,9 @@ struct signal_struct {
 	 * Live threads maintain their own counters and add to these
 	 * in __exit_signal, except for the group leader.
 	 */
-	seqlock_t stats_lock;
+	rwlock_t stats_lock;
+	seqcount_rwlock_t stats_seqc;
+
 	u64 utime, stime, cutime, cstime;
 	u64 gtime;
 	u64 cgtime;
diff --git a/kernel/exit.c b/kernel/exit.c
index f3ba4b97a7d9..8dedb7138f9c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -182,7 +182,8 @@ static void __exit_signal(struct task_struct *tsk)
 	 * see the empty ->thread_head list.
 	 */
 	task_cputime(tsk, &utime, &stime);
-	write_seqlock(&sig->stats_lock);
+	write_lock(&sig->stats_lock);
+	write_seqcount_begin(&sig->stats_seqc);
 	sig->utime += utime;
 	sig->stime += stime;
 	sig->gtime += task_gtime(tsk);
@@ -196,7 +197,8 @@ static void __exit_signal(struct task_struct *tsk)
 	sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
 	sig->nr_threads--;
 	__unhash_process(tsk, group_dead);
-	write_sequnlock(&sig->stats_lock);
+	write_seqcount_end(&sig->stats_seqc);
+	write_unlock(&sig->stats_lock);
 
 	/*
 	 * Do this under ->siglock, we can race with another thread
@@ -1160,7 +1162,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 		 */
 		thread_group_cputime_adjusted(p, &tgutime, &tgstime);
 		spin_lock_irq(&current->sighand->siglock);
-		write_seqlock(&psig->stats_lock);
+		write_lock(&psig->stats_lock);
+		write_seqcount_begin(&psig->stats_seqc);
 		psig->cutime += tgutime + sig->cutime;
 		psig->cstime += tgstime + sig->cstime;
 		psig->cgtime += task_gtime(p) + sig->gtime + sig->cgtime;
@@ -1183,7 +1186,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 			psig->cmaxrss = maxrss;
 		task_io_accounting_add(&psig->ioac, &p->ioac);
 		task_io_accounting_add(&psig->ioac, &sig->ioac);
-		write_sequnlock(&psig->stats_lock);
+		write_seqcount_end(&psig->stats_seqc);
+		write_unlock(&psig->stats_lock);
 		spin_unlock_irq(&current->sighand->siglock);
 	}
 
diff --git a/kernel/fork.c b/kernel/fork.c
index b9d3aa493bbd..bbd5604053f8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1870,7 +1870,8 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	sig->curr_target = tsk;
 	init_sigpending(&sig->shared_pending);
 	INIT_HLIST_HEAD(&sig->multiprocess);
-	seqlock_init(&sig->stats_lock);
+	rwlock_init(&sig->stats_lock);
+	seqcount_rwlock_init(&sig->stats_seqc, &sig->stats_lock);
 	prev_cputime_init(&sig->prev_cputime);
 
 #ifdef CONFIG_POSIX_TIMERS
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index af7952f12e6c..bd6a85bd2a49 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -333,12 +333,13 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
 	nextseq = 0;
 	do {
 		seq = nextseq;
-		flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq);
+		flags = read_seqcount_begin_or_lock_irqsave(&sig->stats_seqc,
+						     &sig->stats_lock, &seq);
 		times->utime = sig->utime;
 		times->stime = sig->stime;
 		times->sum_exec_runtime = sig->sum_sched_runtime;
 
-		for_each_thread(tsk, t) {
+		__for_each_thread(sig, t) {
 			task_cputime(t, &utime, &stime);
 			times->utime += utime;
 			times->stime += stime;
@@ -346,8 +347,9 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
 		}
 		/* If lockless access failed, take the lock. */
 		nextseq = 1;
-	} while (need_seqretry(&sig->stats_lock, seq));
-	done_seqretry_irqrestore(&sig->stats_lock, seq, flags);
+	} while (need_seqcount_retry(&sig->stats_seqc, seq));
+	done_seqcount_retry_irqrestore(&sig->stats_seqc, &sig->stats_lock,
+					seq, flags);
 	rcu_read_unlock();
 }
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH 5/5] time,signal: turn signal_struct.stats_lock into seqcount_rwlock_t

[Alexey Gladkov]

On Wed, Sep 13, 2023 at 05:50:09PM +0200, Oleg Nesterov wrote:
> This way thread_group_cputime() doesn't exclude other readers on the
> 2nd pass.
> 
> thread_group_cputime() still needs to disable irqs because stats_lock
> nests inside siglock. But once we change the getrusage()-like users to
> rely on stats_lock we can remove this dependency, and after that there
> will be no need for _irqsave.
> 
> And IIUC, this is the bugfix for CONFIG_PREEMPT_RT? Before this patch
> read_seqbegin_or_lock() can spin in __read_seqcount_begin() while the
> write_seqlock(stats_lock) section was preempted.
> 
> While at it, change the main loop to use __for_each_thread(sig, t).
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/sched/signal.h |  4 +++-
>  kernel/exit.c                | 12 ++++++++----
>  kernel/fork.c                |  3 ++-
>  kernel/sched/cputime.c       | 10 ++++++----
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index d7fa3ca2fa53..c7c0928b877d 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -182,7 +182,9 @@ struct signal_struct {
>  	 * Live threads maintain their own counters and add to these
>  	 * in __exit_signal, except for the group leader.
>  	 */
> -	seqlock_t stats_lock;
> +	rwlock_t stats_lock;
> +	seqcount_rwlock_t stats_seqc;
> +
>  	u64 utime, stime, cutime, cstime;
>  	u64 gtime;
>  	u64 cgtime;
> diff --git a/kernel/exit.c b/kernel/exit.c
> index f3ba4b97a7d9..8dedb7138f9c 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -182,7 +182,8 @@ static void __exit_signal(struct task_struct *tsk)
>  	 * see the empty ->thread_head list.
>  	 */
>  	task_cputime(tsk, &utime, &stime);
> -	write_seqlock(&sig->stats_lock);
> +	write_lock(&sig->stats_lock);
> +	write_seqcount_begin(&sig->stats_seqc);
>  	sig->utime += utime;
>  	sig->stime += stime;
>  	sig->gtime += task_gtime(tsk);
> @@ -196,7 +197,8 @@ static void __exit_signal(struct task_struct *tsk)
>  	sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
>  	sig->nr_threads--;
>  	__unhash_process(tsk, group_dead);
> -	write_sequnlock(&sig->stats_lock);
> +	write_seqcount_end(&sig->stats_seqc);
> +	write_unlock(&sig->stats_lock);
>  
>  	/*
>  	 * Do this under ->siglock, we can race with another thread
> @@ -1160,7 +1162,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
>  		 */
>  		thread_group_cputime_adjusted(p, &tgutime, &tgstime);
>  		spin_lock_irq(&current->sighand->siglock);
> -		write_seqlock(&psig->stats_lock);
> +		write_lock(&psig->stats_lock);
> +		write_seqcount_begin(&psig->stats_seqc);
>  		psig->cutime += tgutime + sig->cutime;
>  		psig->cstime += tgstime + sig->cstime;
>  		psig->cgtime += task_gtime(p) + sig->gtime + sig->cgtime;
> @@ -1183,7 +1186,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
>  			psig->cmaxrss = maxrss;
>  		task_io_accounting_add(&psig->ioac, &p->ioac);
>  		task_io_accounting_add(&psig->ioac, &sig->ioac);
> -		write_sequnlock(&psig->stats_lock);
> +		write_seqcount_end(&psig->stats_seqc);
> +		write_unlock(&psig->stats_lock);
>  		spin_unlock_irq(&current->sighand->siglock);
>  	}
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b9d3aa493bbd..bbd5604053f8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1870,7 +1870,8 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>  	sig->curr_target = tsk;
>  	init_sigpending(&sig->shared_pending);
>  	INIT_HLIST_HEAD(&sig->multiprocess);
> -	seqlock_init(&sig->stats_lock);
> +	rwlock_init(&sig->stats_lock);
> +	seqcount_rwlock_init(&sig->stats_seqc, &sig->stats_lock);
>  	prev_cputime_init(&sig->prev_cputime);
>  
>  #ifdef CONFIG_POSIX_TIMERS
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index af7952f12e6c..bd6a85bd2a49 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -333,12 +333,13 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
>  	nextseq = 0;
>  	do {
>  		seq = nextseq;
> -		flags = read_seqbegin_or_lock_irqsave(&sig->stats_lock, &seq);
> +		flags = read_seqcount_begin_or_lock_irqsave(&sig->stats_seqc,
> +						     &sig->stats_lock, &seq);
>  		times->utime = sig->utime;
>  		times->stime = sig->stime;
>  		times->sum_exec_runtime = sig->sum_sched_runtime;
>  
> -		for_each_thread(tsk, t) {
> +		__for_each_thread(sig, t) {
>  			task_cputime(t, &utime, &stime);
>  			times->utime += utime;
>  			times->stime += stime;
> @@ -346,8 +347,9 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
>  		}
>  		/* If lockless access failed, take the lock. */
>  		nextseq = 1;

I think you're right, and indeed there is a possible situation here
where write_seqlock will force all readers to take locks one after
another.

I really don’t know how critical it is in this place.

> -	} while (need_seqretry(&sig->stats_lock, seq));
> -	done_seqretry_irqrestore(&sig->stats_lock, seq, flags);
> +	} while (need_seqcount_retry(&sig->stats_seqc, seq));
> +	done_seqcount_retry_irqrestore(&sig->stats_seqc, &sig->stats_lock,
> +					seq, flags);
>  	rcu_read_unlock();
>  }
>  
> -- 
> 2.25.1.362.g51ebf55
> 

-- 
Rgrds, legion