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

  On 02/13/2017 09:39 PM, Torvald Riegel wrote:
> 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).
>
See answer in patch 1/2.

> 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?
 >
I've run own benchmarks in the same manner as your mentioned 
microbenchmark for rwlocks below.
You are right, I can't recognize a real difference between
#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
and
#define SPIN_LOCK_READS_BETWEEN_CMPXCHG -1

As it does not hurt, I prefer to use a CAS every 1000 plain reads.
A CAS is not necessary on current CPUs but from architecture 
perspective, it is more correct if there is such a serialization 
instruction.

There is a difference between
#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 0
and one of the others.

The same applies to
#define SPIN_TRYLOCK_LOAD_AND_TEST_BEFORE_XCHG 1
It does not hurt if the lock is free, but there is a difference if the 
lock is already acquired and trylock is called often.

I've saw your microbenchmark-post and added some notes.

I added a FIXME comment to re-evaluate the choice once we have the 
appropriate microbenchmarks.

> 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.
>
See pthread_spin_parameters.h in updated patch 1/2.

I've attached an updated patch due to the changes in patch 1/2 and added 
comments to the macro definitions.

Bye.
Stefan

ChangeLog:

	* sysdeps/s390/atomic-machine.h:
	(__arch_compare_and_exchange_val_32_acq):
	Cast type to omit volatile qualifier.
	(__arch_compare_and_exchange_val_64_acq): Likewise.
	(atomic_exchange_acq): Likewise.
	* sysdeps/s390/nptl/pthread_spin_init.c: Delete File.
	* sysdeps/s390/nptl/pthread_spin_lock.c: Likewise.
	* sysdeps/s390/nptl/pthread_spin_trylock.c: Likewise.
	* sysdeps/s390/nptl/pthread_spin_unlock.c: Likewise.
	* sysdeps/s390/nptl/pthread_spin_parameters.h: New File.

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

Commit Message

Comments

Patch