[DTrace-devel] [PATCH 15/15] Implement the proc provider
Nick Alcock
nick.alcock at oracle.com
Thu Feb 23 18:02:48 UTC 2023
On 23 Feb 2023, Kris Van Hees told this:
> On Thu, Feb 23, 2023 at 02:00:11PM +0000, Nick Alcock wrote:
>> On 23 Feb 2023, Kris Van Hees via DTrace-devel uttered the following:
>> > +#define KPROBES "kprobes"
>> > +#define SYSCALLS "syscalls"
>> > +#define UPROBES "uprobes"
>> > +#define PID "dt_pid"
>>
>> Do these defines really add anything?
>
> Oh, shoot, I forgot to remove a bunch of stuff that was meant to be cleaned up
> before posting that final patch. Sorry :( Obviously, those are not needed.
I didn't actually notice that they were entirely unused :)
>> > + * Dependent probes need priorities for cases where two probes are dependent on
>> > + * the same underlying probe, but one needs to 'fire' before the other. One
>> > + * such example is proc:::start and proc:::lwp-start.
>> > + *
>> > + * proc:::exec
>> > + * - Use syscall::exec*:entry
>> > + * - Clear arg1 and beyond (arg0 is already the filename)
>> > + * proc:::exec-success
>> > + * - Use syscall::exec*:return
>> > + * - Only when arg0 == 0
>> > + * - Clear all args
>> > + * proc:::exec-failure
>> > + * - Use syscall::exec*:return
>> > + * - Only when arg0 != 0
>> > + * - Argument arg0 is already correct
>>
>> Because of the save_args / restore_args calls in the dependent probe
>> code, all of these get to modify the main/underlying args, and none of
>> them leak args into each other, right?
>>
>> > + */
>
> This entire comment block is old notes that eventually grew stale.
>
> Not sure if I should bother rewriting it or just remove it.
It's still pretty useful -- I'd go with "rewrite". It's always useful to
have notes on how things work in the source tree.
>> > + { "signal-send", 0, { 0, "struct task_struct *", "lwpsinfo_t *" } },
>> > + { "signal-send", 1, { 0, "struct task_struct *", "psinfo_t *" } },
>> > + { "signal-send", 2, { 1, "int", } },
>>
>> It took a bit of effort to figure out that the native arg type (e.g
>> "int" here) being NULL is used to indicate a probe with zero args. You
>> should probably note that too :)
>
> Well, no... NULL as translated type (see above) means that no translation is
> needed. No values other than the name (see exec-success above) means there are
> no arguments and that is simply because the 2nd value in the struct is 0.
Really? It looks to me like you fall out of that loop due to
>> > + if (arg->argdesc.native == NULL)
>> > + break;
i.e. due to the native type being NULL. What am I missing?
>> > + pd.id = DTRACE_IDNONE;
>> > + pd.prv = "rawtp";
>> > + pd.mod = "sched";
>> > + pd.fun = "";
>> > + pd.prb = "sched_process_fork";
>>
>> This stuff will definitely end up needing to be data-driven in some way
>> as kernels change: whether via hardwiring in here or something in the
>> libdir/kernel.version stuff like translators, I'm not sure, but honestly
>> in the long run I think something like translators would be better.
>>
>> Not worth it yet, of course, and this should be easily translatable into
>> that sort of model.
>
> Time will tell. For now, I am waiting to see how complex these things can get
> before deciding how to make it more data-driven. It is just too soon to tell
> what we'll need.
Definitely agreed.
>> > + for (i = 0; i < ARRAY_SIZE(probe_args); i++) {
>> > + probe_arg_t *arg = &probe_args[i];
>> > +
>> > + if (strcmp(arg->name, prp->desc->prb) == 0) {
>> > + if (pidx == -1) {
>> > + pidx = i;
>> > +
>> > + if (arg->argdesc.native == NULL)
>> > + break;
>> > + }
>> > +
>> > + argc++;
>> > + }
>> > + }
>>
>> Needs at least some comment here or above probe_args as noted earlier:
>> this function imposes requirements on the structure of probe_args which
>> are nowhere described.
Doubly true given that I apparently got those requirements *wrong*.
>> ... obviously better than having the harness kill it! I was trying not
>> to rely on the fname, though, because there aren't going to be any other
>> longsleeps on the system, but sleeps doing actual work that shouldn't be
>> picked up are quite common (and yes I know the pr_pid clause should rule
>> those out).
>
> Yes... unfortunately pr_psargs doesn't work right now so I had to come up
> with an alternative. For the tst.exitcore.sh case I had to make it a bit
> more specific because there are multiple sleeps in the actual sleeper loop,
> but since here there is just the long sleep the pr_pid + pr_fname check is
> sufficient.
Agreed.
--
NULL && (void)
More information about the DTrace-devel
mailing list