code review 75960043: runtime: fix yet another race in bgsweep

runtime: fix yet another race in bgsweep
Currently it's possible that bgsweep finishes before all spans
have been swept (we only know that sweeping of all spans has *started*).
In such case bgsweep may fail wake up runfinq goroutine when it needs to.
finq may still be nil at this point, but some finalizers may be queued later.
Make bgsweep to wait for sweeping to *complete*, then it can decide
whether it needs to wake up runfinq for sure.
Update issue 7533

[dvyukov, 2014-03-14 09:14:35]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),

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

[dvyukov, 2014-03-14 09:19:33]

This is somewhat uglier than I would like to.

One constraint on the fix is that on-demand sweep can't wake up runfinq (it's
called from mallocgc, and runtime.ready/newg allocate).

Another possible solution is to count span sweep completions. Then bgsweep can
wait for nspancompleted==nspan. But that once again will be spinning (on-demand
sweep can't wake up goroutines). And it will introduce another contended atomic
RMW (there can easily be 100'000 spans and sweeping of spans with large objects
can be very fast).

[rsc, 2014-03-14 18:24:32]

Can we create the finq goroutine during the first call to runtime.SetFinalizer?
Then runfing would only ever need to call ready, which I believe does not
allocate.

[dvyukov, 2014-03-17 11:06:19]

On 2014/03/14 18:24:32, rsc wrote:
> Can we create the finq goroutine during the first call to
runtime.SetFinalizer?
> Then runfing would only ever need to call ready, which I believe does not
> allocate.

ready can allocate to create a new M
but I like this idea and creating new M is not strictly necessary for finalizers

[dvyukov, 2014-03-17 11:08:20]

On 2014/03/17 11:06:19, dvyukov wrote:
> On 2014/03/14 18:24:32, rsc wrote:
> > Can we create the finq goroutine during the first call to
> runtime.SetFinalizer?
> > Then runfing would only ever need to call ready, which I believe does not
> > allocate.
> 
> ready can allocate to create a new M
> but I like this idea and creating new M is not strictly necessary for
finalizers

no, this does not work
if we ever allocate under schedlock, then that allocation can cause MSpan_Sweep
-> wakefing -> ready(fing)
to execute ready(fing) we need schedlock (because now local queues bounded, so
on overflow we need schedlock to put into global run queue).

[dvyukov, 2014-03-17 12:41:10]

On 2014/03/17 11:08:20, dvyukov wrote:
> On 2014/03/17 11:06:19, dvyukov wrote:
> > On 2014/03/14 18:24:32, rsc wrote:
> > > Can we create the finq goroutine during the first call to
> > runtime.SetFinalizer?
> > > Then runfing would only ever need to call ready, which I believe does not
> > > allocate.
> > 
> > ready can allocate to create a new M
> > but I like this idea and creating new M is not strictly necessary for
> finalizers
> 
> no, this does not work
> if we ever allocate under schedlock, then that allocation can cause
MSpan_Sweep
> -> wakefing -> ready(fing)
> to execute ready(fing) we need schedlock (because now local queues bounded, so
> on overflow we need schedlock to put into global run queue).


OK, PTAL
this become a quite complex change, and I can't say that I 100% trust it

I had to split gclock into gclock/finlock, because otherwise the following
deadlock is possible (it's possible today on tip):

wakefing (locks gclock) -> ready(fing) -> allocate new M -> mallocgc ->
MSpan_Sweep -> queuefinalizer (locks gclock again)

With this change we can allocate while holding gclock again (because mallocgc
can lock finlock at worst, but not gclock), thus the deadlock is resolved.
Also waking of fing becomes merely a write to a bool flag (fingwake), so we can
do in during span sweep.

[rsc, 2014-03-24 19:04:00]

I think this is pretty clean.

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:2613: runtime·lock(&gclock);
Can this be finlock? If not, please comment why not.

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

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:1169: if(runtime·fingwait && runtime·fingwake && (gp =
runtime·wakefing()) != nil)
Can you make these two bools bits in an atomic word and use an atomic load here?
I am worried about ARM.

[dvyukov, 2014-03-25 20:12:04]

PTAL

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:2613: runtime·lock(&gclock);
On 2014/03/24 19:04:00, rsc wrote:
> Can this be finlock? If not, please comment why not.

Done.

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

https://codereview.appspot.com/75960043/diff/180005/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:1169: if(runtime·fingwait && runtime·fingwake && (gp =
runtime·wakefing()) != nil)
On 2014/03/24 19:04:00, rsc wrote:
> Can you make these two bools bits in an atomic word and use an atomic load
here?
> I am worried about ARM.

It does not require any memory barriers or something.
At least the thread that sets these flags will see them here when it reaches
this point. This is enough to guarantee that fing will be eventually scheduled.

[dvyukov, 2014-03-25 20:12:39]

It does not fix sync.Pool finalizer test. There must be something else.

[rsc, 2014-03-25 21:01:10]

LGTM

Please submit even though it doesn't completely fix the problem. Sometimes the
problem is a leak due to conservative collection of one word or another. Once
the heap dump is in I will change the sync.Pool test to write a heap dump on
failure, and then we can fetch the dump from the builder to figure out what went
wrong.

Regarding the arm, okay, submit without the atomics, but I'm not 100% sure we
won't need to add them later.

[dvyukov, 2014-03-26 11:06:39]

On 2014/03/25 21:01:10, rsc wrote:
> LGTM
> 
> Please submit even though it doesn't completely fix the problem. Sometimes the
> problem is a leak due to conservative collection of one word or another.


Yeah, it seems to be the case.
I've checked state of GC/fing/bgsweep when the test fails, and it all looks OK
-- all spans are swept and finalizer queue is empty. It seems that the heap
block is not collected at all.


>  Once
> the heap dump is in I will change the sync.Pool test to write a heap dump on
> failure, and then we can fetch the dump from the builder to figure out what
went
> wrong.

[dvyukov, 2014-03-26 11:11:45]

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

runtime: fix yet another race in bgsweep
Currently it's possible that bgsweep finishes before all spans
have been swept (we only know that sweeping of all spans has *started*).
In such case bgsweep may fail wake up runfinq goroutine when it needs to.
finq may still be nil at this point, but some finalizers may be queued later.
Make bgsweep to wait for sweeping to *complete*, then it can decide
whether it needs to wake up runfinq for sure.
Update issue 7533

LGTM=rsc
R=rsc
CC=golang-codereviews
https://codereview.appspot.com/75960043

[gobot, 2014-03-26 12:34:39]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/933738562c735ee5877ac5b908cc0fe7a5a80fe7