code review 52090045: runtime: fix data race in GC

runtime: fix data race in GC
Fixes issue 5139.
Update issue 7065.

[dvyukov, 2014-01-14 16:05:28]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2014-01-14 16:50:41]

LGTM
 On Jan 14, 2014 8:05 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: fix data race in GC
> Fixes issue 5139.
> Update issue 7065.
>
> Please review this at https://codereview.appspot.com/52090045/
>
> Affected files (+4, -1 lines):
>   M src/pkg/runtime/mgc0.c
>
>
> Index: src/pkg/runtime/mgc0.c
> ===================================================================
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -1956,6 +1956,8 @@
>  void
>  runtime·gchelper(void)
>  {
> +       int32 nproc;
> +
>         gchelperstart();
>
>         // parallel mark for over gc roots
> @@ -1972,7 +1974,8 @@
>
>         runtime·parfordo(work.sweepfor);
>         bufferList[m->helpgc].busy = 0;
> -       if(runtime·xadd(&work.ndone, +1) == work.nproc-1)
> +       nproc = work.nproc;  // work.nproc can change right after we
> increment work.ndone
> +       if(runtime·xadd(&work.ndone, +1) == nproc-1)
>                 runtime·notewakeup(&work.alldone);
>  }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[minux1, 2014-01-14 18:58:23]

On Tue, Jan 14, 2014 at 11:05 AM, <dvyukov@google.com> wrote:

> Description:
> runtime: fix data race in GC
> Fixes issue 5139.
> Update issue 7065.
>
remove the period of the Update issue line.

could somebody complain of the strictness of the update command to the
appropriate people?

[dvyukov, 2014-01-15 15:38:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=302bdb5b08b1 ***

runtime: fix data race in GC
Fixes issue 5139.
Update issue 7065.

R=golang-codereviews, bradfitz, minux.ma
CC=golang-codereviews
https://codereview.appspot.com/52090045

[dvyukov, 2014-01-15 15:40:01]

On 2014/01/14 18:58:23, minux wrote:
> On Tue, Jan 14, 2014 at 11:05 AM, <mailto:dvyukov@google.com> wrote:
> 
> > Description:
> > runtime: fix data race in GC
> > Fixes issue 5139.
> > Update issue 7065.
> >
> remove the period of the Update issue line.

Damn, I forgot to do it
will update manually

[dave_cheney.net, 2014-01-16 00:45:10]

Dmitry, do you think this small patch warrants being backported to 1.2 ?

[dvyukov, 2014-01-16 03:46:53]

yes, I've marked the issue release-1.2.1

On Thu, Jan 16, 2014 at 4:45 AM,  <dave@cheney.net> wrote:
> Dmitry, do you think this small patch warrants being backported to 1.2 ?
>
> https://codereview.appspot.com/52090045/

[dvyukov, 2014-01-16 08:42:23]

On Thu, Jan 16, 2014 at 12:16 PM,  <tracey.brendan@gmail.com> wrote:
> Congrats on finally finding it!

Thanks!

It's a pity that ThreadSanitizer (our race detector for C/C++) does
not work for runtime code, it would find it at once...


> On Wednesday, January 15, 2014 7:46:32 PM UTC-8, Dmitry Vyukov wrote:
>> yes, I've marked the issue release-1.2.1
>>
>>
>>
>> On Thu, Jan 16, 2014 at 4:45 AM,  <d...@cheney.net> wrote:
>>
>> > Dmitry, do you think this small patch warrants being backported to 1.2 ?
>>
>> >
>>
>> > https://codereview.appspot.com/52090045/
>