code review 8699043: runtime: use UMTX_OP_WAIT_UINT on FreeBSD

runtime: use UMTX_OP_WAIT_UINT on FreeBSD

UMTX_OP_WAIT expects that the address points to a uintptr, but
the code in lock_futex.c uses a uint32.  UMTX_OP_WAIT_UINT is
just like UMTX_OP_WAIT, but the address points to a uint32.
This almost certainly makes no difference on a little-endian
system, but since the kernel supports it we should do the
right thing.  And, who knows, maybe it matters.

[iant, 2013-04-12 03:31:39]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[iant, 2013-04-12 03:31:54]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-04-12 03:52:29]

LGTM

But docs on the web about this are surprisingly hard to find. Random commits
(https://code.google.com/p/freebsd/source/detail?r=7a73b98d73c3bd9e7158087d3f7...)
and such. But seems plausible. :)

[iant, 2013-04-12 03:54:51]

On 2013/04/12 03:52:29, bradfitz wrote:
> LGTM
> 
> But docs on the web about this are surprisingly hard to find. Random commits
>
(https://code.google.com/p/freebsd/source/detail?r=7a73b98d73c3bd9e7158087d3f7...)
> and such. But seems plausible. :)

Yeah, I couldn't find any docs.  I actually found it by reading the kernel
source while I was trying to verify that the Go runtime code was doing the right
thing.

[bradfitz, 2013-04-12 03:56:03]

Yeah, I'm reading through http://fxr.watson.org/fxr/source/kern/kern_umtx.c

Guess you don't need docs when you ship your userspace, libc, and kernel
all together.



On Thu, Apr 11, 2013 at 8:54 PM, <iant@golang.org> wrote:

> On 2013/04/12 03:52:29, bradfitz wrote:
>
>> LGTM
>>
>
>  But docs on the web about this are surprisingly hard to find. Random
>>
> commits
>
> (https://code.google.com/p/**freebsd/source/detail?r=**
>
7a73b98d73c3bd9e7158087d3f7c88**0ba9d97da6#<https://code.google.com/p/freebsd/source/detail?r=7a73b98d73c3bd9e7158087d3f7c880ba9d97da6#>
> )
>
>> and such. But seems plausible. :)
>>
>
> Yeah, I couldn't find any docs.  I actually found it by reading the
> kernel source while I was trying to verify that the Go runtime code was
> doing the right thing.
>
>
https://codereview.appspot.**com/8699043/<https://codereview.appspot.com/8699...
>

[r, 2013-04-12 05:22:58]

LGTM
that kernel code is remarkable.

[ality, 2013-04-12 10:55:45]

r@golang.org once said:
> that kernel code is remarkable.

I can think of a few other adjectives.

  Anthony

[iant, 2013-04-12 12:20:23]

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

runtime: use UMTX_OP_WAIT_UINT on FreeBSD

UMTX_OP_WAIT expects that the address points to a uintptr, but
the code in lock_futex.c uses a uint32.  UMTX_OP_WAIT_UINT is
just like UMTX_OP_WAIT, but the address points to a uint32.
This almost certainly makes no difference on a little-endian
system, but since the kernel supports it we should do the
right thing.  And, who knows, maybe it matters.

R=golang-dev, bradfitz, r, ality
CC=golang-dev
https://codereview.appspot.com/8699043

[Phil Pennock, 2013-06-14 22:33:47]

On 2013/04/12 12:20:23, iant wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=64bd7ce0c817 ***
> 
> runtime: use UMTX_OP_WAIT_UINT on FreeBSD
> 
> UMTX_OP_WAIT expects that the address points to a uintptr, but
> the code in lock_futex.c uses a uint32.  UMTX_OP_WAIT_UINT is
> just like UMTX_OP_WAIT, but the address points to a uint32.
> This almost certainly makes no difference on a little-endian
> system, but since the kernel supports it we should do the
> right thing.  And, who knows, maybe it matters.

As a side-note: this caused a severe performance regression on an old,
unsupported, release of FreeBSD (7.0 plus security patches), such that running
"go version" would take 60 seconds to complete (or 5 seconds under ktrace).  It
appears, from kdump, that the code enters a tight loop with 441,722 instances of
"_umtx_op -1 errno 22 Invalid argument" in the output, and the same parameters
passed every time.

