code review 11438044: [release-branch.go1.1] runtime: prevent sysmon from pol...

[release-branch.go1.1] runtime: prevent sysmon from polling network excessivly

««« CL 11569043 / 6b3c351c7fe6
runtime: prevent sysmon from polling network excessivly
If the network is not polled for 10ms, sysmon starts polling network
on every iteration (every 20us) until another thread blocks in netpoll.
Fixes  issue 5922 .

R=golang-dev, iant
CC=golang-dev
https://codereview.appspot.com/11569043
»»»

Update issue 5928

[adg, 2013-07-22 23:46:25]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-07-22 23:48:40]

LGTM



On Mon, Jul 22, 2013 at 4:46 PM, <adg@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> [release-branch.go1.1] runtime: prevent sysmon from polling network
> excessivly
>
> ««« CL 11569043 / 6b3c351c7fe6
> runtime: prevent sysmon from polling network excessivly
> If the network is not polled for 10ms, sysmon starts polling network
> on every iteration (every 20us) until another thread blocks in netpoll.
> Fixes  issue 5922 .
>
> R=golang-dev, iant
> CC=golang-dev
>
https://codereview.appspot.**com/11569043<https://codereview.appspot.com/1156...
> »»»
>
> Update issue 5928
>
> Please review this at
https://codereview.appspot.**com/11438044/<https://codereview.appspot.com/114...
>
> Affected files:
>   M src/pkg/runtime/proc.c
>
>
> Index: src/pkg/runtime/proc.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/proc.c
> +++ b/src/pkg/runtime/proc.c
> @@ -2015,6 +2015,7 @@
>                 lastpoll = runtime·atomicload64(&runtime·**
> sched.lastpoll);
>                 now = runtime·nanotime();
>                 if(lastpoll != 0 && lastpoll + 10*1000*1000 > now) {
> +                       runtime·cas64(&runtime·sched.**lastpoll,
> lastpoll, now);
>                         gp = runtime·netpoll(false);  // non-blocking
>                         injectglist(gp);
>                 }
>
>
> --
>
> ---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...
> .
>
>
>

[rsc, 2013-07-22 23:50:15]

Please wait.

[adg, 2013-07-22 23:50:37]

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

[release-branch.go1.1] runtime: prevent sysmon from polling network excessivly

««« CL 11569043 / 6b3c351c7fe6
runtime: prevent sysmon from polling network excessivly
If the network is not polled for 10ms, sysmon starts polling network
on every iteration (every 20us) until another thread blocks in netpoll.
Fixes  issue 5922 .

R=golang-dev, iant
CC=golang-dev
https://codereview.appspot.com/11569043
»»»

Update issue 5928

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/11438044

[rsc, 2013-07-22 23:50:52]

Dmitriy, are you absolutely sure this is okay? I am worried about
unintended consequences of this fix making Go 1.1.2 worse than Go 1.1.1.

Russ

[adg, 2013-07-22 23:51:54]

Sorry, just missed it. We can roll back if necessary (obviously).


On 23 July 2013 09:50, Russ Cox <rsc@golang.org> wrote:

> Please wait.
>
>

[adg, 2013-07-22 23:53:06]

I'm no familiar with the issue, but is it possible to write a good test for
it?

[iant, 2013-07-23 00:11:08]

On Mon, Jul 22, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
> Dmitriy, are you absolutely sure this is okay? I am worried about unintended
> consequences of this fix making Go 1.1.2 worse than Go 1.1.1.

I'd like to hear Dmitriy's opinion, but I'm pretty sure this patch is OK.

Ian

[dvyukov, 2013-07-23 08:44:13]

On Tue, Jul 23, 2013 at 4:11 AM, Ian Lance Taylor <iant@golang.org> wrote:
> On Mon, Jul 22, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
>> Dmitriy, are you absolutely sure this is okay? I am worried about unintended
>> consequences of this fix making Go 1.1.2 worse than Go 1.1.1.
>
> I'd like to hear Dmitriy's opinion, but I'm pretty sure this patch is OK.


Yes, this patch is OK.
It's very difficult to write a test for it. Alex Brainman reported
potential busy spinning on some version of windows on a single core
box, but we don't know how exactly it happens and how to test for it.

[dvyukov, 2013-07-23 08:45:33]

On Tue, Jul 23, 2013 at 12:43 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Jul 23, 2013 at 4:11 AM, Ian Lance Taylor <iant@golang.org> wrote:
>> On Mon, Jul 22, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
>>> Dmitriy, are you absolutely sure this is okay? I am worried about
unintended
>>> consequences of this fix making Go 1.1.2 worse than Go 1.1.1.
>>
>> I'd like to hear Dmitriy's opinion, but I'm pretty sure this patch is OK.
>
>
> Yes, this patch is OK.
> It's very difficult to write a test for it. Alex Brainman reported
> potential busy spinning on some version of windows on a single core
> box, but we don't know how exactly it happens and how to test for it.

it will be different in Go1.1.2:
-runtime·cas64(&runtime·sched.lastpoll, lastpoll, now);
+runtime·cas64(&runtime·sched.lastpoll, &lastpoll, now);

[adg, 2013-07-24 23:07:07]

That patch doesn't compile:

proc.c:2018 argument prototype mismatch "IND VLONG" for "IND UVLONG":
runtime.cas64

because lastpoll is an int64, but cas64 wants a uint64. By changing
lastpoll to uint64 it appears to work (and all.bash completes).

https://codereview.appspot.com/11796043


On 23 July 2013 18:45, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Tue, Jul 23, 2013 at 12:43 PM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> > On Tue, Jul 23, 2013 at 4:11 AM, Ian Lance Taylor <iant@golang.org>
> wrote:
> >> On Mon, Jul 22, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
> >>> Dmitriy, are you absolutely sure this is okay? I am worried about
> unintended
> >>> consequences of this fix making Go 1.1.2 worse than Go 1.1.1.
> >>
> >> I'd like to hear Dmitriy's opinion, but I'm pretty sure this patch is
> OK.
> >
> >
> > Yes, this patch is OK.
> > It's very difficult to write a test for it. Alex Brainman reported
> > potential busy spinning on some version of windows on a single core
> > box, but we don't know how exactly it happens and how to test for it.
>
> it will be different in Go1.1.2:
> -runtime·cas64(&runtime·sched.lastpoll, lastpoll, now);
> +runtime·cas64(&runtime·sched.lastpoll, &lastpoll, now);
>