runtime: handle windows callback on non-go thread #25575

billziss-gh · 2018-05-25T19:11:54Z

Adds an extra M in mstartm0 and accounts for it in checkdead. This allows Windows callbacks created with syscall.NewCallback and syscall.NewCallbackCDecl to be called on a non-Go thread.

Fixes #6751

googlebot · 2018-05-25T19:12:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

billziss-gh · 2018-05-25T19:16:39Z

I signed it!

googlebot · 2018-05-25T19:16:42Z

CLAs look good, thanks!

alexbrainman · 2018-05-26T03:58:51Z

@bradfitz can you, please, check to see why Gerrit CL has not been created for this PR.

@billziss-gh I just submitted my change https://go-review.googlesource.com/c/go/+/114755 with the test. Could you, please, incorporate into your PR now?

Thank you.

Alex

bradfitz · 2018-05-26T04:24:43Z

@andybons owns the Gerritbot. Andy?

alexbrainman · 2018-05-26T04:30:48Z

Lucky Andy. Thank you, Brad.

Alex

billziss-gh · 2018-05-26T04:54:42Z

@alexbrainman thanks for pushing this forward so fast. I am unclear what you want me to do though. Do you want me to pull your latest commit into my fork so that it is included in this PR?

alexbrainman · 2018-05-26T05:41:28Z

Yes, please include my change in your PR. And remove test line with t.Skip in it, so we can actually verify your change works. Thank you.

Alex

billziss-gh · 2018-05-26T05:49:21Z

Alex, will do. Thanks.

alexbrainman · 2018-05-26T05:54:36Z

Once your change is accepted and submitted it will go to the top of master as single commit. So feel free to rebase and sqwash your changes away.

Alex

billziss-gh · 2018-05-26T06:02:36Z

Understood. Thanks!

billziss-gh · 2018-05-26T06:56:32Z

Ok. Rebased, squashed and pushed -f. Somehow I didn't break anything (I think). Beginners luck!

alexbrainman · 2018-05-26T07:27:09Z

Rebased, squashed and pushed -f

Cool. We will have to wait for Andy now to fix Gobot (that should push your change into our Gerrit). Then others can review and submit your code.

Thank you.

Alex

gopherbot · 2018-05-26T23:05:51Z

This PR (HEAD: 36a8ac8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/114802 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-26T23:06:25Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

andybons · 2018-05-26T23:43:33Z

Lucky me indeed :) Thanks for your patience on this!

alexbrainman · 2018-05-26T23:46:36Z

Lucky me indeed :) Thanks for your patience on this!

Thank you for fixing this. On your weekend.

Alex

gopherbot · 2018-05-26T23:47:22Z

Message from Alex Brainman:

Patch Set 1: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-26T23:47:23Z

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=33e52a95

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-26T23:56:08Z

Message from Gobot Gobot:

Patch Set 1: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-27T00:01:29Z

Message from Alex Brainman:

Patch Set 1: Code-Review+1

I am sure Austin wants to see this.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-27T07:03:15Z

Message from Ian Lance Taylor:

Patch Set 1:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-28T06:39:40Z

Message from Bill Zissimopoulos:

Patch Set 1:

It's not obvious to me that this is always correct. This test means that an extra M was created. At the time that we are executing this code, though, that M may be in use, and we may not have created another extra M yet (see the comment for the needm function in runtime/proc.go). In that case I think we might incorrectly report a deadlock. I'm not sure, though.

Disclaimer: I have a very superficial understanding of the Go runtime, so my analysis may be incomplete or plain wrong.

Lets first rewrite the crucial checkdead expression by replacing mcount():

run := sched.mnext - sched.nmfreed - sched.nmidle - sched.nmidlelocked - sched.nmsys

I note that all variables may only be updated with sched.lock acquired. Furthermore:

