code review 7534044: runtime: fix misaligned 64-bit atomic

runtime: fix misaligned 64-bit atomic
Fixes issue 4869.
Fixes issue 5007.
Update issue 5005.

[dvyukov, 2013-03-10 12:06:24]

Hello golang-dev@googlegroups.com,

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

[atom, 2013-03-10 12:37:30]

https://codereview.appspot.com/7534044/diff/8001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/7534044/diff/8001/src/pkg/runtime/runtime.h#ne...
src/pkg/runtime/runtime.h:486: uint32 pad;			// to align ParForThread.pos for
64-bit atomic operations
In my opinion, it would be conceptually simpler to use two allocations in
parforalloc.

[bradfitz, 2013-03-10 15:12:49]

LGTM

On Sun, Mar 10, 2013 at 5:06 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: fix misaligned 64-bit atomic
> Fixes issue 4869.
> Fixes issue 5007.
> Update issue 5005.
>
> Please review this at
https://codereview.appspot.**com/7534044/<https://codereview.appspot.com/7534...
>
> Affected files:
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/runtime/parfor.c
>   M src/pkg/runtime/runtime.h
>
>
> Index: src/pkg/runtime/mgc0.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -1757,6 +1757,8 @@
>         // a problem in the past.
>         if((((uintptr)&work.empty) & 7) != 0)
>                 runtime·throw("runtime: gc work buffer is misaligned");
> +       if((((uintptr)&work.full) & 7) != 0)
> +               runtime·throw("runtime: gc work buffer is misaligned");
>
>         // The gc is turned off (via enablegc) until
>         // the bootstrap has completed.
> Index: src/pkg/runtime/parfor.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/parfor.c
> +++ b/src/pkg/runtime/parfor.c
> @@ -46,6 +46,7 @@
>  runtime·parforsetup(ParFor *desc, uint32 nthr, uint32 n, void *ctx, bool
> wait, void (*body)(ParFor*, uint32))
>  {
>         uint32 i, begin, end;
> +       uint64 *pos;
>
>         if(desc == nil || nthr == 0 || nthr > desc->nthrmax || body ==
> nil) {
>                 runtime·printf("desc=%p nthr=%d count=%d body=%p\n", desc,
> nthr, n, body);
> @@ -67,7 +68,10 @@
>         for(i=0; i<nthr; i++) {
>                 begin = (uint64)n*i / nthr;
>                 end = (uint64)n*(i+1) / nthr;
> -               desc->thr[i].pos = (uint64)begin | (((uint64)end)<<32);
> +               pos = &desc->thr[i].pos;
> +               if(((uintptr)pos & 7) != 0)
> +                       runtime·throw("parforsetup: pos is not aligned");
> +               *pos = (uint64)begin | (((uint64)end)<<32);
>         }
>  }
>
> Index: src/pkg/runtime/runtime.h
> ==============================**==============================**=======
> --- a/src/pkg/runtime/runtime.h
> +++ b/src/pkg/runtime/runtime.h
> @@ -483,6 +483,7 @@
>         bool wait;                      // if true, wait while all threads
> finish processing,
>                                         // otherwise parfor may return
> while other threads are still working
>         ParForThread *thr;              // array of thread descriptors
> +       uint32 pad;                     // to align ParForThread.pos for
> 64-bit atomic operations
>         // stats
>         uint64 nsteal;
>         uint64 nstealcnt;
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dvyukov, 2013-03-10 16:46:22]

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

runtime: fix misaligned 64-bit atomic
Fixes issue 4869.
Fixes issue 5007.
Update issue 5005.

R=golang-dev, 0xe2.0x9a.0x9b, bradfitz
CC=golang-dev
https://codereview.appspot.com/7534044

[dvyukov, 2013-03-10 16:51:02]

https://codereview.appspot.com/7534044/diff/8001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/7534044/diff/8001/src/pkg/runtime/runtime.h#ne...
src/pkg/runtime/runtime.h:486: uint32 pad;			// to align ParForThread.pos for
64-bit atomic operations
On 2013/03/10 12:37:30, atom wrote:
> In my opinion, it would be conceptually simpler to use two allocations in
> parforalloc.

I don't have a strong opinion here. But I want to fix it finally. And since I've
added the runtime check for alignment, even if this is fragile we should notice
the bug quickly.

