This is the mail archive of the libc-ports@sources.redhat.com mailing list for the libc-ports 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] Unify pthread_spin_[try]lock implementations.


> This code _is_ right.  It may not be optimal for all machines, but it is
> correct.  I agree that the code is not obvious and the attached patch
> clears it up.

It is sufficiently suboptimal on some machines that it does not qualify as
generically correct for a synchronization primitive.

> This is not new code.  It is the exactly same code that ARM, MIPS, HPPA
> and M68K have all been using for several years, it is just moved into the
> generic directory.

Sharing code is good.  I'm all for it.  But generic is not for code that
happens to be about right on several machines.  It's for code that is truly
generic--either it's a stub with a link warning, or it's adequate for any
configuration, only to be overridden by sysdeps versions that are optimized
for a particular configuration.

Here I think the reasonable thing to do is:

/* A machine-specific version can define SPIN_LOCK_READS_BETWEEN_CMPXCHG
   to the number of plain reads that it's optimal to spin on between uses
   of atomic_compare_and_exchange_val_acq.  If spinning forever is optimal
   then use -1.  If no plain reads here would ever be optimal, use 0.  */
#ifndef SPIN_LOCK_READS_BETWEEN_CMPXCHG
# warning machine-dependent file should define SPIN_LOCK_READS_BETWEEN_CMPXCHG
# define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
#endif

Then ARM et al can do:

/* Machine-dependent rationale about the selection of this value.  */
#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
#include <nptl/pthread_spin_lock.c>

while Teil will use -1.

> +      if (PTHREAD_SPIN_LOCK_WAIT)

Don't use implicit boolean coercion.
Use "if (SPIN_LOCK_READS_BETWEEN_CMPXCHG >= 0)".

> +	{
> +	  int wait = PTHREAD_SPIN_LOCK_WAIT;
> +
> +	  while (*lock != 0 && --wait)
> +	    ;

Write it:
	while (wait > 0 && *lock != 0)
	  --wait;

That handles the SPIN_LOCK_READS_BETWEEN_CMPXCHG==0 case implicitly,
avoids the ugly empty statement, and doesn't use implicit coercion.


Thanks,
Roland


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