sched.mnext: counts the total number of m's ever created and is only updated by mcommoninit, which is only called by schedinit and allocm.
sched.nmfreed: counts the total number of freed m's and is only ever updated by mexit.
sched.nmidle: counts the number of idle m's and is only ever incremented by mput which is called by stopm.
sched.nmidlelocked: counts the number of locked idle m's and is only updated by incidlelocked. The most significant usage appears to be in stoplockedm.
sched.nmsys: counts the number of system m's (templateThread and sysmon) and is not frequently updated.

Now for an analysis of needm, which is called from cgocallback_gofunc and signal_unix:

In the signal_unix case the m is acquired and immediately dropped without the sched.lock ever acquired (and in any case it will not run on Windows).
In the cgocallback_gofunc case we get a call to cgocallbackg and then to cgocallbackg1. Immediately upon caling cgocallbackg1 we create an extra m with newextram / oneNewExtraM / allocm. Allocm will call mcommoninit and increment sched.mnext.

The danger case is any of the minus variables getting incremented between the call to needm / lockextra and allocm / mcommonit. In my analysis there appears only one point where this can happen: exitsyscall called from cgocallbackg.

Exitsyscall first calls exitsyscallfast and if that succeeds it returns immediately (without touching any of the sched variables of interest). Therefore checkdead will not fail in this case.

If exitsyscallfast fails, exitsyscall calls exitsyscall0, which may call stoplockedm or stopm, which will increment the nmidlelocked or nmidle variables (I believe stoplockedm will be called in the examined case because of the lockOSThread call in cgocallbackg). However it is important to note that the exitsyscallfast failure means that there are other running m's in the system, which is why we have to go through the scheduler in exitsyscall0. So checkdead will not fail in this case either.

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

billziss-gh · 2018-05-29T21:13:12Z

@alexbrainman thank you for your help with the process so far.

I am wondering if I have misunderstood the process and there is something I need to do at this point or we are simply waiting for all reviewers to respond.

alexbrainman · 2018-05-30T12:09:07Z

I am wondering if I have misunderstood the process and there is something I need to do at this point or we are simply waiting for all reviewers to respond.

@billziss-gh @ianlancetaylor made 3 comments that you need to attend. He suggested you add comment here

https://go-review.googlesource.com/c/go/+/114802/1/src/runtime/proc.go#1296

and here

https://go-review.googlesource.com/c/go/+/114802/1/src/runtime/proc.go#1621

You should add comments and push new version (with the comments).

Ian also questioned correctness of your change here

https://go-review.googlesource.com/c/go/+/114802/1/src/runtime/proc.go#4220

and I see that you replied to his questions.

So maybe add comments as requested and push the change, or explain why you think comments are not needed.

Anyway https://go-review.googlesource.com/c/go/+/114802 has only patch set number 1, once you pushed another change, it will go to 2, and then 3 and so on. And to be sure, send PTAL to confirm once you want some action from reviewers.

And do not hesitate pinging us on this PR or on https://go-review.googlesource.com/c/go/+/114802 if you need some help.

Alex

- add comments as per reviewer

gopherbot · 2018-06-03T05:05:09Z

