[DTrace-devel] [PATCH v2 1/2] Implement the sched provider

[Nick Alcock]

On 22 May 2023, Kris Van Hees via DTrace-devel stated:

> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>

Oh this is so lovely and short now!

Reviewed-by: Nick Alcock <nick.alcock at oracle.com>

with the tiny nit below.

> +#include <assert.h>
> +#include <errno.h>
> +
> +#include "dt_dctx.h"
> +#include "dt_cg.h"
> +#include "dt_provider_sdt.h"
> +#include "dt_probe.h"
> +
> +static const char		prvname[] = "sched";
> +static const char		modname[] = "vmlinux";
> +
> +static probe_dep_t	probes[] = {
> +	{ "dequeue",
> +	  DTRACE_PROBESPEC_NAME,	"fbt::dequeue_task_*:entry" },
> +	{ "enqueue",
> +	  DTRACE_PROBESPEC_NAME,	"fbt::enqueue_task_*:entry" },
> +	{ "off-cpu",
> +	  DTRACE_PROBESPEC_NAME,	"rawtp:sched::sched_switch" },
> +	{ "on-cpu",
> +	  DTRACE_PROBESPEC_NAME,	"fbt::schedule_tail:entry" },
> +	{ "surrender",
> +	  DTRACE_PROBESPEC_NAME,	"fbt::do_sched_yield:entry" },
> +	{ "tick",
> +	  DTRACE_PROBESPEC_NAME,	"fbt::scheduler_tick:entry" },
> +	{ "wakeup",
> +	  DTRACE_PROBESPEC_NAME,	"rawtp:sched::sched_wakeup" },
> +	{ NULL, }
> +};

Now that's so clear even I don't think it needs a comment.

> +/*
> + * Provide all the "sched" SDT probes.
> + */
> +static int populate(dtrace_hdl_t *dtp)
> +{
> +	return dt_sdt_populate(dtp, prvname, modname, &dt_sched, &pattr,				       probe_args, probes);
> +}

The nit: 80 char-wide screen considered harmful (and a missing linefeed).

> +	if (strcmp(prp->desc->prb, "dequeue") == 0) {
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ARG(1)));
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> +		/*
> +		 * FIXME: arg1 should be a pointer to cpuinfo_t for the CPU
> +		 *	  associated with the runqueue.
> +		 */
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(1), 0));
> +	} else if (strcmp(prp->desc->prb, "enqueue") == 0) {
> +#define ENQUEUE_HEAD	0x10
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ARG(1)));
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> +		/*
> +		 * FIXME: arg1 should be a pointer to cpuinfo_t for the CPU
> +		 *	  associated with the runqueue.
> +		 */
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(1), 0));
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ARG(2)));
> +		emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, ENQUEUE_HEAD));
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(2), BPF_REG_0));

The magic ENQUEUE_HEAD *possibly* needs a comment: I had to do a bunch
of grepping about to get a clue what that 0x10 might be for and why it
had that value. (For reference: this is one of the ENQUEUE_* flags from
kernel/sched/sched.h in the kernel -- yes, not in include/ -- which
indicates that the task should be placed at the front of the runqueue
(which is what that arg is meant to do, but this isn't mentioned here:
you have to already know it, or look at the documentation, to figure out
what any of these probes' args actually are. I guess that's what
documentation is for, but it's maintained by a different team: we might
want to put at least a brief description of what these args are meant to
be now that they're being basically implemented as translators-in-
BPF-assembly like this.)

I'm a bit concerned that putting ENQUEUE_HEAD as a #define in the DTrace
source will make it rather hard to adapt to any changes to this deeply
internal value as kernel development proceeds... a shame it's hard to
tie syslibdir defines etc into the system at this level, because it
seems to me this will only get more common :( all this stuff really
feels like something translators should be able to do, dammit. But I
guess that's for later...)

-- 
NULL && (void)