linux-kernel - Re: [tip:locking/core] locking/atomics: Simplify the op definitions in atomic.h some more

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180509130316.GB20254@arm.com>
Date:   Wed, 9 May 2018 14:03:16 +0100
From:   Will Deacon <will.deacon@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        mark.rutland@....com, torvalds@...ux-foundation.org,
        paulmck@...ux.vnet.ibm.com, mingo@...nel.org, tglx@...utronix.de,
        hpa@...or.com, linux-tip-commits@...r.kernel.org
Subject: Re: [tip:locking/core] locking/atomics: Simplify the op definitions
 in atomic.h some more

Hi Ingo, Peter,

[I was at a conference last week and not reading email]

On Wed, May 09, 2018 at 09:33:27AM +0200, Peter Zijlstra wrote:
> On Sun, May 06, 2018 at 05:14:36AM -0700, tip-bot for Ingo Molnar wrote:
> > Commit-ID:  87d655a48dfe74293f72dc001ed042142cf00d44
> > Gitweb:     https://git.kernel.org/tip/87d655a48dfe74293f72dc001ed042142cf00d44
> > Author:     Ingo Molnar <mingo@...nel.org>
> > AuthorDate: Sat, 5 May 2018 10:36:35 +0200
> > Committer:  Ingo Molnar <mingo@...nel.org>
> > CommitDate: Sat, 5 May 2018 15:22:44 +0200
> > 
> > locking/atomics: Simplify the op definitions in atomic.h some more
> > 
> > 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
> >  #  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
> > 
> > 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
> >  #  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 idea is that because we already group these APIs by certain defines
> > such as atomic_fetch_dec_relaxed and atomic_fetch_dec in the primary
> > branches - we can do the same in the secondary branch as well.
> > 
> 
> ARGH, why did you merge this? It's pointless wankery, and doesn't solve
> anything wrt. the annotated atomic crap.
> 
> And if we're going to do codegen, we might as well all generate this
> anyway, so all this mucking about is a complete waste of time.

I have to agree here.

The reason we allowed the atomic primitives to be overridden on a
fine-grained basis is because there are often architecture-specific
barriers or instructions which mean that it is common to be able to provide
optimised atomics in some cases, but not others. Given that this stuff is
error-prone, having the generic code provide a safe and correct fallback
for all of the atomics that the architecture doesn't implement seemed like
a good thing to me. New architecture ports basically just have to provide
the weakest thing they have and a fence to get started, then they can
optimise where they can and not have to worry about the rest of the atomics
API.

With this patch, we're already seeing arch code (powerpc, risc-v) having
to add what is basically boiler-plate code, and it seems like we're just
doing this to make the generic code more readable! I'd much prefer we kept
the arch code simple, and took on the complexity burden in the generic code
where it can be looked after in one place.

For instrumentation, we're going to need to generate this stuff *anyway*,
so I think the readability argument mostly disappears.

Will