[DTrace-devel] [PATCH] dtprobed: handle multiple providers in a single piece of DOF

Nick Alcock nick.alcock at oracle.com
Thu Nov 14 20:38:33 UTC 2024 

On 14 Nov 2024, Eugene Loh told this:

> I'm not familiar with this code, but here is some amateur feedback...

:)

> On 11/14/24 11:03, Nick Alcock wrote:
>> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
>> @@ -24,6 +24,7 @@
>>    *     DIT_ARGS_XLAT (1, optional)
>>    *     DIT_ARGS_MAP (1, optional)
>>    *     DIT_TRACEPOINT (any number >= 1)
>> + * DIT_EOF (no more providers, last record)
>>    *
>>    * The dof_parsed.provider.flags word indicates the presence of the
>>    * various optional args records in the following stream (you can rely on
>
> The new line has indentation inconsistent with the pre-existing lines.

Intentional. The full context might make this clearer:

 * DIT_PROVIDER (at least 1, which contains...)
 *   DIT_PROBE (at least 1, each of which has...)
 *     DIT_ARGS_NATIVE (1, optional)
 *     DIT_ARGS_XLAT (1, optional)
 *     DIT_ARGS_MAP (1, optional)
 *     DIT_TRACEPOINT (any number >= 1)
 * DIT_EOF (no more providers, last record)

i.e. this is showing that "this is at the level of the DIT_PROVIDER
(after all DIT_PROVIDERs) through indentation. It is *not* at the level
of the DIT_TRACEPOINT.

>> diff --git a/test/triggers/Build b/test/triggers/Build
>> @@ -13,7 +13,8 @@ EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie
>>       ustack-tst-spin ustack-tst-mtspin \
>>       visible-constructor visible-constructor-static visible-constructor-static-unstripped
>>   -EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer usdt-tst-special
>> +EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer \
>> +    usdt-tst-multiprovider usdt-tst-special
>>   EXTERNAL_64BIT_TRIGGERS += $(EXTERNAL_64BIT_SDT_TRIGGERS)
>>     EXTERNAL_32BIT_TRIGGERS := visible-constructor-32
>
> Okay.  Introducing a new test/triggers that is used by only one test is... well, I guess it's a judgment call.

It makes the test way simpler :) and if we do add another test that
actually fires these probes, it won't be used by only one test.

>> diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
>> new file mode 100644
>> @@ -0,0 +1,12 @@
>> +/*
>> + * Oracle Linux DTrace.
>> + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> + * Licensed under the Universal Permissive License v 1.0 as shown at
>> + * http://oss.oracle.com/licenses/upl.
>> + */
>> +
>> +/* @@skip: provider declaration - not a test */
>
> I don't think the @@skip is needed for something inside test/triggers.

I copied it from other things in test/triggers just in case. You're
probably right. I'll submit another patch dropping it from everything in
test/triggers.

(I think it originally appeared in usdt-tst-forker-prov.d, and
replicated from there.)

>> +
>> +provider prova { probe entrya(); };
>> +provider provb { probe entryb(); };
>> +provider provc { probe entryc(); };
>
> We should stress test more:
> *)   multiple probes per provider

See v2 of this series! this had... er... fallout.

> *)   some probes with same and different names as in other providers
> *)   probe args (including probes in different providers handling args differently)

I don't really see how this could possibly go wrong -- nothing after the
mid-parser stage can possibly have cross-provider interference as far as
I can see, and that stage doesn't know or care about any of this stuff.

But ICBW...

>> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
>> new file mode 100644
>> index 0000000000000..430c0cdd2cf4d
>> --- /dev/null
>> +++ b/test/triggers/usdt-tst-multiprovider.c
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Oracle Linux DTrace.
>> + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> + * Licensed under the Universal Permissive License v 1.0 as shown at
>> + * http://oss.oracle.com/licenses/upl.
>> + */
>> +
>> +/*
>> + * A trigger with multiple providers in it.
>> + */
>> +#include <stdio.h>
>
> stdio.h not needed

Agreed.

>> +#include <unistd.h>
>
> only needed for usleep()

... ack.

>> +#include "usdt-tst-multiprovider-prov.h"
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	PROVA_ENTRYA();
>> +	PROVB_ENTRYB();
>> +	PROVC_ENTRYC();
>> +	usleep(5 * 1000 * 1000);
>
> usleep not needed and just taking up test time.  It was originally in some tests that were checking /run/dtrace "manually".

Ahhh. Dropped.

>> +	return 0;
>> +}
>> diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
>> new file mode 100644
>> index 0000000000000..b83f559a7e7ff
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.r
>> @@ -0,0 +1,3 @@
>> +ID provaPID usdt-tst-multiprovider main entrya
>> +ID provbPID usdt-tst-multiprovider main entryb
>> +ID provcPID usdt-tst-multiprovider main entryc
>> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
>> new file mode 100755
>> index 0000000000000..ae8493e3d5be4
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.r.p
>> @@ -0,0 +1,2 @@
>> +#!/bin/sh
>> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
>
> Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID *PROVIDER *MODULE *FUNCTION *NAME$'.

Why? We're not dependent on the format of the header, we just want to
get rid of it. Having the test fail because the header we don't care
about changed layout seems wrong.

I might just drop the first line with tail -n +1...

> This whole thing would be clearer to me as a single awk, but maybe that's just me.

How? Piles of gmatches?

> Comments might be nice.  I'm not sure how comfortably people read regex...  I need to concentrate hard to figure it out.

Honestly this is just doing some "replace changing numbers" stuff like a
hundred other .p's in the testsuite. Looking at the .r makes it obvious
what it's up to.

>> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
>> new file mode 100755
>> index 0000000000000..d5b72c2be77ec
>> --- /dev/null
>> +++ b/test/unittest/usdt/tst.multiprovider.sh
>> @@ -0,0 +1,15 @@
>> +#!/bin/bash
>> +#
>> +# Oracle Linux DTrace.
>> +# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> +# Licensed under the Universal Permissive License v 1.0 as shown at
>> +# http://oss.oracle.com/licenses/upl.
>> +#
>> +if [ $# != 1 ]; then
>> +	echo expected one argument: '<'dtrace-path'>'
>> +	exit 2
>> +fi
>> +
>> +dtrace=$1
>> +
>> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>
> Okay, but there should also be a test that actually fires probes.

... why? What we care about is that DTrace can see the probes properly
(since we already know from other tests that once it can do that,
everything else works), and -l is an easy way to verify that (and in
general exercise the entire pathway up to USDT probe discovery in dtrace
itself) without having to get tangled up in even *more* possible bugs
around probe firing. I hit enough bugs in the parser writing this as it
was...

I mean I can write one but I don't see what it's going to tell us.

-- 
NULL && (void)

More information about the DTrace-devel mailing list