Hey Bryan, Cool; looks good to me.
Adam On Tue, Jul 12, 2011 at 10:44 PM, Bryan Cantrill <br...@joyent.com> wrote: > On Tue, Jul 12, 2011 at 5:25 PM, Adam Leventhal <a...@delphix.com> wrote: >>>> I actually meant a negative test case. >>> >>> As I imagine you saw, there's already one there that uses speculative >>> tracing and verifies that enablings are not reaped: >>> >>> usr/src/cmd/dtrace/test/tst/common/usdt/tst.noreap.ksh >>> >>> Having a test that uses ring buffering is certainly possible, it's >>> just a more complicated test to write. (I'm currently cheating a bit >>> by using one enabling to kick off another.) But I'm happy to add it >>> if you'd like to see it... >> >> Yes. I saw that test case. It was just a suggestion to exercise the >> other path in your code, but it's up to you. > > I added that test and integrated into our repo; patch is attached. > >>>> Would that be worth a one- or two-line comment? >>> >>> There's already the comment that it's padded out to avoid false >>> sharing; does it merit more than that? >> >> I guess that's sufficient, but I needed to remind myself that these >> were per-CPU data structures. Your call. > > I'm going to leave that one as it is; in addition to the comment next > to the member, the block comment immediately above the structure also > spells that out: > > /* > * DTrace Buffers > * > * Principal buffers, aggregation buffers, and speculative buffers are all > * managed with the dtrace_buffer structure. By default, this structure > * includes twin data buffers -- dtb_tomax and dtb_xamot -- that serve as the > * active and passive buffers, respectively. For speculative buffers, > * dtb_xamot will be NULL; for "ring" and "fill" buffers, dtb_xamot will point > * to a scratch buffer. For all buffer types, the dtrace_buffer structure is > * always allocated on a per-CPU basis; a single dtrace_buffer structure is > * never shared among CPUs. (That is, there is never true sharing of the > * dtrace_buffer structure; to prevent false sharing of the structure, it must > * always be aligned to the coherence granularity -- generally 64 bytes.) > * > ... > > Let me know if you have any other comments; otherwise, I'll assume > that you've moved on to reviewing unprivileged tick probes. ;) > > - Bryan > -- Adam Leventhal, Delphix http://dtrace.org/blogs/ahl 275 Middlefield Road, Suite 50 Menlo Park, CA 94025 http://www.delphix.com _______________________________________________ dtrace-discuss mailing list dtrace-discuss@opensolaris.org
