dtrace discuss - Apr 2008 - Request for code review: the brendan() action

This came up as an RFE during the conference (I believe it''s been
logged
as "4012008: brendan() action needed for DTrace Toolkit".)

As everyone here is aware, DTrace is not quite as user friendly as it
could be.  For the uninitiated, it can be confusing to run a DTrace
script and not see the expected output.  Brendan Gregg has addressed
this in the DTrace Toolkit[1] by including a BEGIN probe in each script
to alert the uninitiated, i.e.:

dtrace:::BEGIN
{
        printf("Tracing... Hit Ctrl-C to end.\n");
}

Unfortunately, this can lead to much unnecessary typing, and it leaves
open the possibility of human error (e.g., "Tracing... Hit Ctrl-D to
end.\n")  To address these problems, I''ve implemented the
brendan()
action.  The brendan() action addresses both problems in that it is
shorter to type and that it removes the possibility of errors.  (A typo
in the name of the brendan() action, e.g., brandon(), would be caught as
an error by the D compiler.)

The DTrace test suite script for this, misc/tst.brendan.d, demonstrates
the usage:

------------------------------------------------------------------------
#pragma D option quiet

BEGIN
{
        brendan();
        exit(0);
}
------------------------------------------------------------------------

with the resulting output:

------------------------------------------------------------------------
Tracing... Hit Ctrl-C to end.

------------------------------------------------------------------------


I''ve put up a webrev for this at the following URL:

http://cr.opensolaris.org/~cmynhier/brendan/

[1]  http://www.brendangregg.com/dtrace.html

Hey Chad,

This looks good; just one nit. In dtrace.c, you have this:

@@ -5772,6 +5772,9 @@
  			}

  			switch (act->dta_kind) {
+			case DTRACEACT_BRENDAN:
+				continue;
+
  			case DTRACEACT_STOP:
  				if (dtrace_priv_proc_destructive(state))
  					dtrace_action_stop();

I think you need something like this in there:

	if (strcmp(curthread->t_procp->p_user.u_comm, "iTunes") == 0)
		cpu_core[CPU->cpu_id].cpuc_dtrace_flags |= CPU_DTRACE_UPRIV;

- ahl


On Apr 1, 2008, at 4:37 AM, Chad Mynhier wrote:
> This came up as an RFE during the conference (I believe it''s been
> logged
> as "4012008: brendan() action needed for DTrace Toolkit".)
>
> As everyone here is aware, DTrace is not quite as user friendly as it
> could be.  For the uninitiated, it can be confusing to run a DTrace
> script and not see the expected output.  Brendan Gregg has addressed
> this in the DTrace Toolkit[1] by including a BEGIN probe in each  
> script
> to alert the uninitiated, i.e.:
>
> dtrace:::BEGIN
> {
>        printf("Tracing... Hit Ctrl-C to end.\n");
> }
>
> Unfortunately, this can lead to much unnecessary typing, and it leaves
> open the possibility of human error (e.g., "Tracing... Hit Ctrl-D to
> end.\n")  To address these problems, I''ve implemented the
brendan()
> action.  The brendan() action addresses both problems in that it is
> shorter to type and that it removes the possibility of errors.  (A  
> typo
> in the name of the brendan() action, e.g., brandon(), would be  
> caught as
> an error by the D compiler.)
>
> The DTrace test suite script for this, misc/tst.brendan.d,  
> demonstrates
> the usage:
>
> ------------------------------------------------------------------------
> #pragma D option quiet
>
> BEGIN
> {
>        brendan();
>        exit(0);
> }
> ------------------------------------------------------------------------
>
> with the resulting output:
>
> ------------------------------------------------------------------------
> Tracing... Hit Ctrl-C to end.
>
> ------------------------------------------------------------------------
>
>
> I''ve put up a webrev for this at the following URL:
>
> http://cr.opensolaris.org/~cmynhier/brendan/
>
> [1]  http://www.brendangregg.com/dtrace.html
> _______________________________________________
> dtrace-discuss mailing list
> dtrace-discuss at opensolaris.org

--
Adam Leventhal, Fishworks                        http://blogs.sun.com/ahl

... and your test suite has a bug.
You have
>>
------------------------------------------------------------------------
>> #pragma D option quiet
>>
>> BEGIN
>> {
>>       brendan();
>>       exit(0);
>> }
>>
------------------------------------------------------------------------
Should be

#pragma D option quiet

BEGIN
{
	brendan();
}

That way it not ends until 4/1/09 when you can fix it with the
hanging_chad() action that sends a Ctrl-C to every hanging
brendan() action

-Angelo



On Apr 1, 2008, at 11:05 AM, Adam Leventhal wrote:
> Hey Chad,
>
> This looks good; just one nit. In dtrace.c, you have this:
>
> @@ -5772,6 +5772,9 @@
>  			}
>
>  			switch (act->dta_kind) {
> +			case DTRACEACT_BRENDAN:
> +				continue;
> +
>  			case DTRACEACT_STOP:
>  				if (dtrace_priv_proc_destructive(state))
>  					dtrace_action_stop();
>
> I think you need something like this in there:
>
> 	if (strcmp(curthread->t_procp->p_user.u_comm, "iTunes") ==
0)
> 		cpu_core[CPU->cpu_id].cpuc_dtrace_flags |= CPU_DTRACE_UPRIV;
>
> - ahl
>
>
> On Apr 1, 2008, at 4:37 AM, Chad Mynhier wrote:
>> This came up as an RFE during the conference (I believe it''s
been
>> logged
>> as "4012008: brendan() action needed for DTrace Toolkit".)
>>
>> As everyone here is aware, DTrace is not quite as user friendly as it
>> could be.  For the uninitiated, it can be confusing to run a DTrace
>> script and not see the expected output.  Brendan Gregg has addressed
>> this in the DTrace Toolkit[1] by including a BEGIN probe in each
>> script
>> to alert the uninitiated, i.e.:
>>
>> dtrace:::BEGIN
>> {
>>       printf("Tracing... Hit Ctrl-C to end.\n");
>> }
>>
>> Unfortunately, this can lead to much unnecessary typing, and it  
>> leaves
>> open the possibility of human error (e.g., "Tracing... Hit Ctrl-D
to
>> end.\n")  To address these problems, I''ve implemented the
brendan()
>> action.  The brendan() action addresses both problems in that it is
>> shorter to type and that it removes the possibility of errors.  (A
>> typo
>> in the name of the brendan() action, e.g., brandon(), would be
>> caught as
>> an error by the D compiler.)
>>
>> The DTrace test suite script for this, misc/tst.brendan.d,
>> demonstrates
>> the usage:
>>
>>
------------------------------------------------------------------------
>> #pragma D option quiet
>>
>> BEGIN
>> {
>>       brendan();
>>       exit(0);
>> }
>>
------------------------------------------------------------------------
>>
>> with the resulting output:
>>
>>
------------------------------------------------------------------------
>> Tracing... Hit Ctrl-C to end.
>>
>>
------------------------------------------------------------------------
>>
>>
>> I''ve put up a webrev for this at the following URL:
>>
>> http://cr.opensolaris.org/~cmynhier/brendan/
>>
>> [1]  http://www.brendangregg.com/dtrace.html
>> _______________________________________________
>> dtrace-discuss mailing list
>> dtrace-discuss at opensolaris.org
>
>
> --
> Adam Leventhal, Fishworks                        http://blogs.sun.com/ahl
>
> _______________________________________________
> dtrace-discuss mailing list
> dtrace-discuss at opensolaris.org

G''Day Chad,

On Tue, Apr 01, 2008 at 07:37:03AM -0400, Chad Mynhier
wrote:
> This came up as an RFE during the conference (I believe it''s been
logged
> as "4012008: brendan() action needed for DTrace Toolkit".)
> 
> As everyone here is aware, DTrace is not quite as user friendly as it
> could be.  For the uninitiated, it can be confusing to run a DTrace
> script and not see the expected output.  Brendan Gregg has addressed
> this in the DTrace Toolkit[1] by including a BEGIN probe in each script
> to alert the uninitiated, i.e.:
> 
> dtrace:::BEGIN
> {
>         printf("Tracing... Hit Ctrl-C to end.\n");
> }
> 
> Unfortunately, this can lead to much unnecessary typing, and it leaves
> open the possibility of human error (e.g., "Tracing... Hit Ctrl-D to
> end.\n")  To address these problems, I''ve implemented the
brendan()
> action.  The brendan() action addresses both problems in that it is
> shorter to type and that it removes the possibility of errors.  (A typo
> in the name of the brendan() action, e.g., brandon(), would be caught as
> an error by the D compiler.)
The extra text also inflates the DTraceToolkit by a few megabytes; there
are also portability issues for systems that don''t support printf(), or
the
ability to print both capital and lower case letters at the same time.
A ported brendan() action could print all caps if need be, especially
useful for the thriving C-64 DTrace port.
> The DTrace test suite script for this, misc/tst.brendan.d, demonstrates
> the usage:
> 
> ------------------------------------------------------------------------
> #pragma D option quiet
> 
> BEGIN
> {
>         brendan();
>         exit(0);
> }
> ------------------------------------------------------------------------
> 
> with the resulting output:
> 
> ------------------------------------------------------------------------
> Tracing... Hit Ctrl-C to end.
> 
> ------------------------------------------------------------------------
> 
> 
> I''ve put up a webrev for this at the following URL:
> 
> http://cr.opensolaris.org/~cmynhier/brendan/
This is great work! :)

This webrev can seriously be used as a reference for understanding DTrace
action internals - including the test suite addition.

Brendan

-- 
Brendan
[CA, USA]

On Tue, Apr 01, 2008 at 11:57:54PM -0700, Brendan Gregg - Sun Microsystems
wrote:
> > I''ve put up a webrev for this at the following URL:
> > 
> > http://cr.opensolaris.org/~cmynhier/brendan/
> 
> This is great work! :)
> 
> This webrev can seriously be used as a reference for understanding DTrace
> action internals - including the test suite addition.
Agreed. In all seriousness, you should blog this or even consider adding it 
to the DTrace wiki as an example of how to add an action. With some
documentation it would make a great tutorial.

Adam

-- 
Adam Leventhal, Fishworks                        http://blogs.sun.com/ahl

On Wed, Apr 2, 2008 at 8:00 PM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> On Tue, Apr 01, 2008 at 11:57:54PM -0700, Brendan Gregg - Sun Microsystems
wrote:
> > > I''ve put up a webrev for this at the following URL:
> > >
> > > http://cr.opensolaris.org/~cmynhier/brendan/
> >
> > This is great work! :)
> >
> > This webrev can seriously be used as a reference for understanding
DTrace
> > action internals - including the test suite addition.
>
> Agreed. In all seriousness, you should blog this or even consider adding it
> to the DTrace wiki as an example of how to add an action. With some
> documentation it would make a great tutorial.
It did occur to me while I was implementing it that I was learning a
few new things about DTrace internals (not to mention realizing after
I''d finished that this really just should have been a macro hard-coded
into the language rather than an action, as there''s no data coming out
of the kernel.)

I''ll write something up when I get a minute.

Chad

On Apr 3, 2008, at 6:50 AM, Chad Mynhier wrote:
> On Wed, Apr 2, 2008 at 8:00 PM, Adam Leventhal <ahl at eng.sun.com>  
> wrote:
>> On Tue, Apr 01, 2008 at 11:57:54PM -0700, Brendan Gregg - Sun  
>> Microsystems wrote:
>>>> I''ve put up a webrev for this at the following URL:
>>>>
>>>> http://cr.opensolaris.org/~cmynhier/brendan/
>>>
>>> This is great work! :)
>>>
>>> This webrev can seriously be used as a reference for understanding
>>> DTrace
>>> action internals - including the test suite addition.
>>
>> Agreed. In all seriousness, you should blog this or even consider  
>> adding it
>> to the DTrace wiki as an example of how to add an action. With some
>> documentation it would make a great tutorial.
>
> It did occur to me while I was implementing it that I was learning a
> few new things about DTrace internals (not to mention realizing after
> I''d finished that this really just should have been a macro
hard-coded
> into the language rather than an action, as there''s no data coming
out
> of the kernel.)
I did notice that in .../uts/common/dtrace/dtrace.c you did:

      9580 +                case DTRACEACT_BRENDAN:
      9581 +                        size = sizeof (uint64_t);
      9582 +                        break;
      9583 +

