code review 7448048: runtime: add network polling support into scheduler

runtime: add network polling support into scheduler
This is a part of the bigger change that moves network poller into runtime:
https://codereview.appspot.com/7326051/

[dvyukov, 2013-03-02 12:16:23]

Depends on https://codereview.appspot.com/7429048/ for xchg64

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

https://codereview.appspot.com/7448048/diff/2001/src/pkg/runtime/runtime.h#ne...
src/pkg/runtime/runtime.h:694: uint64	runtime·xchg64(uint64 volatile*, uint64);
This will go away once https://codereview.appspot.com/7429048/ is in.

[dvyukov, 2013-03-02 12:22:28]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-03-04 21:45:28]

ready for review

[bradfitz, 2013-03-04 21:50:30]

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c#newcode1003
src/pkg/runtime/proc.c:1003: gp = runtime·netpoll(0, 8);  // non-blocking
why 8?

[dvyukov, 2013-03-05 08:31:57]

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c#newcode1003
src/pkg/runtime/proc.c:1003: gp = runtime·netpoll(0, 8);  // non-blocking
On 2013/03/04 21:50:30, bradfitz wrote:
> why 8?

Good question. It used to be in slightly different place, and the idea was that
worker threads can get small pieces of work from netpoll() independently. But in
this place it won't work that way.
I've changed netpoll to accept just a single parameter - bool dontblock.
PTAL.

[mikio, 2013-03-05 09:06:27]

https://codereview.appspot.com/7448048/diff/10001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7448048/diff/10001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:8: // returns list of goroutines that become runnable
full stop?

[dvyukov, 2013-03-05 11:04:19]

What do you mean?
On Mar 5, 2013 11:06 AM, <mikioh.mikioh@gmail.com> wrote:

>
> https://codereview.appspot.**com/7448048/diff/10001/src/**
>
pkg/runtime/netpoll.c<https://codereview.appspot.com/7448048/diff/10001/src/pkg/runtime/netpoll.c>
> File src/pkg/runtime/netpoll.c (right):
>
> https://codereview.appspot.**com/7448048/diff/10001/src/**
>
pkg/runtime/netpoll.c#newcode8<https://codereview.appspot.com/7448048/diff/10001/src/pkg/runtime/netpoll.c#newcode8>
> src/pkg/runtime/netpoll.c:8: // returns list of goroutines that become
> runnable
> full stop?
>
>
https://codereview.appspot.**com/7448048/<https://codereview.appspot.com/7448...
>

[mikio, 2013-03-05 13:20:12]

> What do you mean?

I mean, please add a full stop.

[bradfitz, 2013-03-05 15:48:45]

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c#newcode1003
src/pkg/runtime/proc.c:1003: gp = runtime·netpoll(0, 8);  // non-blocking
On 2013/03/05 08:31:57, dvyukov wrote:
> On 2013/03/04 21:50:30, bradfitz wrote:
> > why 8?
> 
> Good question. It used to be in slightly different place, and the idea was
that
> worker threads can get small pieces of work from netpoll() independently. But
in
> this place it won't work that way.
> I've changed netpoll to accept just a single parameter - bool dontblock.
> PTAL.

Fewer parameters works for me. Is it common in this code to name boo parameters
starting with "dont"?  The negative reads confusingly, but I don't care much.

[dvyukov, 2013-03-07 13:15:34]

On 2013/03/05 13:20:12, mikio wrote:
> > What do you mean?
> 
> I mean, please add a full stop.

Done.

[dvyukov, 2013-03-07 13:15:52]

On 2013/03/05 15:48:45, bradfitz wrote:
> https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/7448048/diff/3/src/pkg/runtime/proc.c#newcode1003
> src/pkg/runtime/proc.c:1003: gp = runtime·netpoll(0, 8);  // non-blocking
> On 2013/03/05 08:31:57, dvyukov wrote:
> > On 2013/03/04 21:50:30, bradfitz wrote:
> > > why 8?
> > 
> > Good question. It used to be in slightly different place, and the idea was
> that
> > worker threads can get small pieces of work from netpoll() independently.
But
> in
> > this place it won't work that way.
> > I've changed netpoll to accept just a single parameter - bool dontblock.
> > PTAL.
> 
> Fewer parameters works for me. Is it common in this code to name boo
parameters
> starting with "dont"?  The negative reads confusingly, but I don't care much.

Renamed to 'bool block'

[dvyukov, 2013-03-07 13:16:46]

I've also renamed netpoll.c to netpoll0.c, because it's intended for platforms
that do not support the poller, and netpoll.c will contain common code for
platforms that do support the poller.

PTAL

