This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 03/14/2017 04:55 PM, Stefan Liebler wrote:
On 02/18/2017 06:05 PM, Torvald Riegel wrote:
On Wed, 2017-02-15 at 17:26 +0100, Stefan Liebler wrote:
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.

What do you mean by "more correct"?  I'm not aware of an architecture
that would not ensure that stores on one CPU will eventually become
visible to other CPUs.

Thus, as I wrote in my review of patch 1/2, I think we should just
remove the occassional CAS (ie, just do the equivalent of the -1
setting, always).

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

Right.  We do want to spin if the lock is acquired by another thread.
What we should do in a future patch is to experiment with the back-off
between loads.  tile already has some code for this, which we at least
need to integrate at some point.

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.

Yes.  I've replied to this point in the 1/2 patch thread.

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

Thanks.  I'll get to them later.

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 suggest that we'll work towards consensus on patch 1/2 first.  I
believe once that is done, patch 2/2 would likely just remove s390 code.

I've attached an updated patch which just removes the s390 code.



PING

Thanks for continuing the work on this.  I know we have some back and
forth here in terms of overall direction, but I also think we're making
progress and are continually improving the changes.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]