This PR (HEAD: 51f9bd2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/114802 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

billziss-gh · 2018-06-03T05:10:52Z

@alexbrainman sorry for the delay. I have added some comments as per the reviewer's request.

So "please take another look".

gopherbot · 2018-06-03T08:33:32Z

Message from Alex Brainman:

Patch Set 2:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

- additional comments as per reviewer

gopherbot · 2018-06-03T17:25:34Z

This PR (HEAD: d429e3e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/114802 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

billziss-gh · 2018-06-03T17:29:25Z

Thank you. Per your instructions I have added the additional comment: "See issue #6751 for details."

billziss-gh · 2018-06-04T21:15:40Z

@alexbrainman @ianlancetaylor please take another look.

gopherbot · 2018-06-05T09:44:13Z

Message from Alex Brainman:

Patch Set 3: Code-Review+1

Ian, please, review this. I would have done it myself, but I do not trust myself with scheduler code. Thank you.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

alexbrainman · 2018-06-05T09:46:20Z

@alexbrainman @ianlancetaylor please take another look.

I just pinged Ian on https://go-review.googlesource.com/c/go/+/114802 in Gerrit. Hopefully he will get back to you when he can. You can always comment here or on Gerrit directly (you should be able to comment there and everything).

Alex

gopherbot · 2018-06-05T13:30:28Z

Message from Ian Lance Taylor:

Patch Set 3: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-06-05T13:30:43Z

Message from Gobot Gobot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b0cd0ffe

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-06-05T13:39:48Z

Message from Gobot Gobot:

Patch Set 3: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-06-05T13:51:03Z

Message from Ian Lance Taylor:

Patch Set 3: Code-Review+2

Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/114802.
After addressing review feedback, remember to publish your drafts!

Adds an extra M in mstartm0 and accounts for it in checkdead. This allows Windows callbacks created with syscall.NewCallback and syscall.NewCallbackCDecl to be called on a non-Go thread. Fixes #6751 Change-Id: I57626bc009a6370b9ca0827ab64b14b01dec39d4 GitHub-Last-Rev: d429e3e GitHub-Pull-Request: #25575 Reviewed-on: https://go-review.googlesource.com/114802 Reviewed-by: Alex Brainman <alex.brainman@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot · 2018-06-05T13:56:00Z

This PR is being closed because golang.org/cl/114802 has been merged.

billziss-gh · 2018-06-05T16:21:31Z

Thank you @alexbrainman for your help and @ianlancetaylor for your review.

alexbrainman · 2018-06-09T01:00:24Z

Thank you @billziss-gh for fixing very old issue.

Alex

googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label May 25, 2018

googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels May 25, 2018

alexbrainman mentioned this pull request May 25, 2018

runtime: windows callback on non-Go thread fails #6751

Closed

runtime: handle windows callback on non-go thread

36a8ac8

gopherbot force-pushed the master branch from e4259d6 to 6dbaf03 Compare May 31, 2018 18:23

runtime: handle windows callback on non-go thread

51f9bd2

- add comments as per reviewer

runtime: handle windows callback on non-go thread

d429e3e

- additional comments as per reviewer

billziss-gh mentioned this pull request Jun 4, 2018

Windows mount support ipfs/kubo#5003

Closed

gopherbot closed this Jun 5, 2018

runtime: handle windows callback on non-go thread #25575

runtime: handle windows callback on non-go thread #25575

Conversation

billziss-gh commented May 25, 2018

googlebot commented May 25, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

billziss-gh commented May 25, 2018

googlebot commented May 25, 2018

alexbrainman commented May 26, 2018

bradfitz commented May 26, 2018

alexbrainman commented May 26, 2018

billziss-gh commented May 26, 2018

alexbrainman commented May 26, 2018

billziss-gh commented May 26, 2018

alexbrainman commented May 26, 2018

billziss-gh commented May 26, 2018

billziss-gh commented May 26, 2018

alexbrainman commented May 26, 2018

gopherbot commented May 26, 2018

gopherbot commented May 26, 2018

andybons commented May 26, 2018

alexbrainman commented May 26, 2018

gopherbot commented May 26, 2018

gopherbot commented May 26, 2018

gopherbot commented May 26, 2018

gopherbot commented May 27, 2018

gopherbot commented May 27, 2018

gopherbot commented May 28, 2018

billziss-gh commented May 29, 2018

alexbrainman commented May 30, 2018

gopherbot commented Jun 3, 2018

billziss-gh commented Jun 3, 2018 • edited Loading

gopherbot commented Jun 3, 2018

gopherbot commented Jun 3, 2018

billziss-gh commented Jun 3, 2018

billziss-gh commented Jun 4, 2018

gopherbot commented Jun 5, 2018

alexbrainman commented Jun 5, 2018

gopherbot commented Jun 5, 2018

gopherbot commented Jun 5, 2018

gopherbot commented Jun 5, 2018

gopherbot commented Jun 5, 2018

gopherbot commented Jun 5, 2018

billziss-gh commented Jun 5, 2018

alexbrainman commented Jun 9, 2018

billziss-gh commented Jun 3, 2018 •

edited

Loading