Issue 254780043: i#1703 cache simulator: ensure tracer atomic pipe writes
|
Created:
8 years, 10 months ago by zhaoqin Modified:
8 years, 9 months ago CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#1703 cache simulator: ensure tracer atomic pipe writes
This CL change tracer pipe write to ensure atomic pipe writes.
---------------
Patch Set 1 #
Total comments: 19
Patch Set 2 : PTAL #Patch Set 3 : Fixed possible non-atomic pipe write bug #
Total comments: 3
Patch Set 4 : final #Patch Set 5 : Committed #
MessagesTotal messages: 13
Doesn't seem quite correct to me. Grammar problems in commit msg. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:58: // Linux atomic pipe write buffer size Seems cleaner to have this provided by named_pipe_t https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:58: // Linux atomic pipe write buffer size What about Windows? Another reason to not have it here. The whole point of named_pipe_t is to have a cross-platform pipe abstraction. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:156: // Split to multiple pipe writes if too many entries to ensure aotmic. spelling https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:164: // Re-meit thread entry header spelling https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:165: pipe_start = pipe_end - BUF_HDR_SLOTS_SIZE; assert that this doesn't underflow https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so can't it cross that boundary in the first of 2 data entries (or the 2nd if no more instr entries) and you'll go and try to write something too large here? https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:179: memset(data->buf_base, 0, TRACE_BUF_SIZE); Why do we need to do this memset?
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1703 cache simulator: split up tracer pipe writes This CL splits up tracer pipe writes into multiple writes to ensure atomic pipe operations. Review-URL: https://codereview.appspot.com/254780043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:58: // Linux atomic pipe write buffer size On 2015/07/01 14:50:31, bruening wrote: > Seems cleaner to have this provided by named_pipe_t I am not 100% sure about this, because how to split buffer should be out side of named_pipe_t scope. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:58: // Linux atomic pipe write buffer size On 2015/07/01 14:50:31, bruening wrote: > What about Windows? Another reason to not have it here. The whole point of > named_pipe_t is to have a cross-platform pipe abstraction. I cannot find the limit on Windows. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:156: // Split to multiple pipe writes if too many entries to ensure aotmic. On 2015/07/01 14:50:31, bruening wrote: > spelling Done. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:164: // Re-meit thread entry header On 2015/07/01 14:50:31, bruening wrote: > spelling Done. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:165: pipe_start = pipe_end - BUF_HDR_SLOTS_SIZE; On 2015/07/01 14:50:31, bruening wrote: > assert that this doesn't underflow Done. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe On 2015/07/01 14:50:31, bruening wrote: > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so can't > it cross that boundary in the first of 2 data entries (or the 2nd if no more > instr entries) and you'll go and try to write something too large here? we assume instr entries are really frequent, so they won't be largely apart. https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:179: memset(data->buf_base, 0, TRACE_BUF_SIZE); On 2015/07/01 14:50:31, bruening wrote: > Why do we need to do this memset? we load the buffer content and jecxz to skip the clean call. So the trace buffer must be 0.
Sign in to reply to this message.
https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe On 2015/07/01 20:06:46, zhaoqin wrote: > On 2015/07/01 14:50:31, bruening wrote: > > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so > can't > > it cross that boundary in the first of 2 data entries (or the 2nd if no more > > instr entries) and you'll go and try to write something too large here? > > we assume instr entries are really frequent, so they won't be largely apart. That does not seem good enough: you can still have a too-big write that won't be atomic, right? https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:179: memset(data->buf_base, 0, TRACE_BUF_SIZE); On 2015/07/01 20:06:46, zhaoqin wrote: > On 2015/07/01 14:50:31, bruening wrote: > > Why do we need to do this memset? > > we load the buffer content and jecxz to skip the clean call. So the trace buffer > must be 0. Please add a comment about it here
Sign in to reply to this message.
On 2015/07/01 20:55:55, bruening wrote: > https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... > File clients/drcachesim/tracer/tracer.cpp (right): > > https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... > clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe > On 2015/07/01 20:06:46, zhaoqin wrote: > > On 2015/07/01 14:50:31, bruening wrote: > > > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so > > can't > > > it cross that boundary in the first of 2 data entries (or the 2nd if no more > > > instr entries) and you'll go and try to write something too large here? > > > > we assume instr entries are really frequent, so they won't be largely apart. > > That does not seem good enough: you can still have a too-big write that won't be > atomic, right? still not sure what case could cause large write. > > https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... > clients/drcachesim/tracer/tracer.cpp:179: memset(data->buf_base, 0, > TRACE_BUF_SIZE); > On 2015/07/01 20:06:46, zhaoqin wrote: > > On 2015/07/01 14:50:31, bruening wrote: > > > Why do we need to do this memset? > > > > we load the buffer content and jecxz to skip the clean call. So the trace > buffer > > must be 0. > > Please add a comment about it here done
Sign in to reply to this message.
https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe On 2015/07/01 20:55:54, bruening wrote: > On 2015/07/01 20:06:46, zhaoqin wrote: > > On 2015/07/01 14:50:31, bruening wrote: > > > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so > > can't > > > it cross that boundary in the first of 2 data entries (or the 2nd if no more > > > instr entries) and you'll go and try to write something too large here? > > > > we assume instr entries are really frequent, so they won't be largely apart. > > That does not seem good enough: you can still have a too-big write that won't be > atomic, right? See my first comment for the explanation: consider an instr entry that gets really close to PIPE_BUF. Its data entry pushes it past PIPE_BUF. It's the final entry in the buffer, so you go and write the whole thing, which is too big to be atomic.
Sign in to reply to this message.
https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe On 2015/07/02 17:01:59, bruening wrote: > On 2015/07/01 20:55:54, bruening wrote: > > On 2015/07/01 20:06:46, zhaoqin wrote: > > > On 2015/07/01 14:50:31, bruening wrote: > > > > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, so > > > can't > > > > it cross that boundary in the first of 2 data entries (or the 2nd if no > more > > > > instr entries) and you'll go and try to write something too large here? > > > > > > we assume instr entries are really frequent, so they won't be largely apart. > > > > That does not seem good enough: you can still have a too-big write that won't > be > > atomic, right? > > See my first comment for the explanation: consider an instr entry that gets > really close to PIPE_BUF. Its data entry pushes it past PIPE_BUF. It's the > final entry in the buffer, so you go and write the whole thing, which is too big > to be atomic. No, look at line 158-169. On seeing a TYPE_INSTR, we check if the buffer BEFORE this ref exceed the PIPE_BUF size. If exceed, it will do a pipe write with previous pipe_end, otherwise we only move pipe_end to this point, and the [pipe_start, pipe_end) does not include current entry, so won't overflow no matter how large current entry with data entry.
Sign in to reply to this message.
https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/1/clients/drcachesim/tracer/tra... clients/drcachesim/tracer/tracer.cpp:172: // Write the rest to pipe On 2015/07/04 21:20:56, qin.zhao wrote: > On 2015/07/02 17:01:59, bruening wrote: > > On 2015/07/01 20:55:54, bruening wrote: > > > On 2015/07/01 20:06:46, zhaoqin wrote: > > > > On 2015/07/01 14:50:31, bruening wrote: > > > > > This seems buggy. You're only checking vs PIPE_BUF prior to an instr, > so > > > > can't > > > > > it cross that boundary in the first of 2 data entries (or the 2nd if no > > more > > > > > instr entries) and you'll go and try to write something too large here? > > > > > > > > we assume instr entries are really frequent, so they won't be largely > apart. > > > > > > That does not seem good enough: you can still have a too-big write that > won't > > be > > > atomic, right? > > > > See my first comment for the explanation: consider an instr entry that gets > > really close to PIPE_BUF. Its data entry pushes it past PIPE_BUF. It's the > > final entry in the buffer, so you go and write the whole thing, which is too > big > > to be atomic. > > No, look at line 158-169. > On seeing a TYPE_INSTR, we check if the buffer BEFORE this ref exceed the > PIPE_BUF size. > If exceed, it will do a pipe write with previous pipe_end, otherwise we only > move pipe_end to this point, and the [pipe_start, pipe_end) does not include > current entry, so won't overflow no matter how large current entry with data > entry. As discussed, you're not using pipe_end for the final write, so the bug is there.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1703 cache simulator: split up tracer pipe writes This CL splits up tracer pipe writes into multiple writes to ensure atomic pipe operations. Review-URL: https://codereview.appspot.com/254780043 ---------------
Sign in to reply to this message.
LGTM https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/common... File clients/drcachesim/common/named_pipe.h (right): https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/common... clients/drcachesim/common/named_pipe.h:86: size_t atomic_write_size(); Please label as const. Maybe also add "get_" to name to make it even clearer it's const? Maybe not, too long then. https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/common... File clients/drcachesim/common/named_pipe_win.cpp (right): https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/common... clients/drcachesim/common/named_pipe_win.cpp:62: // FIXME i#1703: what's the atomic pipe write limit? Use 1727, not 1703 https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/tracer... File clients/drcachesim/tracer/tracer.cpp (right): https://codereview.appspot.com/254780043/diff/40001/clients/drcachesim/tracer... clients/drcachesim/tracer/tracer.cpp:166: // We can only split before TRACE_TYPE_INSTR. This assumes few data entries in between instr entries: so will fail if ever don't unroll "rep cmps", e.g. Worth a comment on assumption. I guess the atomic write assert would fail then at least.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1703 cache simulator: split up tracer pipe writes This CL splits up tracer pipe writes into multiple writes to ensure atomic pipe operations. Review-URL: https://codereview.appspot.com/254780043 ---------------
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/9c06b61ff7cc52399900b6f03dd8196... Final commit log: --------------- i#1703 cache simulator: split up tracer pipe writes This CL splits up tracer pipe writes into multiple writes to ensure atomic pipe operations. Review-URL: https://codereview.appspot.com/254780043 ---------------
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
