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 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.

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]