Since the code supporting UMTX_OP_WAIT_UINT went into FreeBSD in 2007 (
http://svnweb.freebsd.org/base?view=revision&revision=173800 ), I suspect that
leaving this change in Go is correct, but perhaps there could be some better
error detection?

Reverting this change resolved the issues for me.

[iant, 2013-06-14 23:17:46]

On Fri, Jun 14, 2013 at 3:33 PM,  <syscomet@gmail.com> wrote:
>
> As a side-note: this caused a severe performance regression on an old,
> unsupported, release of FreeBSD (7.0 plus security patches), such that
> running "go version" would take 60 seconds to complete (or 5 seconds
> under ktrace).  It appears, from kdump, that the code enters a tight
> loop with 441,722 instances of "_umtx_op -1 errno 22 Invalid argument"
> in the output, and the same parameters passed every time.
>
> Since the code supporting UMTX_OP_WAIT_UINT went into FreeBSD in 2007 (
> http://svnweb.freebsd.org/base?view=revision&revision=173800 ), I
> suspect that leaving this change in Go is correct, but perhaps there
> could be some better error detection?
>
> Reverting this change resolved the issues for me.

The code is os_freebsd.c is intended to crash if the system call
fails.  It looks correct to me but I guess it isn't.  I don't think we
care about a version of FreeBSD from before 2007, but if you can work
up a patch that detects the error correctly--i.e., crashes--we can
probably commit it.

Ian

[dho, 2013-06-15 02:30:22]

2013/6/14 Ian Lance Taylor <iant@golang.org>:
> On Fri, Jun 14, 2013 at 3:33 PM,  <syscomet@gmail.com> wrote:
>>
>> As a side-note: this caused a severe performance regression on an old,
>> unsupported, release of FreeBSD (7.0 plus security patches), such that
>> running "go version" would take 60 seconds to complete (or 5 seconds
>> under ktrace).  It appears, from kdump, that the code enters a tight
>> loop with 441,722 instances of "_umtx_op -1 errno 22 Invalid argument"
>> in the output, and the same parameters passed every time.
>>
>> Since the code supporting UMTX_OP_WAIT_UINT went into FreeBSD in 2007 (
>> http://svnweb.freebsd.org/base?view=revision&revision=173800 ), I
>> suspect that leaving this change in Go is correct, but perhaps there
>> could be some better error detection?
>>
>> Reverting this change resolved the issues for me.
>
> The code is os_freebsd.c is intended to crash if the system call
> fails.  It looks correct to me but I guess it isn't.  I don't think we
> care about a version of FreeBSD from before 2007, but if you can work
> up a patch that detects the error correctly--i.e., crashes--we can
> probably commit it.

I am not sure that 7 was ever explicitly supported. The original port
was developed on FreeBSD 8, and there were a few bits (pkg/net,
maybe?) that had kqueue / other system needs that weren't in 7 (if I
recall correctly -- it's possible I don't, it's been almost 4 years
now).

I'm surprised the *(int32*)1005=0x1005 doesn't cause a crash. If
that's not crashing, it would be good to figure out why ktrace shows
it returning EINVAL and that line not running. I don't have a FreeBSD
7 machine to try this out on.

--dho

> Ian
>
> --
>
> ---
> 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.
>
>

[minux1, 2013-06-15 06:14:40]

On Sat, Jun 15, 2013 at 10:30 AM, Devon H. O'Dell <devon.odell@gmail.com>wrote:

> I'm surprised the *(int32*)1005=0x1005 doesn't cause a crash. If
> that's not crashing, it would be good to figure out why ktrace shows
> it returning EINVAL and that line not running. I don't have a FreeBSD
> 7 machine to try this out on.

It's because the returned AX contains the errno value but it's positive.

Hi @syscomet, could you please patch in
https://codereview.appspot.com/10295043
and see if you got an error when executing on FreeBSD 7?

[dho, 2013-06-15 07:50:10]

2013/6/15 minux <minux.ma@gmail.com>:
>
> On Sat, Jun 15, 2013 at 10:30 AM, Devon H. O'Dell <devon.odell@gmail.com>
> wrote:
>>
>> I'm surprised the *(int32*)1005=0x1005 doesn't cause a crash. If
>> that's not crashing, it would be good to figure out why ktrace shows
>> it returning EINVAL and that line not running. I don't have a FreeBSD
>> 7 machine to try this out on.
>
> It's because the returned AX contains the errno value but it's positive.

There may be other cases then. I haven't checked, but I'm reasonably
sure that's not the only one. That means the EINTR check is bad, too.

> Hi @syscomet, could you please patch in
> https://codereview.appspot.com/10295043
> and see if you got an error when executing on FreeBSD 7?

[dho, 2013-06-15 07:50:41]

2013/6/15 Devon H. O'Dell <devon.odell@gmail.com>:
> 2013/6/15 minux <minux.ma@gmail.com>:
>>
>> On Sat, Jun 15, 2013 at 10:30 AM, Devon H. O'Dell <devon.odell@gmail.com>
>> wrote:
>>>
>>> I'm surprised the *(int32*)1005=0x1005 doesn't cause a crash. If
>>> that's not crashing, it would be good to figure out why ktrace shows
>>> it returning EINVAL and that line not running. I don't have a FreeBSD
>>> 7 machine to try this out on.
>>
>> It's because the returned AX contains the errno value but it's positive.
>
> There may be other cases then. I haven't checked, but I'm reasonably
> sure that's not the only one. That means the EINTR check is bad, too.

Actually, it means the ret >= 0 check is bad in general.

>> Hi @syscomet, could you please patch in
>> https://codereview.appspot.com/10295043
>> and see if you got an error when executing on FreeBSD 7?

[Phil Pennock, 2013-06-16 21:11:29]

On 2013/06/15 07:50:41, dho wrote:
> Actually, it means the ret >= 0 check is bad in general.
> 
> >> Hi @syscomet, could you please patch in
> >> https://codereview.appspot.com/10295043
> >> and see if you got an error when executing on FreeBSD 7?

Forgive me for silence, I've just been travelling and am now back online.

I reverted my reversion of this UMTX UINT change and then patched in that
assembler change; the Go tool-chain failed to build, with a segfault in the
relevant area of code.

Output of "./all.bash" from the failing step onwards, with traces:
  https://gist.github.com/syscomet/5793451

Let me know if we should take this discussion out of this issue to someplace
else?

[iant, 2013-06-16 22:34:41]

On Sun, Jun 16, 2013 at 2:11 PM,  <syscomet@gmail.com> wrote:
>
> I reverted my reversion of this UMTX UINT change and then patched in
> that assembler change; the Go tool-chain failed to build, with a
> segfault in the relevant area of code.
>
> Output of "./all.bash" from the failing step onwards, with traces:
>   https://gist.github.com/syscomet/5793451

That sounds right.  It should crash.  Crashing on an unsupported
version of FreeBSD is much better than silently doing the wrong thing.

> Let me know if we should take this discussion out of this issue to
> someplace else?

Open an issue at http://code.google.com/p/go/issues .  Thanks.

Ian