Issue 5617048: code review 5617048: pkg/runtime: Plan 9 signal handling in Go (Closed)
|
Created:
13 years, 1 month ago by akumar Modified:
12 years, 10 months ago Reviewers:
CC:
golang-dev, ality, rminnich, rsc, John Floren, aam Visibility:
Public. |
Descriptionpkg/runtime: Plan 9 signal handling in Go
This adds proper note handling for Plan 9,
and fixes the issue of properly killing go procs.
Without this change, the first go proc that dies
(using runtime·exit()) would kill all the running
go procs. Proper signal handling is needed.
Patch Set 1 #Patch Set 2 : diff -r 855fc8e73259 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r da15310087d6 https://go.googlecode.com/hg/ #
Total comments: 8
Patch Set 4 : diff -r 8dd8f41d4a7e https://go.googlecode.com/hg/ #
Total comments: 11
Patch Set 5 : diff -r 762426ee0cca https://code.google.com/p/go #Patch Set 6 : diff -r 762426ee0cca https://code.google.com/p/go #Patch Set 7 : diff -r 762426ee0cca https://code.google.com/p/go #
Total comments: 12
Patch Set 8 : diff -r a94cc825cbb3 https://code.google.com/p/go #Patch Set 9 : diff -r 7ee60b35f644 https://code.google.com/p/go #Patch Set 10 : diff -r 7ee60b35f644 https://code.google.com/p/go #
MessagesTotal messages: 17
Hello golang-dev@googlegroups.com (cc: ality@pbrane.org, john@jfloren.net, mirtchovski@gmail.com, nix-dev@googlegroups.com, rminnich@gmail.com, rsc@golang.org), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
I'd like to postpone all the Plan 9 work until after Go 1. That said, this approach does not work quite right. An exiting process needs to take down all the other procs that have been created; notepg is the wrong way to do this but so is not doing it. There needs to be a list of the procs and their pids and then exit needs to signal all the other pids to exit. See /sys/src/libthread. Thanks. Russ
Sign in to reply to this message.
Hi Russ, Just to point out: creating a list of pids and sending them a signal is exactly what I'm doing at thread_plan9.c:162. We can wait until after Go 1. Thanks, Akshat On 6 February 2012 09:47, Russ Cox <rsc@golang.org> wrote: > I'd like to postpone all the Plan 9 work until after Go 1. > > That said, this approach does not work quite right. > An exiting process needs to take down all the other > procs that have been created; notepg is the wrong > way to do this but so is not doing it. There needs to > be a list of the procs and their pids and then exit needs > to signal all the other pids to exit. See /sys/src/libthread. > > Thanks. > Russ
Sign in to reply to this message.
On Mon, Feb 6, 2012 at 13:36, Akshat Kumar <seed@mail.nanosouffle.net> wrote: > Just to point out: creating a list of pids and > sending them a signal is exactly what I'm doing > at thread_plan9.c:162. Indeed, sorry about that. The atomic stuff made me think it was something else.
Sign in to reply to this message.
https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/os_plan9.h File src/pkg/runtime/os_plan9.h (right): https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/os_plan9.h#n... src/pkg/runtime/os_plan9.h:16: int32 runtime·plan9_notify(void (*fn)(void*, byte*)); There's no need for the plan9_ prefix on these ones since they don't conflict with any other functions. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:117: runtime·plan9_noted(NCONT); Is this really what we want? Shouldn't we be calling runtime·exits instead? Also, we should name this "go: exiting" since there me be others in the future, especially once we start looking at the os/signal package. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:124: runtime·postnote(int32 pid, int8* msg, uint32 len) I would get rid of the length argument and just call strlen before the write. I imagine most calls will be passing in string constants. It might also be a good idea to have it return zero on success and negative one on failure. So: int32 runtime·postnote(int32 pid, int8* msg) https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:131: if(pid == 0 || pid == _tos->pid) If pid is zero (which means m->procid was also) then there's a problem with the Tos information. In this case we should just let the open fail and return an error.
Sign in to reply to this message.
PTAL I've updated how the note handling works, how we attain PIDs, and added an extra case in the note handler. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/os_plan9.h File src/pkg/runtime/os_plan9.h (right): https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/os_plan9.h#n... src/pkg/runtime/os_plan9.h:16: int32 runtime·plan9_notify(void (*fn)(void*, byte*)); On 2012/02/08 08:20:39, ality wrote: > There's no need for the plan9_ prefix on these > ones since they don't conflict with any other > functions. Done. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:117: runtime·plan9_noted(NCONT); I've now implemented the proper behaviour. The "gointr" case remains as a way to interrupt running go procs. This may be needed for future signal handling, so I'm leaving it in there. As such, the name "gointr" is appropriate here. On 2012/02/08 08:20:39, ality wrote: > Is this really what we want? Shouldn't we be calling > runtime·exits instead? > > Also, we should name this "go: exiting" since there > me be others in the future, especially once we start > looking at the os/signal package. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:124: runtime·postnote(int32 pid, int8* msg, uint32 len) On 2012/02/08 08:20:39, ality wrote: > I would get rid of the length argument and just > call strlen before the write. I imagine most calls > will be passing in string constants. > > It might also be a good idea to have it return > zero on success and negative one on failure. > > So: > > int32 > runtime·postnote(int32 pid, int8* msg) Done. https://codereview.appspot.com/5617048/diff/4001/src/pkg/runtime/thread_plan9... src/pkg/runtime/thread_plan9.c:131: if(pid == 0 || pid == _tos->pid) I disagree with this assessment: in the topmost parent go proc, m->procid is 0 because it has only been initialized. It takes its value from the Tos structure only during an rfork. Thus, new go procs will have non-zero m->procid, but the initial go proc will not. On 2012/02/08 08:20:39, ality wrote: > If pid is zero (which means m->procid was also) > then there's a problem with the Tos information. > In this case we should just let the open fail > and return an error.
Sign in to reply to this message.
https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/os_plan9.h File src/pkg/runtime/os_plan9.h (right): https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/os_plan9.h#... src/pkg/runtime/os_plan9.h:20: int32 runtime·strncmp(byte*, byte*, int32); Put this declaration in runtime.c. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:11: int32 runtime·ppid; Why not just set m->procid for the initial process during runtime·osinit? Then you can get rid of ppid and the check for m->procid != 0 in goexitsall. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:44: getpid(void) This function is unnecessary. Just use _tos->pid. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:158: if(runtime·strncmp(s, (byte*)"sys:", 4) == 0){ I think we should just call noted(NDFLT) for this case since it might be useful to have the other processess alive when debugging. What do you think? https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:187: for(q=tmp; pid > 0;) { Since we do this a few times it might be useful to have a runtime·itoa function in runtime.c: int32 runtime·atoi(num int32, buf *byte, size int32) https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:198: fd = runtime·open(buf, OWRITE); Style nit. Ignore if you disagree. :) if((fd = runtime·open(buf, OWRITE)) < 0) return -1;
Sign in to reply to this message.
On 2012/02/16 01:17:18, ality wrote: > > https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... > src/pkg/runtime/thread_plan9.c:44: getpid(void) > This function is unnecessary. Just use _tos->pid. Yes but be sure to create a test so we can be sure getpid() == _tos->pid, as this has been a continuing issue. It will save wasted time as from time to time tos has gotten broken. Don't do the change until you add this test please. I've wasted too much time on this problem! If the test fails I think the build should fail -- it's ridiculous that we keep fouling this up :-) ron
Sign in to reply to this message.
PTAL. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/os_plan9.h File src/pkg/runtime/os_plan9.h (right): https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/os_plan9.h#... src/pkg/runtime/os_plan9.h:20: int32 runtime·strncmp(byte*, byte*, int32); On 2012/02/16 01:17:19, ality wrote: > Put this declaration in runtime.c. I assume you mean runtime.h. Done. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:11: int32 runtime·ppid; On 2012/02/16 01:17:19, ality wrote: > Why not just set m->procid for the initial process during > runtime·osinit? Then you can get rid of ppid and the check > for m->procid != 0 in goexitsall. Done. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:44: getpid(void) On 2012/02/16 01:17:19, ality wrote: > This function is unnecessary. Just use _tos->pid. Tos is unreliable. This is straightforward and works reliably. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:158: if(runtime·strncmp(s, (byte*)"sys:", 4) == 0){ On 2012/02/16 01:17:19, ality wrote: > I think we should just call noted(NDFLT) for this case since > it might be useful to have the other processess alive when > debugging. What do you think? Go's fault-intolerant design dictates that if one go proc meets a gruesome demise from the system, then they all do. It may have children, but it's still monolithic. https://codereview.appspot.com/5617048/diff/12001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:187: for(q=tmp; pid > 0;) { On 2012/02/16 01:17:19, ality wrote: > Since we do this a few times it might be useful to > have a runtime·itoa function in runtime.c: > > int32 > runtime·atoi(num int32, buf *byte, size int32) Added the function, but since we're the only users, I'll keep it in here for now. If it looks good for general purpose, I can push it into runtime.c.
Sign in to reply to this message.
This looks great. And, yes, I am glad you do not use tos->pid. It's a cute idea but it's not been stable enough to rely on. Maybe later. LGTM
Sign in to reply to this message.
ping! Other reviewers? There are quite a few CLs for Plan 9 now that are lingering.
Sign in to reply to this message.
Sorry for the slow response. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:10: int8 *runtime·exitstatus; I think there's a potential data race with exitstatus. Is it worth the extra code since the program is about to die? https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:43: getpid(void) This is fine with me but I want to point out that runtime·rfork initializes m->procid with _tos->pid. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:57: n = runtime·atoi(c); return runtime·atoi(c); https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:137: runtime·itoa(int32 n, byte *p, uint32 len) We should check writes into p against len. Also, space before brace. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:223: if(runtime·write(fd, msg, len) != len){ Space before brace. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:238: else{ Same.
Sign in to reply to this message.
PTAL. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan9.c File src/pkg/runtime/thread_plan9.c (right): https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:10: int8 *runtime·exitstatus; This needs to be done, since we need to properly set errstr on exit, depending upon whether we exited cleanly or if there was an error of some sort. Tests will need this, among other things. The race will have to be fixed, but that should be done in increments. On 2012/04/17 00:15:52, ality wrote: > I think there's a potential data race with exitstatus. > Is it worth the extra code since the program is about > to die? https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:43: getpid(void) A separate CL will change that behaviour in sys_plan9_386.s as well. On 2012/04/17 00:15:52, ality wrote: > This is fine with me but I want to point out > that runtime·rfork initializes m->procid with > _tos->pid. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:57: n = runtime·atoi(c); On 2012/04/17 00:15:52, ality wrote: > return runtime·atoi(c); Done. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:137: runtime·itoa(int32 n, byte *p, uint32 len) On 2012/04/17 00:15:52, ality wrote: > We should check writes into p against len. > Also, space before brace. Done. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:223: if(runtime·write(fd, msg, len) != len){ On 2012/04/17 00:15:52, ality wrote: > Space before brace. Done. https://codereview.appspot.com/5617048/diff/29001/src/pkg/runtime/thread_plan... src/pkg/runtime/thread_plan9.c:238: else{ On 2012/04/17 00:15:52, ality wrote: > Same. Done.
Sign in to reply to this message.
LGTM Does anyone else have comments? I'll submit this on Monday if there are no objections. Cheers, Anthony
Sign in to reply to this message.
LGTM Let's drop strncmp. The one place you use it could just as well be runtime·mcmp. Russ
Sign in to reply to this message.
PTAL. Done. The only reservation I had about using runtime·mcmp was that it doesn't check for hitting an end of string on s1 before n hits zero. Of course, this makes sense for its intended use. Thanks, Akshat On 2012/05/03 21:51:41, rsc wrote: > LGTM > > Let's drop strncmp. > The one place you use it could just as well be runtime·mcmp. > > Russ
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=9233f811c522 *** pkg/runtime: Plan 9 signal handling in Go This adds proper note handling for Plan 9, and fixes the issue of properly killing go procs. Without this change, the first go proc that dies (using runtime·exit()) would kill all the running go procs. Proper signal handling is needed. R=golang-dev, ality, rminnich, rsc CC=golang-dev, john, mirtchovski http://codereview.appspot.com/5617048 Committer: Anthony Martin <ality@pbrane.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
