Issue 9843046: code review 9843046: runtime: introduce preemption function (not used for now) (Closed)
Descriptionruntime: introduce preemption function (not used for now)
This is part of preemptive scheduler.
Patch Set 1 #Patch Set 2 : diff -r 7bd52af9db1c https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r 7bd52af9db1c https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 4 : diff -r 7bd52af9db1c https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 2
Patch Set 5 : diff -r e3f2b59f2404 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 9
Patch Set 6 : diff -r ccd8c0dad0a7 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 7 : diff -r ccd8c0dad0a7 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 14
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
Sign in to reply to this message.
Can this be part of a change with more context?
Sign in to reply to this message.
On 2013/05/31 04:23:23, cshapiro1 wrote: > Can this be part of a change with more context? dunno I used to do it this way, because people here asked for small CLs.
Sign in to reply to this message.
https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:2071: preemptall(void) Please add a comment before these new functions explaining what they do and explaining what locks need to be held before calling them. https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c#newco... src/pkg/runtime/proc.c:2096: gp->stackguard = StackPreempt; If I understand this correctly, this g might actually be running in parallel. So this assignment to gp->stackguard might be overwritten before it takes effect. If I'm right I think there needs to be a comment explaining that.
Sign in to reply to this message.
On 2013/06/01 00:24:46, iant wrote: > https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c > File src/pkg/runtime/proc.c (right): > > https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c#newco... > src/pkg/runtime/proc.c:2071: preemptall(void) > Please add a comment before these new functions explaining what they do and > explaining what locks need to be held before calling them. > > https://codereview.appspot.com/9843046/diff/8001/src/pkg/runtime/proc.c#newco... > src/pkg/runtime/proc.c:2096: gp->stackguard = StackPreempt; > If I understand this correctly, this g might actually be running in parallel. > So this assignment to gp->stackguard might be overwritten before it takes > effect. If I'm right I think there needs to be a comment explaining that. Added the comments. The semantics are formally very weak, by no locks needs to be held. Thanks!
Sign in to reply to this message.
LGTM https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2074: // Issue preemption signal to all running goroutines. Don't use the word signal here. It sounds like, well, a signal, via the kill function. Which is not what happens here. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2077: // the signal to a wrong goroutine that has already finished running. I'm not sure what you mean by "a wrong goroutine" here. And why do we even need to say anything about that case? It seems that trying to preempt a goroutine that has finished will do nothing in any case. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2080: preemptall(void) Tell all goroutines that they have been preempted and they should stop. This function is purely best-effort. It can fail to inform a goroutine if a processor just started running it. No locks need to be held. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2099: static void Tell the goroutine running on processor P to stop. This function is purely best-effort. It can incorrectly fail to inform the goroutine. It can send inform the wrong goroutine. Even if it informs the correct goroutine, that goroutine might ignore the request if it is simultaneously executing runtime·newstack. No lock needs to be held. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/stack.h File src/pkg/runtime/stack.h (right): https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/stack.h#new... src/pkg/runtime/stack.h:112: StackPreempt = (uintptr)-42, In practice people looking at stackguard0 are going to see a number printed in hex, so I think this number should be something recognizable in hex. Perhaps something like (uintptr)(intptr)0xfffffade ?
Sign in to reply to this message.
https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2077: // the signal to a wrong goroutine that has already finished running. On 2013/06/03 04:41:44, iant wrote: > I'm not sure what you mean by "a wrong goroutine" here. And why do we even need > to say anything about that case? It seems that trying to preempt a goroutine > that has finished will do nothing in any case. I mean that if P finishes running g0 and starts running g1; this code can notify g0, while from user point of view the correct goroutine is g1 in this case, and g0 is the wrong goroutine to notify. Well, actually, if the thread that executes preemptall() is preempted by OS after reading p->m->curg but before notifying, later it will notify essentially a random goroutine (e.g. the goroutine can finish and be reused several times, and be e.g. in a syscall). https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2080: preemptall(void) On 2013/06/03 04:41:44, iant wrote: > Tell all goroutines that they have been preempted and they should stop. > This function is purely best-effort. It can fail to inform a goroutine if a > processor just started running it. > No locks need to be held. Done. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/proc.c#newc... src/pkg/runtime/proc.c:2099: static void On 2013/06/03 04:41:44, iant wrote: > Tell the goroutine running on processor P to stop. > This function is purely best-effort. It can incorrectly fail to inform the > goroutine. It can send inform the wrong goroutine. Even if it informs the > correct goroutine, that goroutine might ignore the request if it is > simultaneously executing runtime·newstack. > No lock needs to be held. Done. https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/stack.h File src/pkg/runtime/stack.h (right): https://codereview.appspot.com/9843046/diff/15001/src/pkg/runtime/stack.h#new... src/pkg/runtime/stack.h:112: StackPreempt = (uintptr)-42, On 2013/06/03 04:41:44, iant wrote: > In practice people looking at stackguard0 are going to see a number printed in > hex, so I think this number should be something recognizable in hex. Perhaps > something like (uintptr)(intptr)0xfffffade ? Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=01810e5c68e9 *** runtime: introduce preemption function (not used for now) This is part of preemptive scheduler. R=golang-dev, cshapiro, iant CC=golang-dev https://codereview.appspot.com/9843046
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/9843046/diff/28001/src/pkg/runtime/stack.h File src/pkg/runtime/stack.h (right): https://codereview.appspot.com/9843046/diff/28001/src/pkg/runtime/stack.h#new... src/pkg/runtime/stack.h:112: StackPreempt = (uintptr)(intptr)0xfffffade, This line breaks the Plan 9 build: /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 name not declared: intptr /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 enum not a constant: StackPreempt /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 syntax error, last name: 0xfffffade changing it to StackPreempt = (uintptr)0xfffffade, makes the error go away. Is plan9 missing a def somewhere?
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/06/04 18:13:37, jnj wrote: > https://codereview.appspot.com/9843046/diff/28001/src/pkg/runtime/stack.h > File src/pkg/runtime/stack.h (right): > > https://codereview.appspot.com/9843046/diff/28001/src/pkg/runtime/stack.h#new... > src/pkg/runtime/stack.h:112: StackPreempt = (uintptr)(intptr)0xfffffade, > This line breaks the Plan 9 build: > > /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 > /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 name not declared: intptr > /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 > /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 enum not a constant: StackPreempt > /usr/ec2/src/go/src/cmd/8l/../ld/../../pkg/runtime/stack.h:112 > /usr/ec2/src/go/src/cmd/8l/../ld/lib.c:35 syntax error, last name: 0xfffffade > > changing it to > > StackPreempt = (uintptr)0xfffffade, > > makes the error go away. Is plan9 missing a def somewhere? The cast to intptr matters because we want to sign extend if necessary, to ensure that we get an invalid address. intptr is a typedef in runtime.h. I see now that cmd/ld also includes stack.h, naturally without including runtime.h. I also see that intptr is a typedef in include/u.h, though, and 8l/l.h includes u.h. Why isn't intptr defined there? Ian
Sign in to reply to this message.
iant@golang.org once said: > The cast to intptr matters because we want to sign extend if necessary, > to ensure that we get an invalid address. intptr is a typedef in > runtime.h. I see now that cmd/ld also includes stack.h, naturally > without including runtime.h. I also see that intptr is a typedef in > include/u.h, though, and 8l/l.h includes u.h. Why isn't intptr defined > there? Sorry. I forgot to add intptr when I originally added the Plan 9 includes. Fix is here: https://codereview.appspot.com/10023043/ Anthony
Sign in to reply to this message.
On Tue, Jun 4, 2013 at 2:43 PM, <iant@golang.org> wrote: > The cast to intptr matters because we want to sign extend if necessary, > to ensure that we get an invalid address. intptr is a typedef in > runtime.h. I see now that cmd/ld also includes stack.h, naturally > without including runtime.h. I also see that intptr is a typedef in > include/u.h, though, and 8l/l.h includes u.h. Why isn't intptr defined > there? > One reason not to have intptr is so that it cannot be used incorrectly. There are very few correct uses for it. If we are on a 32-bit machine, then I think we all agree that (uintptr)(intptr)0xfffffade == (uintptr)0xfffffade. If we are on a 64-bit machine, then: 0xfffffade has type unsigned int, meaning it is an unsigned 32-bit int, intptr is a signed 64-bit int. A conversion of a uint32 to an int64 can be injective, and so it should be, meaning that (intptr)0xfffffade has zeros in its high bits. So (uintptr)(intptr)0xfffffade == (uintptr)0xfffffade. Really this code should say (uintptr)(int32)0xfffffade, with the inner cast forcing the high bit of the value to become a sign bit, which will in turn cause the int32->uintptr conversion to sign extend as described. This code is even more sketchy because it is being used to initialize an enum value. The standard is super vague about what the representation type of an enum is, and compilers disagree. 6c uses the "maximum" type of the initializers, which in this case ends up being uintptr. But that's not something you really want to rely on (and it still doesn't lead to correct behavior here). I included stack.h in runtime.c and added { uintptr p = StackPreempt; if((p>>32) == 0) runtime·throw("StackPreempt -> uintptr is small"); } to runtime.check. It fails. I can't see how the preemption check is working, except that maybe Dmitriy is not really testing with this specific code. Russ
Sign in to reply to this message.
On Mon, Jun 10, 2013 at 11:29 PM, Russ Cox <rsc@golang.org> wrote: > On Tue, Jun 4, 2013 at 2:43 PM, <iant@golang.org> wrote: >> >> The cast to intptr matters because we want to sign extend if necessary, >> to ensure that we get an invalid address. intptr is a typedef in >> runtime.h. I see now that cmd/ld also includes stack.h, naturally >> without including runtime.h. I also see that intptr is a typedef in >> include/u.h, though, and 8l/l.h includes u.h. Why isn't intptr defined >> there? > > > One reason not to have intptr is so that it cannot be used incorrectly. > There are very few correct uses for it. > > If we are on a 32-bit machine, then I think we all agree that > (uintptr)(intptr)0xfffffade == (uintptr)0xfffffade. > > If we are on a 64-bit machine, then: 0xfffffade has type unsigned int, > meaning it is an unsigned 32-bit int, intptr is a signed 64-bit int. A > conversion of a uint32 to an int64 can be injective, and so it should be, > meaning that (intptr)0xfffffade has zeros in its high bits. So > (uintptr)(intptr)0xfffffade == (uintptr)0xfffffade. > > Really this code should say (uintptr)(int32)0xfffffade, with the inner cast > forcing the high bit of the value to become a sign bit, which will in turn > cause the int32->uintptr conversion to sign extend as described. > > This code is even more sketchy because it is being used to initialize an > enum value. The standard is super vague about what the representation type > of an enum is, and compilers disagree. 6c uses the "maximum" type of the > initializers, which in this case ends up being uintptr. But that's not > something you really want to rely on (and it still doesn't lead to correct > behavior here). > > I included stack.h in runtime.c and added > > { uintptr p = StackPreempt; > if((p>>32) == 0) runtime·throw("StackPreempt -> uintptr is small"); } > > to runtime.check. It fails. > > I can't see how the preemption check is working, except that maybe Dmitriy > is not really testing with this specific code. It's not yet used, it's blocked on several other changes. Most likely it would be discovered later :)
Sign in to reply to this message.
On Mon, Jun 10, 2013 at 12:29 PM, Russ Cox <rsc@golang.org> wrote: > > If we are on a 64-bit machine, then: 0xfffffade has type unsigned int, > meaning it is an unsigned 32-bit int, intptr is a signed 64-bit int. A > conversion of a uint32 to an int64 can be injective, and so it should be, > meaning that (intptr)0xfffffade has zeros in its high bits. So > (uintptr)(intptr)0xfffffade == (uintptr)0xfffffade. Sorry about that. I hope that was just a bad day. Ian
Sign in to reply to this message.
|
