[webkit-dev] Atomic read/write operations and thread safety
I have a question about whether WebCore code makes assumptions about the atomicity of certain read/write operations. The particular instance I'm interested in is IconDatabase, where m_threadTerminationRequested is written to from the main thread (in close() for example) and read from the DB thread (in syncThreadMainLoop()) without any locking. It looks like the code is written such that there's no particular synchronization requirement in terms of the order in which the value is set and read. But there's a risk of a garbled read in the case of a simultaneous read and write from the two threads. I assume that the lack of locking is an intentional decision, based on the fact that on all relevant architectures, a boolean read/write is atomic? Is this a general WebCore policy? It would be great if somebody could confirm this, or point out why I'm wrong. Thanks, Steve -- Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ Registered in England Number: 3977902 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
On Dec 15, 2010, at 4:28 AM, Steve Block wrote: > I have a question about whether WebCore code makes assumptions about > the atomicity of certain read/write operations. > > The particular instance I'm interested in is IconDatabase, where > m_threadTerminationRequested is written to from the main thread (in > close() for example) and read from the DB thread (in > syncThreadMainLoop()) without any locking. It looks like the code is > written such that there's no particular synchronization requirement in > terms of the order in which the value is set and read. But there's a > risk of a garbled read in the case of a simultaneous read and write > from the two threads. > > I assume that the lack of locking is an intentional decision, based on > the fact that on all relevant architectures, a boolean read/write is > atomic? Correct - or, at least, at the time it was correct. Have you discovered a relevant architecture where it is no longer correct? > Is this a general WebCore policy? It would be great if somebody could > confirm this, or point out why I'm wrong. I wouldn't be surprised if there were other examples of an unprotected thread shared boolean in either WebCore or any platform's WebKit. I *would* be surprised if there was an example of *any other data type* that was shared between threads unprotected. That said, I don't think we have any general WebCore policies about multithread practices. There might be value in such a document. ~Brady > > Thanks, > Steve > > -- > Google UK Limited > Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ > Registered in England Number: 3977902 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
On Dec 15, 2010, at 8:47 AM, Brady Eidson wrote: > > On Dec 15, 2010, at 4:28 AM, Steve Block wrote: > >> I have a question about whether WebCore code makes assumptions about >> the atomicity of certain read/write operations. >> >> The particular instance I'm interested in is IconDatabase, where >> m_threadTerminationRequested is written to from the main thread (in >> close() for example) and read from the DB thread (in >> syncThreadMainLoop()) without any locking. It looks like the code is >> written such that there's no particular synchronization requirement in >> terms of the order in which the value is set and read. But there's a >> risk of a garbled read in the case of a simultaneous read and write >> from the two threads. >> >> I assume that the lack of locking is an intentional decision, based on >> the fact that on all relevant architectures, a boolean read/write is >> atomic? > > Correct - or, at least, at the time it was correct. Have you discovered a > relevant architecture where it is no longer correct? > >> Is this a general WebCore policy? It would be great if somebody could >> confirm this, or point out why I'm wrong. > > I wouldn't be surprised if there were other examples of an unprotected thread > shared boolean in either WebCore or any platform's WebKit. In hindsight I realize my response involved assumptions about this code that were known to me but not to the general audience, so I'll followup more thoroughly. The boolean in question isn't both checked and set at the same time - it's not an acquired resource. One thread sets it, the other checks it. My belief is that this is safe for booleans, but I would love to hear where I'm wrong. Additionally, while there isn't a guard specifically around the boolean, there is the "m_syncLock" Mutex which is implicitly guarding operations on this bool, so the "thread safety of a bool?" argument becomes somewhat moot. If there's a specific race condition in this code you've spotted, I'd love to hear how it would manifest! Thanks, ~Brady > I *would* be surprised if there was an example of *any other data type* that > was shared between threads unprotected. > > That said, I don't think we have any general WebCore policies about > multithread practices. There might be value in such a document. > > ~Brady > >> >> Thanks, >> Steve >> >> -- >> Google UK Limited >> Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ >> Registered in England Number: 3977902 > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
Hi, Am 15.12.2010 18:43, schrieb Brady Eidson: On Dec 15, 2010, at 8:47 AM, Brady Eidson wrote: On Dec 15, 2010, at 4:28 AM, Steve Block wrote: I wouldn't be surprised if there were other examples of an unprotected thread shared boolean in either WebCore or any platform's WebKit. In hindsight I realize my response involved assumptions about this code that were known to me but not to the general audience, so I'll followup more thoroughly. The boolean in question isn't both checked and set at the same time - it's not an acquired resource. One thread sets it, the other checks it. My belief is that this is safe for booleans, but I would love to hear where I'm wrong. I have seen this particular technique quite often before. The only thing one needs to watch out for is to declare such a boolean volatile (which I believe this code does, if memory serves). Otherwise the thread which polls the condition may read from a cached location and miss the change. Worst that can happen on a hypothecial architecture where writing a byte is not atomic is that the changed condition takes affect one loop cycle later. Best regards, -Stephan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
On Dec 15, 2010, at 11:11 AM, Steve Block wrote: > Thanks for the reply Brady. > >> The boolean in question isn't both checked and set at the same time - it's >> not an acquired resource. One >> thread sets it, the other checks it. > I don't follow. If it's set from one thread and checked from another > thread without locks, how can you guarantee this (other than with > application logic)? I don't understand your question - "how can you guarantee this?" >> Additionally, while there isn't a guard specifically around the boolean, >> there is the "m_syncLock" Mutex >> which is implicitly guarding operations on this bool, so the "thread safety >> of a bool?" argument becomes >> somewhat moot. > Ah, so you're saying that there's application logic using m_syncLock > to prevent the boolean from being written and read at the same time? I > didn't spot that. It doesn't matter if it's written and read at the same time in this case - but after every write on the main thread, the background thread is signaled to read, so that read will always happen after the write. ~Brady ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
>>> The boolean in question isn't both checked and set at the same time - it's >>> not an acquired resource. One >>> thread sets it, the other checks it. >> I don't follow. If it's set from one thread and checked from another >> thread without locks, how can you guarantee this (other than with >> application logic)? > I don't understand your question - "how can you guarantee this?" I meant 'how can you guarantee that the boolean isn't both checked and set at the same time, given that you don't use locks'. But it looks like you're saying that this is guaranteed by higher-level application logic, as you describe below. > It doesn't matter if it's written and read at the same time in this case - > but after every write on the main > thread, the background thread is signaled to read, so that read will always > happen after the write. OK, makes sense. Thanks, Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
Thanks for the reply Brady. > The boolean in question isn't both checked and set at the same time - it's > not an acquired resource. One > thread sets it, the other checks it. I don't follow. If it's set from one thread and checked from another thread without locks, how can you guarantee this (other than with application logic)? > Additionally, while there isn't a guard specifically around the boolean, > there is the "m_syncLock" Mutex > which is implicitly guarding operations on this bool, so the "thread safety > of a bool?" argument becomes > somewhat moot. Ah, so you're saying that there's application logic using m_syncLock to prevent the boolean from being written and read at the same time? I didn't spot that. > I have seen this particular technique quite often before. The only thing one > needs to watch out for is to declare such a boolean volatile (which I > believe this code does, if memory serves). Otherwise the thread which polls > the condition may read from a cached location and miss the change. The boolean in question isn't volatile. This isn't a problem because as I mentioned, there's no requirement for a given write to be picked up by a particular read. If a read 'misses' a write, we'll pick it up later. Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
On Dec 15, 2010, at 8:47 AM, Brady Eidson wrote: > > On Dec 15, 2010, at 4:28 AM, Steve Block wrote: > >> I have a question about whether WebCore code makes assumptions about >> the atomicity of certain read/write operations. >> >> The particular instance I'm interested in is IconDatabase, where >> m_threadTerminationRequested is written to from the main thread (in >> close() for example) and read from the DB thread (in >> syncThreadMainLoop()) without any locking. It looks like the code is >> written such that there's no particular synchronization requirement in >> terms of the order in which the value is set and read. But there's a >> risk of a garbled read in the case of a simultaneous read and write >> from the two threads. >> >> I assume that the lack of locking is an intentional decision, based on >> the fact that on all relevant architectures, a boolean read/write is >> atomic? > > Correct - or, at least, at the time it was correct. Have you discovered a > relevant architecture where it is no longer correct? > >> Is this a general WebCore policy? It would be great if somebody could >> confirm this, or point out why I'm wrong. > > I wouldn't be surprised if there were other examples of an unprotected thread > shared boolean in either WebCore or any platform's WebKit. > > I *would* be surprised if there was an example of *any other data type* that > was shared between threads unprotected. > > That said, I don't think we have any general WebCore policies about > multithread practices. There might be value in such a document. One possibility is to check the perf impact os using atomic read/write primitives; if it doesn't have a performance cost, it might be safer to do it that way. - Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
15.12.2010, в 10:36, Stephan Aßmus написал(а): > The only thing one needs to watch out for is to declare such a boolean > volatile (which I believe this code does, if memory serves). Otherwise the > thread which polls the condition may read from a cached location and miss the > change. Worst that can happen on a hypothecial architecture where writing a > byte is not atomic is that the changed condition takes affect one loop cycle > later. The behavior of volatile depends on the compiler - there is no guarantee that a memory barrier will be emitted. Historically, volatile existed not for multiprocessor systems, but for special addresses in address space (like memory mapped I/O), where it is important to actually access the address each time, and not than keep a local variable in a boolean. There is code in WebKit that doesn't care about the other thread immediately seeing the change of a boolean value, and just hopes that it will propagate soon enough in practice. I remember implementing worker thread termination in that way. This code is of course formally wrong. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Atomic read/write operations and thread safety
In MSDN there is a widely cited article about lockless programming on x86 and Xbox: http://msdn.microsoft.com/en-us/library/ee418650(v=vs.85).aspx In short: using boolean as a flag for thread termination is OK. Using boolean as a guard for multi-threaded access to data isn't OK, as read / write reordering can be made both by C++ compiler and CPU. On Thu, Dec 16, 2010 at 00:34, Alexey Proskuryakov wrote: > > 15.12.2010, в 10:36, Stephan Aßmus написал(а): > >> The only thing one needs to watch out for is to declare such a boolean >> volatile (which I believe this code does, if memory serves). Otherwise the >> thread which polls the condition may read from a cached location and miss >> the change. Worst that can happen on a hypothecial architecture where >> writing a byte is not atomic is that the changed condition takes affect one >> loop cycle later. > > > The behavior of volatile depends on the compiler - there is no guarantee that > a memory barrier will be emitted. Historically, volatile existed not for > multiprocessor systems, but for special addresses in address space (like > memory mapped I/O), where it is important to actually access the address each > time, and not than keep a local variable in a boolean. > > There is code in WebKit that doesn't care about the other thread immediately > seeing the change of a boolean value, and just hopes that it will propagate > soon enough in practice. I remember implementing worker thread termination in > that way. This code is of course formally wrong. > > - WBR, Alexey Proskuryakov > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
