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

[Alexey Gladkov <legion@kernel.org>]

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