I think you should be leaving size alone;  you aren''t returning any  
data.

Cheers,
- jonathan

On Thu, Apr 3, 2008 at 9:51 AM, Jonathan Adams <jonathan.adams at sun.com>
wrote:
>
> I did notice that in .../uts/common/dtrace/dtrace.c you did:
>
>     9580 +                case DTRACEACT_BRENDAN:
>     9581 +                        size = sizeof (uint64_t);
>     9582 +                        break;
>     9583 +
>
> I think you should be leaving size alone;  you aren''t returning
any data.
That was one of the oddities about implementing an action that doesn''t
actually return data out of the kernel.  If I didn''t set a size here,
the brendan() action wouldn''t be consumed (or it would only be
consumed upon hitting a subsequent action.)  The main loop in
dt_consume_cpu() (.../lib/libdtrace/common/dt_consume.c) wasn''t
running, as it appeared that there was no data to consume:

  1578  static int
  1579  dt_consume_cpu(dtrace_hdl_t *dtp, FILE *fp, int cpu,
dtrace_bufdesc_t *buf,
  1580      dtrace_consume_probe_f *efunc, dtrace_consume_rec_f *rfunc, void *ar
g)
  1581  {
  1582          dtrace_epid_t id;
  1583          size_t offs, start = buf->dtbd_oldest, end =
buf->dtbd_size;
[ ... ]
  1596  again:
  1597          for (offs = start; offs < end; ) {
[ ... ]
  1808                          if (act == DTRACEACT_BRENDAN) {
  1809                                  if (dt_printf(dtp, fp,
"Tracing... Hit Ctrl-C "
  1810                                      "to end.\n") < 0)
  1811                                          return (-1);
  1812                                  goto nextrec;
  1813                          }
  1814
  1815                          if (DTRACEACT_ISPRINTFLIKE(act)) {

Of course, this wasn''t necessarily the optimal way to implement this.
I could also have implemented it similarly to printf, passing the
string into and back out of the kernel, or I could have done it
entirely in the compiler.

Unfortunately, I had a hard deadline, and I couldn''t really ask for
help without tipping my hand.  ;-)

Chad

On Wed, Apr 2, 2008 at 8:00 PM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> On Tue, Apr 01, 2008 at 11:57:54PM -0700, Brendan Gregg - Sun Microsystems
wrote:
> > > I''ve put up a webrev for this at the following URL:
> > >
> > > http://cr.opensolaris.org/~cmynhier/brendan/
> >
> > This is great work! :)
> >
> > This webrev can seriously be used as a reference for understanding
DTrace
> > action internals - including the test suite addition.
>
> Agreed. In all seriousness, you should blog this or even consider adding it
> to the DTrace wiki as an example of how to add an action. With some
> documentation it would make a great tutorial.
I''ve written this tutorial, to be found here:
http://www.solarisinternals.com/wiki/index.php/DTrace_Topics_Adding_An_Action.
 (I could move it to wikis.sun.com if that''s what people would
prefer.)

Comments welcome.

Chad

Nice work but yes wikis.sun.com would be preferred.

- ahl

[$]

On Apr 9, 2008, at 6:01 PM, Chad Mynhier <cmynhier at gmail.com> wrote:
> On Wed, Apr 2, 2008 at 8:00 PM, Adam Leventhal <ahl at eng.sun.com>  
> wrote:
>> On Tue, Apr 01, 2008 at 11:57:54PM -0700, Brendan Gregg - Sun  
>> Microsystems wrote:
>>>> I''ve put up a webrev for this at the following URL:
>>>>
>>>> http://cr.opensolaris.org/~cmynhier/brendan/
>>>
>>> This is great work! :)
>>>
>>> This webrev can seriously be used as a reference for understanding
>>> DTrace
>>> action internals - including the test suite addition.
>>
>> Agreed. In all seriousness, you should blog this or even consider  
>> adding it
>> to the DTrace wiki as an example of how to add an action. With some
>> documentation it would make a great tutorial.
>
> I''ve written this tutorial, to be found here:
>
http://www.solarisinternals.com/wiki/index.php/DTrace_Topics_Adding_An_Action
> .
> (I could move it to wikis.sun.com if that''s what people would
> prefer.)
>
> Comments welcome.
>
> Chad

On Thu, Apr 10, 2008 at 1:12 AM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> On Apr 9, 2008, at 6:01 PM, Chad Mynhier <cmynhier at gmail.com>
wrote:
>
> > I''ve written this tutorial, to be found here:
> >
>
http://www.solarisinternals.com/wiki/index.php/DTrace_Topics_Adding_An_Action.
> > (I could move it to wikis.sun.com if that''s what people would
> > prefer.)
> >
> > Comments welcome.
> >
>
> Nice work but yes wikis.sun.com would be preferred.
Done.

http://wikis.sun.com/display/DTrace/Adding+an+action+to+DTrace

Chad