[DTrace-devel] [PATCH 15/15] Implement the proc provider

[Nick Alcock]

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)