|
|
Descriptionnet: limit number of concurrent cgo calls
The limit is 500. There is no way to change it.
This primarily affects name resolution.
If a million goroutines try to resolve DNS names,
only 500 will get to execute cgo calls at a time.
But in return the operating system will not crash.
Fixes issue 5625.
Patch Set 1 #Patch Set 2 : diff -r aeb72c90c10e https://code.google.com/p/go/ #Patch Set 3 : diff -r aeb72c90c10e https://code.google.com/p/go/ #
Total comments: 3
Patch Set 4 : diff -r b3ee0be87a1c https://code.google.com/p/go/ #
MessagesTotal messages: 31
Hello golang-dev@googlegroups.com (cc: bradfitz), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go File src/pkg/net/net.go (right): https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446 src/pkg/net/net.go:446: threadLimit <- struct{}{} Is this safe since threadLimit is buffered? Referring to thread at https://groups.google.com/d/topic/golang-dev/ShqsqvCzkWg
Sign in to reply to this message.
On Thu, Aug 15, 2013 at 11:34 PM, <dan.kortschak@adelaide.edu.au> wrote: > https://codereview.appspot.**com/13038043/diff/6001/src/** > pkg/net/net.go#newcode446<https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446> > src/pkg/net/net.go:446: threadLimit <- struct{}{} > Is this safe since threadLimit is buffered? Referring to thread at > https://groups.google.com/d/**topic/golang-dev/ShqsqvCzkWg<https://groups.goo... Yes, it is safe. The example is unsafe because it is using the channel ops to protect access to memory, and when used in this way they do not do that. But they do work fine for just making sure there are a certain number of goroutines doing a particular operation. Russ
Sign in to reply to this message.
https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test.go File src/pkg/net/dialgoogle_test.go (right): https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test... src/pkg/net/dialgoogle_test.go:71: for i := 0; i < N*9/10; i++ { There are tests that count number of goroutines in net. And they also appear in stack traces. Does waiting for all makes the test longer, or it makes it flaky? I would not bother about few additional seconds in a non Short test.
Sign in to reply to this message.
On 2013/08/16 05:32:21, rsc wrote: > On Thu, Aug 15, 2013 at 11:34 PM, <mailto:dan.kortschak@adelaide.edu.au> wrote: > > > https://codereview.appspot.**com/13038043/diff/6001/src/** > > > pkg/net/net.go#newcode446<https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446> > > src/pkg/net/net.go:446: threadLimit <- struct{}{} > > Is this safe since threadLimit is buffered? Referring to thread at > > > https://groups.google.com/d/**topic/golang-dev/ShqsqvCzkWg%3Chttps://groups.g...> > > > Yes, it is safe. The example is unsafe because it is using the channel ops > to protect access to memory, and when used in this way they do not do that. > But they do work fine for just making sure there are a certain number of > goroutines doing a particular operation. This is a hard question. I believe formally Dan is correct. There is no difference in the spec for memory accesses and other operations. Formally all cgo calls happen concurrently (not ordered by happens-before).
Sign in to reply to this message.
On 2013/08/16 08:33:11, dvyukov wrote: > On 2013/08/16 05:32:21, rsc wrote: > > On Thu, Aug 15, 2013 at 11:34 PM, <mailto:dan.kortschak@adelaide.edu.au> > wrote: > > > > > https://codereview.appspot.**com/13038043/diff/6001/src/** > > > > > > pkg/net/net.go#newcode446<https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446> > > > src/pkg/net/net.go:446: threadLimit <- struct{}{} > > > Is this safe since threadLimit is buffered? Referring to thread at > > > > > > https://groups.google.com/d/**topic/golang-dev/ShqsqvCzkWg%253Chttps://groups...> > > > > > > Yes, it is safe. The example is unsafe because it is using the channel ops > > to protect access to memory, and when used in this way they do not do that. > > But they do work fine for just making sure there are a certain number of > > goroutines doing a particular operation. > > > This is a hard question. > I believe formally Dan is correct. There is no difference in the spec for memory > accesses and other operations. Formally all cgo calls happen concurrently (not > ordered by happens-before). I am not using the channel to protect anything except the number of concurrent uses. There is no memory involved here. I don't see how the memory model is even relevant. I am claiming that for a channel with buffer size N, if an arbitrary number of goroutines execute a send to the channel, then do work, then receive from the channel, that there can be at most N goroutines doing work. Are you claiming that we have not guaranteed that? Because if not, we've screwed up. Russ
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=565e2d46bb68 *** net: limit number of concurrent cgo calls The limit is 500. There is no way to change it. This primarily affects name resolution. If a million goroutines try to resolve DNS names, only 500 will get to execute cgo calls at a time. But in return the operating system will not crash. Fixes issue 5625. R=golang-dev, dan.kortschak, r, dvyukov CC=bradfitz, golang-dev https://codereview.appspot.com/13038043 https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test.go File src/pkg/net/dialgoogle_test.go (right): https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test... src/pkg/net/dialgoogle_test.go:71: for i := 0; i < N*9/10; i++ { On 2013/08/16 08:26:53, dvyukov wrote: > There are tests that count number of goroutines in net. And they also appear in > stack traces. > Does waiting for all makes the test longer, or it makes it flaky? > I would not bother about few additional seconds in a non Short test. It makes it significantly longer. With the print below uncommented, the test gets to 9900 and then sits, I assume because it is waiting for timeouts on some dropped DNS requests. As long as the other tests are not failing, I think stopping early makes sense.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/08/17 01:24:10, rsc wrote: > On 2013/08/16 08:33:11, dvyukov wrote: > > On 2013/08/16 05:32:21, rsc wrote: > > > On Thu, Aug 15, 2013 at 11:34 PM, <mailto:dan.kortschak@adelaide.edu.au> > > wrote: > > > > > > > https://codereview.appspot.**com/13038043/diff/6001/src/** > > > > > > > > > > pkg/net/net.go#newcode446<https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446> > > > > src/pkg/net/net.go:446: threadLimit <- struct{}{} > > > > Is this safe since threadLimit is buffered? Referring to thread at > > > > > > > > > > https://groups.google.com/d/**topic/golang-dev/ShqsqvCzkWg%25253Chttps://grou...> > > > > > > > > > Yes, it is safe. The example is unsafe because it is using the channel ops > > > to protect access to memory, and when used in this way they do not do that. > > > But they do work fine for just making sure there are a certain number of > > > goroutines doing a particular operation. > > > > > > This is a hard question. > > I believe formally Dan is correct. There is no difference in the spec for > memory > > accesses and other operations. Formally all cgo calls happen concurrently (not > > ordered by happens-before). > > I am not using the channel to protect anything except the number of concurrent > uses. There is no memory involved here. I don't see how the memory model is even > relevant. > > I am claiming that for a channel with buffer size N, if an arbitrary number of > goroutines execute a send to the channel, then do work, then receive from the > channel, that there can be at most N goroutines doing work. Are you claiming > that we have not guaranteed that? Because if not, we've screwed up. Memory model is relevant here. If it would not be relevant here then: if you have a program where one goroutine prints "A" and another goroutine prints "B" (or sends to network or whatever), you would never be able make this program reliably print "AB". Memory model allows you to enforce that printing of "A" happens before printing of "B", by e.g. synchronizing them with a channel or mutex. In your example potentially infinite amount of "work" happens concurrently. Channels are meant for exchanging messages, and you are exchanging empty slots in channels internal buffer. There may even be no slots at all, if it's built as linked list. So semantically you are doing nothing.
Sign in to reply to this message.
On Sat, Aug 17, 2013 at 5:30 AM, <dvyukov@google.com> wrote: > Memory model is relevant here. If it would not be relevant here then: > if you have a program where one goroutine prints "A" and another > goroutine prints "B" (or sends to network or whatever), you would never > be able make this program reliably print "AB". Memory model allows you > to enforce that printing of "A" happens before printing of "B", by e.g. > synchronizing them with a channel or mutex. > I am not trying to enforce any ordering, only collective mutual exclusion. > In your example potentially infinite amount of "work" happens > concurrently. > How can that happen? > Channels are meant for exchanging messages, and you are exchanging empty > slots in channels internal buffer. There may even be no slots at all, if > it's built as linked list. So semantically you are doing nothing. > It sounds like you are claiming there is no difference between a make(chan struct{}, 500) and a make(chan struct{}, 500000). But there is. They have different capacities. This is not a property of the memory model. Russ
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/09/06 19:30:56, rsc wrote: > It sounds like you are claiming there is no difference between a make(chan > struct{}, 500) and a make(chan struct{}, 500000). But there is. They have > different capacities. This is not a property of the memory model. This is what I was trying to address with Issue 6242. If it's not part of the memory model, then where can someone point to that calms discussions (e.g. those listed in the issue) about whether it's safe or not. If it's safe, there should be a place where someone can point to that refutes arguments like Dmitry's definitionally. At the moment, nothing like that exists.
Sign in to reply to this message.
I'm going to support Russ here. We don't care about the ordering so avoiding the init overhead is worth it. Let's revert. -rob
Sign in to reply to this message.
Message was sent while issue was closed.
Personally, I'm happy that the original approach is being used - it's clearer and more concise. The use of it here after discussion is something that supports the idiom's wider use and can be pointed to, but I am still concerned that there is nowhere that can be pointed to in a specification document that says that it's safe. The concern is based on conflicting views about the formal correctness of it (under aggressive optimisation).
Sign in to reply to this message.
On Wed, Sep 18, 2013 at 9:00 PM, <dan.kortschak@adelaide.edu.au> wrote: > The use of it here after discussion is something that supports the > idiom's wider use and can be pointed to, but I am still concerned that > there is nowhere that can be pointed to in a specification document that > says that it's safe. The concern is based on conflicting views about the > formal correctness of it (under aggressive optimisation). > For what it's worth, I have yet to see someone explain to me why it could be perceived as unsafe. The memory model is concerned with memory; there is no memory here. Russ
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/09/23 17:39:46, rsc wrote: > On Wed, Sep 18, 2013 at 9:00 PM, <mailto:dan.kortschak@adelaide.edu.au> wrote: > > > The use of it here after discussion is something that supports the > > idiom's wider use and can be pointed to, but I am still concerned that > > there is nowhere that can be pointed to in a specification document that > > says that it's safe. The concern is based on conflicting views about the > > formal correctness of it (under aggressive optimisation). > > > > For what it's worth, I have yet to see someone explain to me why it could > be perceived as unsafe. The memory model is concerned with memory; there is > no memory here. Probably I need to write up something more fundamental, but I don't have time right now. The short version: It is the Happens-Before (HB) relation that is the foundation for reasoning about ordering of events in different goroutines. If you have 2 unordered by HB memory accesses, you have a data race. If you have 2 unordered by HB fmt.Printf calls, then you have non-deterministic output. If you want deterministic "Hello, world!" output, then you need to ensure that fmt.Printf("Hello, ") happens-before fmt.Printf("world!"). There is really not much difference between "memory actions" and "non memory actions" here. Modern compilers tend to treat them equally, and in fact clang can reorder memory accesses with malloc/free calls, something that used to be "opaque external calls". So this may actually be a concern with gccgo. Hardware definitely does not care much about external calls, etc when it reorders memory accesses. For example, if you need to ensure mutual exclusion for some C routine, and you do it with such "inverted chan semaphore", and we do not put proper memory barriers into chan implementation (because it is not specified in Memory Model), you do can corrupt C routine state. The point here is that you must not think about this in terms of memory/non-memory actions. You need to think in terms of ordering of actions. If there is something in the language specification (e.g. capacity of channels) that you think affects reasoning about ordering of events in different goroutines, this must be explicitly duplicated in definition of HB relation.
Sign in to reply to this message.
Message was sent while issue was closed.
> If you have 2 unordered by HB memory accesses, you have a data race. Exactly. I have no such thing here.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/09/23 21:14:11, rsc wrote: > > If you have 2 unordered by HB memory accesses, you have a data race. > > Exactly. I have no such thing here. Well, but you have unlimited number of cgo calls.
Sign in to reply to this message.
The issue for me is if the memory model is not ordering things then what is (this seems to me an RAA that it is actually what is ordering things here - vis. Dmitry's comment that memory actions and non-memory actions are pretty much the same thing in this context; really the taxonomy is between observable [relevant here] and unobservable [unknowable anywhen] actions). The argument that initially got me worried was Dmitry's here https://groups.google.com/d/msg/golang-dev/ShqsqvCzkWg/9WGj2yPK9xYJ. Addressing that would solve the issue I believe. On Mon, 2013-09-23 at 21:14 +0000, rsc@golang.org wrote: > Exactly. I have no such thing here.
Sign in to reply to this message.
On Mon, Sep 23, 2013 at 3:03 PM, Dan Kortschak < dan.kortschak@adelaide.edu.au> wrote: > The issue for me is if the memory model is not ordering things then what > is (this seems to me an RAA that it is actually what is ordering things > here - vis. Dmitry's comment that memory actions and non-memory actions > are pretty much the same thing in this context; really the taxonomy is > between observable [relevant here] and unobservable [unknowable anywhen] > actions). > > The argument that initially got me worried was Dmitry's here > https://groups.google.com/d/msg/golang-dev/ShqsqvCzkWg/9WGj2yPK9xYJ. > Addressing that would solve the issue I believe. > > Addressing that single case is not enough; that's only a single consequence. We must specify all guarantees about ordering of events in different goroutines that we care about in HB relation and then consistently ensure that they are obeyed on both hardware and compiler level.
Sign in to reply to this message.
On Mon, 2013-09-23 at 16:36 -0700, Dmitry Vyukov wrote: > Addressing that single case is not enough; that's only a single > consequence. We must specify all guarantees about ordering of events > in different goroutines that we care about in HB relation and then > consistently ensure that they are obeyed on both hardware and compiler > level. Yes, sorry, I should have been more clear. The ordering issue is the concern for me, but that argument was what initially raised the issue for me.
Sign in to reply to this message.
The code before my CL had an unlimited number of concurrent calls to a function - let's call it F. I used my CL to limit the number of concurrent calls of F to 500. If F worked before with no ordering between the calls, it still works now, with no ordering between the slightly fewer calls. There is no shared Go data between the many calls to F. Internally, F is calling the C getaddrinfo(3) routine, and that routine is required to use locks or some other mechanism to support multiple simultaneous calls. The point is this: if the code was okay before (and I assert it was), it's still okay now.
Sign in to reply to this message.
I think this thread is unfortunately tangled, and I apologise if I contributed to that. The issue that I'm concerned about is really Issue 6242, so perhaps that is a better location. I'll just answer the following here though. On Mon, 2013-09-23 at 22:32 -0400, Russ Cox wrote: > The code before my CL had an unlimited number of concurrent calls to a > function - let's call it F. I used my CL to limit the number of > concurrent calls of F to 500. If F worked before with no ordering > between the calls, it still works now, with no ordering between the > slightly fewer calls. The ordering at issue here is not the ordering between the calls but rather between the completion of a call to releaseThread and acquireThread; the CL here depends on there being a releaseThread HB acquireThread relationship when len(threadLimit) == 500, which is true with the implementation of the compilers as they are, but according to Dmitry (AFAICS) not guaranteed. > There is no shared Go data between the many calls to F. Internally, F > is calling the C getaddrinfo(3) routine, and that routine is required > to use locks or some other mechanism to support multiple simultaneous > calls. > The point is this: if the code was okay before (and I assert it was), > it's still okay now. Yes, that is absolutely true, but not what I'm trying to address. I'm sorry if I've made this bigger that it really needed to be.
Sign in to reply to this message.
On Mon, Sep 23, 2013 at 11:01 PM, Dan Kortschak < dan.kortschak@adelaide.edu.au> wrote: > The ordering at issue here is not the ordering between the calls but > rather between the completion of a call to releaseThread and > acquireThread; the CL here depends on there being a releaseThread HB > acquireThread relationship when len(threadLimit) == 500, which is true > with the implementation of the compilers as they are, but according to > Dmitry (AFAICS) not guaranteed. > 'happens before' only controls whether memory reads are visible. It does not change the semantics of channels in general. If len(threadLimit) == 500, the releaseThread (<-threadLimit) absolutely must execute before another acquireThread (threadLimit <- struct{}{}) can execute, because to do otherwise would be to ignore the channel's capacity. You can't send 501 times on a capacity 500 channel without any receives. That's just fundamental to the definition of channels. (It does not establish a 'happens before' relationship that guarantees the visibility of memory writes before the releaseThread by memory reads after the acquireThread, but the code does not depend on such visibility; we seem to agree on that but it was worth restating.) Russ
Sign in to reply to this message.
On Mon, Sep 23, 2013 at 8:14 PM, Russ Cox <rsc@golang.org> wrote: > On Mon, Sep 23, 2013 at 11:01 PM, Dan Kortschak < > dan.kortschak@adelaide.edu.au> wrote: > >> The ordering at issue here is not the ordering between the calls but >> rather between the completion of a call to releaseThread and >> acquireThread; the CL here depends on there being a releaseThread HB >> acquireThread relationship when len(threadLimit) == 500, which is true >> with the implementation of the compilers as they are, but according to >> Dmitry (AFAICS) not guaranteed. >> > > 'happens before' only controls whether memory reads are visible. It does > not change the semantics of channels in general. > > If len(threadLimit) == 500, the releaseThread (<-threadLimit) absolutely > must execute before another acquireThread (threadLimit <- struct{}{}) can > execute, because to do otherwise would be to ignore the channel's > capacity. You can't send 501 times on a capacity 500 channel without any > receives. That's just fundamental to the definition of channels. (It does > not establish a 'happens before' relationship that guarantees the > visibility of memory writes before the releaseThread by memory reads after > the acquireThread, but the code does not depend on such visibility; we seem > to agree on that but it was worth restating.) > Reasoning about semantics and correctness of Go programs will be really complicated and messy (to the point where I doubt anybody will have complete understanding) if you have separate sets of ordering rules for "memory actions" and "non memory actions", where the rules and guarantees for the former are specified by the Memory Model and Happens Before relation, and for the latter by some implicit subset of the language specification. And note that we don't define memory and non-memory actions as well (is an opaque cgo call a memory access?.. well it depends on what it actually does under the hood, and you don't frequently know). Sure, we can have that. But the question is whether we want that or no. A more reasonable model would be if all rules and guarantees about ordering of events in different goroutines are reflected in the HB relation explicitly. Then, for example, the maximum number of concurrent actions X is the maximum number of unordered X in HB graph. That simple. The answer to this question is non-disputable and almost trivial to answer (including possibly by automatic tools).
Sign in to reply to this message.
If you have a channel of capacity 500, you cannot execute 501 sends before any receives. That is what it means to have capacity 500. It is fine to omit this fact from the memory model, because this is not a statement about memory. However, if you try to reduce all execution to the memory model, you will lose this fundamental fact about channels. If you do that, you are no longer describing Go. It is like the physicist and the spherical cows. Russ
Sign in to reply to this message.
On Mon, Sep 23, 2013 at 10:03 PM, Russ Cox <rsc@golang.org> wrote: > If you have a channel of capacity 500, you cannot execute 501 sends before > any receives. That is what it means to have capacity 500. It is fine to > omit this fact from the memory model, because this is not a statement about > memory. However, if you try to reduce all execution to the memory model, > you will lose this fundamental fact about channels. If you do that, you are > no longer describing Go. It is like the physicist and the spherical cows. > > Then it's probably non necessary to say that receive happens-before the corresponding send, because otherwise you can infer that channel length is negative, which is prohibited by language. I do not understand why I will lose the fundamental fact about channels. The happens-before wording is along the lines of "i-th receive happens-before i+cap(c)-th send". This fully agrees with capacity rules in the specification.
Sign in to reply to this message.
So what does this do to the hypothetical optimisation path that Dmitry sketched in his argument linked above? I'd just like to see a statement that that cannot happen (I guess this is 6242) or that there is a flaw in the argument. Can I have something that demonstrates that cows aren't actually point masses? Dan
Sign in to reply to this message.
The rewrite Dmitriy suggested is not really something an optimizing compiler could ever do except in very limited circumstances. That is, given: var sem = make(chan int, MaxOutstanding) func handle(r *Request) { sem <- 1 // Wait for active queue to drain. process(r) // May take a long time. <-sem // Done; enable next request to run. } it is not okay to move process(r) up one line or down one line unless the compiler can guarantee that the reordering cannot be observed within the goroutine. In particular, it must be sure that (1) process(r) never refers to sem, including by calling other routines that might end up indirectly back at handle, (2) process(r) never blocks, because if it did it might establish an ordering with other code that uses sem, and (3) process(r) cannot panic, since if it did you'd need to see sem in the correct state. Since the code in question in the real example in this CL is calling into C via cgo, the compiler has very limited visibility into what's going on. There's no way it can gather enough evidence to prove that a reordering is safe. Russ
Sign in to reply to this message.
Thanks. Dan
Sign in to reply to this message.
On Mon, Sep 23, 2013 at 10:30 PM, Russ Cox <rsc@golang.org> wrote: > The rewrite Dmitriy suggested is not really something an optimizing compiler > could ever do except in very limited circumstances. > > That is, given: > > var sem = make(chan int, MaxOutstanding) > func handle(r *Request) { > sem <- 1 // Wait for active queue to drain. > process(r) // May take a long time. > <-sem // Done; enable next request to run. > } > > it is not okay to move process(r) up one line or down one line unless the > compiler can guarantee that the reordering cannot be observed within the > goroutine. In particular, it must be sure that (1) process(r) never refers > to sem, including by calling other routines that might end up indirectly > back at handle, (2) process(r) never blocks, because if it did it might > establish an ordering with other code that uses sem, and (3) process(r) > cannot panic, since if it did you'd need to see sem in the correct state. > > Since the code in question in the real example in this CL is calling into C > via cgo, the compiler has very limited visibility into what's going on. > There's no way it can gather enough evidence to prove that a reordering is > safe. I agree that it's quite unlikely (probably) to happen in reality. But to make it clear, we agree that formally it is a bug (it does not limit concurrency), but we are relying on our belief that compilers are not smart enough and do not do any kind of global program analysis (something that modern compiler can actually do) and do not have any a priory knowledge about libc functions (which they actually do have today). Have I captured this correctly? And on top of that we do not have a consistent and clear answer for Go users regarding this aspect. It's possible that some users will be forced into senti-threads like this to get some understanding about Go semantics.
Sign in to reply to this message.
On Tue, Sep 24, 2013 at 1:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Mon, Sep 23, 2013 at 10:30 PM, Russ Cox <rsc@golang.org> wrote:I agree > that it's quite unlikely (probably) to happen in reality. > > But to make it clear, we agree that formally it is a bug (it does not > limit concurrency), but we are relying on our belief that compilers > are not smart enough and do not do any kind of global program analysis > (something that modern compiler can actually do) and do not have any a > priory knowledge about libc functions (which they actually do have > today). > My point was only that even the memory model cannot justify an optimization that would break this code. If there is a bug, I think the bug is in the use of the memory model to justify hypothetical statement reordering. There are other concerns besides memory access, channel communication being the biggest one. I don't believe the compiler should be reordering across channel operations, and of course it does not. If we need to add a sentence about that to the memory model to make people happy, so be it. Russ
Sign in to reply to this message.
|