From mboxrd@z Thu Jan 1 00:00:00 1970 From: Atsushi Nakagawa Subject: Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER Date: Tue, 25 Oct 2011 20:05:19 -0700 (PDT) Message-ID: <16520370.401.1319598319120.JavaMail.geo-discussion-forums@prms22> References: <1319554509-6532-1-git-send-email-kusmabite@gmail.com> Reply-To: msysgit@googlegroups.com Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_400_8158501.1319598319117" Cc: git@vger.kernel.org, johannes.schindelin@gmx.de, j.sixt@viscovery.net To: msysgit@googlegroups.com X-From: msysgit+bncCL3AtdjPFhDK_p71BBoEVZbnlg@googlegroups.com Wed Oct 26 10:05:39 2011 Return-path: Envelope-to: gcvm-msysgit@m.gmane.org Received: from mail-gy0-f186.google.com ([209.85.160.186]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1RIyUU-0003SU-Bo for gcvm-msysgit@m.gmane.org; Wed, 26 Oct 2011 10:05:38 +0200 Received: by gyd8 with SMTP id 8sf2638864gyd.3 for ; Wed, 26 Oct 2011 01:05:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlegroups.com; s=beta; h=x-beenthere:date:from:reply-to:to:cc:message-id:in-reply-to :references:subject:mime-version:x-original-sender:precedence :mailing-list:list-id:x-google-group-id:list-post:list-help :list-archive:sender:list-subscribe:list-unsubscribe:content-type; bh=6pJ4sZHYA32MPYS1YQO1l0QvzxuBLpj4srWd5MHLwAo=; b=p8rZKfaMbQhQXbF2Ys6FW2Wyc14SYq45vpe0FXQEXyrrkg6iS5Mrb2kZZiJmT7Tb4O CyNABRYM2eT66R5PaGCH4yymGn8FcpRsDVGTJoCx0/a+y3huzCjI6vE7bR9Dgo+u1BCZ cIHIxORej9JmEJS2hZt58055KsCW6Dyu6xpHM= Received: by 10.236.161.34 with SMTP id v22mr8211314yhk.9.1319616330145; Wed, 26 Oct 2011 01:05:30 -0700 (PDT) X-BeenThere: msysgit@googlegroups.com Received: by 10.101.2.12 with SMTP id e12ls1085080ani.3.gmail; Wed, 26 Oct 2011 01:05:29 -0700 (PDT) Received: by 10.100.236.38 with SMTP id j38mr4320888anh.42.1319616329307; Wed, 26 Oct 2011 01:05:29 -0700 (PDT) Received: by 10.150.167.20 with SMTP id p20msybe; Tue, 25 Oct 2011 20:05:20 -0700 (PDT) Received: by 10.100.19.8 with SMTP id 8mr1833173ans.26.1319598319807; Tue, 25 Oct 2011 20:05:19 -0700 (PDT) In-Reply-To: <1319554509-6532-1-git-send-email-kusmabite@gmail.com> X-Original-Sender: atnak@chejz.com Precedence: list Mailing-list: list msysgit@googlegroups.com; contact msysgit+owners@googlegroups.com List-ID: X-Google-Group-Id: 152234828034 List-Post: , List-Help: , List-Archive: Sender: msysgit@googlegroups.com List-Subscribe: , List-Unsubscribe: , Archived-At: ------=_Part_400_8158501.1319598319117 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On Oct 25, 11:55 pm, Erik Faye-Lund wrote: > [...] > +int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *attr) > +{ > + InitializeCriticalSection(&mutex->cs); > + mutex->autoinit = 0; > + return 0; > +} > + > +int pthread_mutex_lock(pthread_mutex_t *mutex) > +{ > + if (mutex->autoinit) { > + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) { I'm making the assumption that mutex->autoinit starts off as 1 before things get multi-threaded.. I've only looked at what's in the patch so I could be missing vital context.. Anyways, is there a reason why you made this "InterlockedCompareExchange(..., -1, 1) != -1" and not "InterlockedCompareExchange(..., -1, 1) == 1"? It looks to me the former adds a race condition after "if (mutex->autoinit) {". e.g. A second thread could reinitialize mutex->cs after the first thread has already entered EnterCriticalSection(...). > + pthread_mutex_init(mutex, NULL); > + mutex->autoinit = 0; > + } else > + while (mutex->autoinit != 0) > + ; /* wait for other thread */ > + } > + > + EnterCriticalSection(&mutex->cs); > + return 0; > +} > [...] -- Atsushi Nakagawa ------=_Part_400_8158501.1319598319117 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Oct 25, 11:55 pm, Erik Faye-Lund <kusmab...@gmail.com> w= rote:
> [...]
> +int pthread_mutex_init(pthre= ad_mutex_t *mutex, const pthread_mutexattr_t *attr)
> +{
=
> +       InitializeCriticalSection(&mutex->c= s);
> +       mutex->autoinit =3D 0;
> +       return 0;
> +}
> +=
> +int pthread_mutex_lock(pthread_mutex_t *mutex)
&= gt; +{
> +       if (mutex->autoinit) {
> +               if (Interlock= edCompareExchange(&mutex->autoinit, -1, 1) !=3D -1) {

=
I'm making the assumption that mutex->autoinit starts off as = 1 before things get multi-threaded..

I've only loo= ked at what's in the patch so I could be missing vital context..  Anyw= ays, is there a reason why you made this "InterlockedCompareExchange(..., -= 1, 1) !=3D -1" and not "InterlockedCompareExchange(..., -1, 1) =3D=3D 1"?

It looks to me the former adds a race condition aft= er "if (mutex->autoinit) {".  e.g. A second thread could reinitiali= ze mutex->cs after the first thread has already entered EnterCriticalSec= tion(...).

> +         &nbs= p;             pthread_mutex_init(mutex, NULL= );
> +                =       mutex->autoinit =3D 0;
> +   &nb= sp;           } else
> +    = ;                   while (mut= ex->autoinit !=3D 0)
> +          =                     ; /*= wait for other thread */
> +       }
> +
> +       EnterCriticalSection(&mut= ex->cs);
> +       return 0;
> = +}
> [...]

-- 
Atsushi= Nakagawa

------=_Part_400_8158501.1319598319117--