[dave_cheney.net, 2013-03-11 00:04:33]

Thank you



On 11/03/2013, at 2:12 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> LGTM
> 
> On Sun, Mar 10, 2013 at 5:06 AM, <dvyukov@google.com> wrote:
>> Reviewers: golang-dev1,
>> 
>> Message:
>> Hello golang-dev@googlegroups.com,
>> 
>> I'd like you to review this change to
>> https://dvyukov%40google.com@code.google.com/p/go/
>> 
>> 
>> Description:
>> runtime: fix misaligned 64-bit atomic
>> Fixes issue 4869.
>> Fixes issue 5007.
>> Update issue 5005.
>> 
>> Please review this at https://codereview.appspot.com/7534044/
>> 
>> Affected files:
>>   M src/pkg/runtime/mgc0.c
>>   M src/pkg/runtime/parfor.c
>>   M src/pkg/runtime/runtime.h
>> 
>> 
>> Index: src/pkg/runtime/mgc0.c
>> ===================================================================
>> --- a/src/pkg/runtime/mgc0.c
>> +++ b/src/pkg/runtime/mgc0.c
>> @@ -1757,6 +1757,8 @@
>>         // a problem in the past.
>>         if((((uintptr)&work.empty) & 7) != 0)
>>                 runtime·throw("runtime: gc work buffer is misaligned");
>> +       if((((uintptr)&work.full) & 7) != 0)
>> +               runtime·throw("runtime: gc work buffer is misaligned");
>> 
>>         // The gc is turned off (via enablegc) until
>>         // the bootstrap has completed.
>> Index: src/pkg/runtime/parfor.c
>> ===================================================================
>> --- a/src/pkg/runtime/parfor.c
>> +++ b/src/pkg/runtime/parfor.c
>> @@ -46,6 +46,7 @@
>>  runtime·parforsetup(ParFor *desc, uint32 nthr, uint32 n, void *ctx, bool
wait, void (*body)(ParFor*, uint32))
>>  {
>>         uint32 i, begin, end;
>> +       uint64 *pos;
>> 
>>         if(desc == nil || nthr == 0 || nthr > desc->nthrmax || body == nil)
{
>>                 runtime·printf("desc=%p nthr=%d count=%d body=%p\n", desc,
nthr, n, body);
>> @@ -67,7 +68,10 @@
>>         for(i=0; i<nthr; i++) {
>>                 begin = (uint64)n*i / nthr;
>>                 end = (uint64)n*(i+1) / nthr;
>> -               desc->thr[i].pos = (uint64)begin | (((uint64)end)<<32);
>> +               pos = &desc->thr[i].pos;
>> +               if(((uintptr)pos & 7) != 0)
>> +                       runtime·throw("parforsetup: pos is not aligned");
>> +               *pos = (uint64)begin | (((uint64)end)<<32);
>>         }
>>  }
>> 
>> Index: src/pkg/runtime/runtime.h
>> ===================================================================
>> --- a/src/pkg/runtime/runtime.h
>> +++ b/src/pkg/runtime/runtime.h
>> @@ -483,6 +483,7 @@
>>         bool wait;                      // if true, wait while all threads
finish processing,
>>                                         // otherwise parfor may return while
other threads are still working
>>         ParForThread *thr;              // array of thread descriptors
>> +       uint32 pad;                     // to align ParForThread.pos for
64-bit atomic operations
>>         // stats
>>         uint64 nsteal;
>>         uint64 nstealcnt;
>> 
>> 
>> -- 
>> 
>> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
> 
> -- 
>  
> --- 
> You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>  
>  

[albert.strasheim, 2013-03-11 05:34:14]

On 2013/03/10 16:46:22, dvyukov wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=9fb011139c2b ***
> runtime: fix misaligned 64-bit atomic
> Fixes issue 4869.
> Fixes issue 5007.
> Update issue 5005.

Stress test update:

Things are looking much better, but a few issues remain.

issue 5005: sporadic test failures on linux/386

issue 5025: timeouts in runtime tests that I mentioned in issue 5007

I think we have internal tests that are also hitting the same problem as issue
5025. It always includes a goroutine that seems to be hanging while doing a
system call.

Cheers

Albert