Re: [PATCH 2/2] S390: Use generic spinlock code.

[Torvald Riegel <triegel at redhat dot com>]

On Wed, 2017-02-08 at 15:49 +0100, Stefan Liebler wrote:
> This is an updated version of the patch, which adjusts the s390 specific 
> atomic-macros in the same way as in include/atomic.h.
> Thus passing a volatile int pointer is fine, too.

The general direction of this is okay.
Some of my comments for patch 1/2 apply here as well (eg, volatile vs.
atomics).

What I don't like is the choice of 1000 for
SPIN_LOCK_READS_BETWEEN_CMPXCHG.  Have you run benchmarks to come up
with this value, or is it a guess?  Why isn't it documented how you end
up with this number?
We can keep these with a choice such as this, but then we need to have a
FIXME comment in the code, explaining that this is just an arbitrary
choice.

I would guess that just spinning forever is sufficient, and that we
don't need to throw in a CAS every now and then; using randomized
exponential back-off might be more important.  This is something that we
would be in a better position to answer if you'd provide a
microbenchmark for this choice too.
At the end of 2016, I've posted a draft of a microbenchmark for rwlocks.
Maybe you can use this as a start and run a few experiments?

Also, I'm not quite sure whether this number is really
spinlock-specific, and I would like to find a better place for these.
IMO, they should be in some header that contains default tuning
parameters for synchronization code, which is provided by each
architecture that uses the generic spinlock; we'd have no #ifdef for the
tuning parameters, so we'd catch typos in those headers.