From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21387 invoked by alias); 25 Jul 2012 18:13:15 -0000 Received: (qmail 21362 invoked by uid 22791); 25 Jul 2012 18:13:14 -0000 X-SWARE-Spam-Status: No, hits=-2.9 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from toast.topped-with-meat.com (HELO topped-with-meat.com) (204.197.218.159) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 25 Jul 2012 18:13:02 +0000 Received: by topped-with-meat.com (Postfix, from userid 5281) id DD1812C0B5; Wed, 25 Jul 2012 11:13:00 -0700 (PDT) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit From: Roland McGrath To: Maxim Kuvyrkov Cc: Andrew Haley , David Miller , , , , Subject: Re: [PATCH] Unify pthread_spin_[try]lock implementations. In-Reply-To: Maxim Kuvyrkov's message of Thursday, 12 July 2012 10:16:32 +1200 <7FBB4F87-9FF3-4239-818F-5A38C8094011@codesourcery.com> References: <65B470D2-4D01-4BA1-AEC5-A72C0006EA22@codesourcery.com> <20120711081441.73BB22C093@topped-with-meat.com> <20120711.012509.1325789838255235021.davem@davemloft.net> <4FFD3CD9.4030206@redhat.com> <84304C03-6A49-4263-9016-05486EDC0E98@codesourcery.com> <4FFD4114.9000806@redhat.com> <20120711112235.B28CA2C099@topped-with-meat.com> <7FBB4F87-9FF3-4239-818F-5A38C8094011@codesourcery.com> Message-Id: <20120725181300.DD1812C0B5@topped-with-meat.com> Date: Wed, 25 Jul 2012 18:13:00 -0000 X-CMAE-Score: 0 X-CMAE-Analysis: v=2.0 cv=e8d9udV/ c=1 sm=1 a=IOX5nZC-PoQA:10 a=Z6MIti7PxpgA:10 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=14OXPxybAAAA:8 a=ISAAncdZkxMjBOQHybIA:9 a=CjuIK1q_8ugA:10 a=WkljmVdYkabdwxfqvArNOQ==:117 X-IsSubscribed: yes Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org X-SW-Source: 2012-07/txt/msg00051.txt.bz2 > 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 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