[PATCH] locking/atomics: Simplify the op definitions in atomic.h some more

[Ingo Molnar]

* Ingo Molnar <mingo@xxxxxxxxxx> wrote:

> Before:
> 
>  #ifndef atomic_fetch_dec_relaxed
> 
>  #ifndef atomic_fetch_dec
>  #define atomic_fetch_dec(v)	        atomic_fetch_sub(1, (v))
>  #define atomic_fetch_dec_relaxed(v)	atomic_fetch_sub_relaxed(1, (v))
>  #define atomic_fetch_dec_acquire(v)	atomic_fetch_sub_acquire(1, (v))
>  #define atomic_fetch_dec_release(v)	atomic_fetch_sub_release(1, (v))
>  #else /* atomic_fetch_dec */
>  #define atomic_fetch_dec_relaxed	atomic_fetch_dec
>  #define atomic_fetch_dec_acquire	atomic_fetch_dec
>  #define atomic_fetch_dec_release	atomic_fetch_dec
>  #endif /* atomic_fetch_dec */
> 
>  #else /* atomic_fetch_dec_relaxed */
> 
>  #ifndef atomic_fetch_dec_acquire
>  #define atomic_fetch_dec_acquire(...)					\
> 	__atomic_op_acquire(atomic_fetch_dec, __VA_ARGS__)
>  #endif
> 
>  #ifndef atomic_fetch_dec_release
>  #define atomic_fetch_dec_release(...)					\
> 	__atomic_op_release(atomic_fetch_dec, __VA_ARGS__)
>  #endif
> 
>  #ifndef atomic_fetch_dec
>  #define atomic_fetch_dec(...)						\
> 	__atomic_op_fence(atomic_fetch_dec, __VA_ARGS__)
>  #endif
>  #endif /* atomic_fetch_dec_relaxed */
> 
> After:
> 
>  #ifndef atomic_fetch_dec_relaxed
>  # ifndef atomic_fetch_dec
>  #  define atomic_fetch_dec(v)		atomic_fetch_sub(1, (v))
>  #  define atomic_fetch_dec_relaxed(v)	atomic_fetch_sub_relaxed(1, (v))
>  #  define atomic_fetch_dec_acquire(v)	atomic_fetch_sub_acquire(1, (v))
>  #  define atomic_fetch_dec_release(v)	atomic_fetch_sub_release(1, (v))
>  # else
>  #  define atomic_fetch_dec_relaxed		atomic_fetch_dec
>  #  define atomic_fetch_dec_acquire		atomic_fetch_dec
>  #  define atomic_fetch_dec_release		atomic_fetch_dec
>  # endif
>  #else
>  # ifndef atomic_fetch_dec_acquire
>  #  define atomic_fetch_dec_acquire(...)	__atomic_op_acquire(atomic_fetch_dec, __VA_ARGS__)
>  # endif
>  # ifndef atomic_fetch_dec_release
>  #  define atomic_fetch_dec_release(...)	__atomic_op_release(atomic_fetch_dec, __VA_ARGS__)
>  # endif
>  # ifndef atomic_fetch_dec
>  #  define atomic_fetch_dec(...)		__atomic_op_fence(atomic_fetch_dec, __VA_ARGS__)
>  # endif
>  #endif
> 
> The new variant is readable at a glance, and the hierarchy of defines is very 
> obvious as well.
> 
> And I think we could do even better - there's absolutely no reason why _every_ 
> operation has to be made conditional on a finegrained level - they are overriden 
> in API groups. In fact allowing individual override is arguably a fragility.
> 
> So we could do the following simplification on top of that:
> 
>  #ifndef atomic_fetch_dec_relaxed
>  # ifndef atomic_fetch_dec
>  #  define atomic_fetch_dec(v)		atomic_fetch_sub(1, (v))
>  #  define atomic_fetch_dec_relaxed(v)	atomic_fetch_sub_relaxed(1, (v))
>  #  define atomic_fetch_dec_acquire(v)	atomic_fetch_sub_acquire(1, (v))
>  #  define atomic_fetch_dec_release(v)	atomic_fetch_sub_release(1, (v))
>  # else
>  #  define atomic_fetch_dec_relaxed		atomic_fetch_dec
>  #  define atomic_fetch_dec_acquire		atomic_fetch_dec
>  #  define atomic_fetch_dec_release		atomic_fetch_dec
>  # endif
>  #else
>  # ifndef atomic_fetch_dec
>  #  define atomic_fetch_dec(...)		__atomic_op_fence(atomic_fetch_dec, __VA_ARGS__)
>  #  define atomic_fetch_dec_acquire(...)	__atomic_op_acquire(atomic_fetch_dec, __VA_ARGS__)
>  #  define atomic_fetch_dec_release(...)	__atomic_op_release(atomic_fetch_dec, __VA_ARGS__)
>  # endif
>  #endif

The attached patch implements this, which gives us another healthy simplification:

 include/linux/atomic.h | 312 ++++++++++---------------------------------------
 1 file changed, 62 insertions(+), 250 deletions(-)

Note that the simplest definition block is now:

#ifndef atomic_cmpxchg_relaxed
# define atomic_cmpxchg_relaxed			atomic_cmpxchg
# define atomic_cmpxchg_acquire			atomic_cmpxchg
# define atomic_cmpxchg_release			atomic_cmpxchg
#else
# ifndef atomic_cmpxchg
#  define atomic_cmpxchg(...)			__atomic_op_fence(atomic_cmpxchg, __VA_ARGS__)
#  define atomic_cmpxchg_acquire(...)		__atomic_op_acquire(atomic_cmpxchg, __VA_ARGS__)
#  define atomic_cmpxchg_release(...)		__atomic_op_release(atomic_cmpxchg, __VA_ARGS__)
# endif
#endif

... which is very readable!

The total linecount reduction of the two patches is pretty significant as well:

 include/linux/atomic.h | 1063 ++++++++++++++++--------------------------------
 1 file changed, 343 insertions(+), 720 deletions(-)

Note that I kept the second patch separate, because technically it changes the way 
we use the defines - it should not break anything, unless I missed some detail.

Please keep this kind of clarity and simplicity in new instrumentation patches!

Thanks,

	Ingo

==================>