[mikio, 2013-03-09 02:22:18]

LGTM

[rsc, 2013-03-09 04:30:09]

I'll take a look over the weekend.

[dvyukov, 2013-03-12 08:13:01]

On 2013/03/09 04:30:09, rsc wrote:
> I'll take a look over the weekend.

ping
this holds another big piece of changes for soaking

[rsc, 2013-03-12 16:03:45]

LGTM

https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c
File src/pkg/runtime/netpoll0.c (right):

https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c#...
src/pkg/runtime/netpoll0.c:12: runtime·netpoll(bool block)
Can you perhaps call this file netpoll_stub.c?
The 0 suffix is a bit magical (I already regret mgc0.c and that's a completely
different 0).

https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:1015: if (gp) {
s/if (/if(/

https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:1075: if (runtime·xchg64(&runtime·sched.lastpoll, 0) !=
0) {
s/if (/if(/

[dvyukov, 2013-03-12 16:22:19]

On Tue, Mar 12, 2013 at 8:03 PM,  <rsc@golang.org> wrote:
> LGTM
>
>
>
> https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c
> File src/pkg/runtime/netpoll0.c (right):
>
>
https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c#...
> src/pkg/runtime/netpoll0.c:12: runtime·netpoll(bool block)
> Can you perhaps call this file netpoll_stub.c?

will do

> The 0 suffix is a bit magical (I already regret mgc0.c and that's a
> completely different 0).

and what is that 0? :)


> https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
>
>
https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:1015: if (gp) {
> s/if (/if(/
>
>
https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:1075: if (runtime·xchg64(&runtime·sched.lastpoll,
> 0) != 0) {
> s/if (/if(/
>
> https://codereview.appspot.com/7448048/

[rsc, 2013-03-12 16:28:00]

On Tue, Mar 12, 2013 at 12:22 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Tue, Mar 12, 2013 at 8:03 PM,  <rsc@golang.org> wrote:
> > LGTM
> >
> >
> >
> >
> https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c
> > File src/pkg/runtime/netpoll0.c (right):
> >
> >
>
https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c#...
> > src/pkg/runtime/netpoll0.c:12: runtime·netpoll(bool block)
> > Can you perhaps call this file netpoll_stub.c?
>
> will do
>
> > The 0 suffix is a bit magical (I already regret mgc0.c and that's a
> > completely different 0).
>
> and what is that 0? :)
>

It was step 0 of my multi-step plan for writing a garbage collector. I
never changed the file name as the work progressed (and the work went off
in a different direction anyway).

[dvyukov, 2013-03-12 17:14:12]

On 2013/03/12 16:28:00, rsc wrote:
> On Tue, Mar 12, 2013 at 12:22 PM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > On Tue, Mar 12, 2013 at 8:03 PM,  <mailto:rsc@golang.org> wrote:
> > > LGTM
> > >
> > >
> > >
> > >
> > https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c
> > > File src/pkg/runtime/netpoll0.c (right):
> > >
> > >
> >
>
https://codereview.appspot.com/7448048/diff/31001/src/pkg/runtime/netpoll0.c#...
> > > src/pkg/runtime/netpoll0.c:12: runtime·netpoll(bool block)
> > > Can you perhaps call this file netpoll_stub.c?
> >
> > will do
> >
> > > The 0 suffix is a bit magical (I already regret mgc0.c and that's a
> > > completely different 0).
> >
> > and what is that 0? :)
> >
> 
> It was step 0 of my multi-step plan for writing a garbage collector. I
> never changed the file name as the work progressed (and the work went off
> in a different direction anyway).

All done

[dvyukov, 2013-03-12 17:15:40]

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

runtime: add network polling support into scheduler
This is a part of the bigger change that moves network poller into runtime:
https://codereview.appspot.com/7326051/

R=golang-dev, bradfitz, mikioh.mikioh, rsc
CC=golang-dev
https://codereview.appspot.com/7448048

[atom, 2013-03-14 08:56:13]

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

https://codereview.appspot.com/7448048/diff/45001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:972: UseSpanType = 0,
Should be reverted to UseSpanType = 1.

[dvyukov, 2013-03-14 09:48:09]

oops

On Thu, Mar 14, 2013 at 12:56 PM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
>
> https://codereview.appspot.com/7448048/diff/45001/src/pkg/runtime/runtime.h
> File src/pkg/runtime/runtime.h (right):
>
>
https://codereview.appspot.com/7448048/diff/45001/src/pkg/runtime/runtime.h#n...
> src/pkg/runtime/runtime.h:972: UseSpanType = 0,
> Should be reverted to UseSpanType = 1.
>
> https://codereview.appspot.com/7448048/