Re: [PATCH] Unify pthread_spin_[try]lock implementations.

[Maxim Kuvyrkov <maxim@codesourcery.com>]

On 16/08/2012, at 4:26 AM, Roland McGrath wrote:

>> 	2012-07-09  Maxim Kuvyrkov  <maxim@codesourcery.com>
> 
> The indentation here is funny so I don't know if that's just how you were
> quoting the fragment in the message or if you meant to indent the header
> line in ChangeLog (which you should not).  Remember to use the date of
> commit in the header line.

This is just an indentation artifact in git commit log.  It will be properly formatted in the actual ChangeLog file.

> 
>> 	* nptl/pthread_spin_lock.c: New file.
>> 	* nptl/pthread_spin_trylock.c: New file.
> 
> These go in nptl/ChangeLog and don't get the "nptl/" prefix.

This I didn't notice.  Thanks.

> 
>> 	ports/
> 
> These go in the separate ChangeLog.{arm,hppa,m68k,mips} files.
> From the way you quoted the fragments it wasn't clear if that's
> what you meant.

This I know.

> 
>> +#define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000
>> +#include_next <nptl/pthread_spin_lock.c>
> 
> You don't use #include_next when you're giving the exact full file name
> like this.  Just use #include.

The machine-specific pthread_spin_lock.c files go to ports/sysdeps/<machine>/nptl/pthread_spin_lock.c, which comes first in sysdeps search path before the generic nptl/pthread_spin_lock.c.  So it is either '#include_next <nptl/pthread_spin_lock.c>' or '#include "../../../../nptl/pthread_spin_lock.c"'.  The former looks less ugly than the later.

> 
> 
> Aside from the log nits, the generic additions look fine to me.  You can
> commit those as soon as one of the affected machine's maintainers agrees.
> For each machine, the changes need to be approved by that machine's
> maintainer.  I'm not the maintainer for any those machines, but I would be
> inclined to insist that each definition of SPIN_LOCK_READS_BETWEEN_CMPXCHG
> have a comment giving the machine-specific rationale for the choice of value.
> I wouldn't wait for all the machine maintainers before committing the
> generic parts and whichever machine(s) are approved first.  Each machine's
> bits can come along later as those approvals arrive.

OK.

Thanks for the review,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics