[DTrace-devel] [PATCH 2/2] Pass load-time constants to BPF programs in a consistent way
Kris Van Hees
kris.van.hees at oracle.com
Fri Apr 22 04:21:23 UTC 2022
On Thu, Feb 17, 2022 at 03:45:31PM -0500, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> In bpf/, there are .c and .S programs that are cross-compiled at
> build time, but they sometimes rely on constants that are not known
> until the programs are loaded. Two ways were being used to provide
> those constants. One was to put them in the state map. Another was
> to set the values during relocation.
>
> Use one consistent method for providing these constants.
>
> Specifically, set the constants during relocation rather than using
> the state map:
>
> *) These constants do not change during execution.
>
> *) Avoiding a map lookup saves a function call, saving BPF
> instructions, simplifying BPF programming, and reducing
> register pressure.
>
> *) The constants' values are made known to the BPF verifier.
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com
... and I will put it on dev.
By the way, this is a nice improvement!
> ---
> bpf/get_bvar.c | 41 ++++++++---------------------------------
> libdtrace/dt_bpf.c | 30 ------------------------------
> libdtrace/dt_bpf.h | 6 ++++--
> libdtrace/dt_cc.c | 35 ++++++++++++++++++++---------------
> libdtrace/dt_dlibs.c | 4 +++-
> libdtrace/dt_state.h | 7 -------
> 6 files changed, 35 insertions(+), 88 deletions(-)
>
> diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
> index c04822e4..0da4b35a 100644
> --- a/bpf/get_bvar.c
> +++ b/bpf/get_bvar.c
> @@ -135,58 +135,33 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id)
> }
> case DIF_VAR_EXECNAME: {
> uint64_t ptr;
> - uint32_t key;
> - uint32_t *comm_off;
> -
> - /*
> - * In the "state" map, look up the "struct task_struct" offset
> - * of "comm".
> - */
> - key = DT_STATE_TASK_COMM_OFF;
> - comm_off = bpf_map_lookup_elem(&state, &key);
> - if (comm_off == NULL)
> - return error(dctx, DTRACEFLT_ILLOP, 0);
> + extern uint64_t TASK_COMM;
>
> /* &(current->comm) */
> ptr = bpf_get_current_task();
> if (ptr == 0)
> return error(dctx, DTRACEFLT_BADADDR, ptr);
>
> - return (uint64_t)ptr + *comm_off;
> + return (uint64_t)ptr + (uint64_t)&TASK_COMM;
> }
> case DIF_VAR_WALLTIMESTAMP:
> return bpf_ktime_get_ns() + ((uint64_t)&BOOTTM);
> case DIF_VAR_PPID: {
> uint64_t ptr;
> int32_t val = -1;
> - uint32_t key;
> - uint32_t *parent_off;
> - uint32_t *tgid_off;
> -
> - /*
> - * In the "state" map, look up the "struct task_struct" offsets
> - * of real_parent and tgid.
> - */
> - key = DT_STATE_TASK_PARENT_OFF;
> - parent_off = bpf_map_lookup_elem(&state, &key);
> - if (parent_off == NULL)
> - return -1;
> -
> - key = DT_STATE_TASK_TGID_OFF;
> - tgid_off = bpf_map_lookup_elem(&state, &key);
> - if (tgid_off == NULL)
> - return -1;
> + extern uint64_t TASK_REAL_PARENT;
> + extern uint64_t TASK_TGID;
>
> /* Chase pointers val = current->real_parent->tgid. */
> ptr = bpf_get_current_task();
> if (ptr == 0)
> return error(dctx, DTRACEFLT_BADADDR, ptr);
> if (bpf_probe_read((void *)&ptr, 8,
> - (const void *)(ptr + *parent_off)))
> - return error(dctx, DTRACEFLT_BADADDR, ptr + *parent_off);
> + (const void *)(ptr + (uint64_t)&TASK_REAL_PARENT)))
> + return error(dctx, DTRACEFLT_BADADDR, ptr + (uint64_t)&TASK_REAL_PARENT);
> if (bpf_probe_read((void *)&val, 4,
> - (const void *)(ptr + *tgid_off)))
> - return error(dctx, DTRACEFLT_BADADDR, ptr + *tgid_off);
> + (const void *)(ptr + (uint64_t)&TASK_TGID)))
> + return error(dctx, DTRACEFLT_BADADDR, ptr + (uint64_t)&TASK_TGID);
>
> return (uint64_t)val;
> }
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 635c74cb..a597ca1c 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -134,32 +134,6 @@ create_gmap(dtrace_hdl_t *dtp, const char *name, enum bpf_map_type type,
> return fd;
> }
>
> -static int
> -set_task_offsets(dtrace_hdl_t *dtp)
> -{
> - ctf_id_t type;
> - ctf_membinfo_t ctm;
> - ctf_file_t *cfp = dtp->dt_shared_ctf;
> -
> - type = ctf_lookup_by_name(cfp, "struct task_struct");
> - if (type == CTF_ERR)
> - return -1;
> -
> - if (ctf_member_info(cfp, type, "real_parent", &ctm) == CTF_ERR)
> - return -1;
> - dt_state_set_offparent(dtp, ctm.ctm_offset / NBBY);
> -
> - if (ctf_member_info(cfp, type, "tgid", &ctm) == CTF_ERR)
> - return -1;
> - dt_state_set_offtgid(dtp, ctm.ctm_offset / NBBY);
> -
> - if (ctf_member_info(cfp, type, "comm", &ctm) == CTF_ERR)
> - return -1;
> - dt_state_set_offcomm(dtp, ctm.ctm_offset / NBBY);
> -
> - return 0;
> -}
> -
> static void
> populate_probes_map(dtrace_hdl_t *dtp, int fd)
> {
> @@ -375,10 +349,6 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> /* Populate the 'probes' map. */
> populate_probes_map(dtp, pr_mapfd);
>
> - /* Set some task_struct offsets in state. */
> - if (set_task_offsets(dtp))
> - return dt_set_errno(dtp, EDT_CTF);
> -
> return 0;
> }
>
> diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
> index e361d103..957d593e 100644
> --- a/libdtrace/dt_bpf.h
> +++ b/libdtrace/dt_bpf.h
> @@ -28,8 +28,10 @@ extern "C" {
> #define DT_CONST_BOOTTM 8
> #define DT_CONST_NSPEC 9
> #define DT_CONST_NCPUS 10
> -#define DT_CONST_TASK_REAL_PARENT 11
> -#define DT_CONST_TASK_PID 12
> +#define DT_CONST_TASK_PID 11
> +#define DT_CONST_TASK_TGID 12
> +#define DT_CONST_TASK_REAL_PARENT 13
> +#define DT_CONST_TASK_COMM 14
>
> extern int perf_event_open(struct perf_event_attr *attr, pid_t pid, int cpu,
> int group_fd, unsigned long flags);
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index f5b59109..b73789a2 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -2367,28 +2367,33 @@ dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t *prp, dtrace_difo_t *dp,
> return -1;
> nrp->dofr_data = boottime;
> continue;
> - case DT_CONST_TASK_REAL_PARENT: {
> + case DT_CONST_TASK_PID:
> + case DT_CONST_TASK_TGID:
> + case DT_CONST_TASK_REAL_PARENT:
> + case DT_CONST_TASK_COMM: {
> ctf_file_t *cfp = dtp->dt_shared_ctf;
> ctf_id_t type = ctf_lookup_by_name(cfp, "struct task_struct");
> ctf_membinfo_t ctm;
> + int rc;
>
> if (type == CTF_ERR)
> return -1;
>
> - if (ctf_member_info(cfp, type, "real_parent", &ctm) == CTF_ERR)
> - return -1;
> - nrp->dofr_data = ctm.ctm_offset / NBBY;
> - continue;
> - }
> - case DT_CONST_TASK_PID: {
> - ctf_file_t *cfp = dtp->dt_shared_ctf;
> - ctf_id_t type = ctf_lookup_by_name(cfp, "struct task_struct");
> - ctf_membinfo_t ctm;
> -
> - if (type == CTF_ERR)
> - return -1;
> -
> - if (ctf_member_info(cfp, type, "pid", &ctm) == CTF_ERR)
> + switch (idp->di_id) {
> + case DT_CONST_TASK_PID:
> + rc = ctf_member_info(cfp, type, "pid", &ctm);
> + break;
> + case DT_CONST_TASK_TGID:
> + rc = ctf_member_info(cfp, type, "tgid", &ctm);
> + break;
> + case DT_CONST_TASK_REAL_PARENT:
> + rc = ctf_member_info(cfp, type, "real_parent", &ctm);
> + break;
> + case DT_CONST_TASK_COMM:
> + rc = ctf_member_info(cfp, type, "comm", &ctm);
> + break;
> + }
> + if (rc == CTF_ERR)
> return -1;
> nrp->dofr_data = ctm.ctm_offset / NBBY;
> continue;
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index a8d112d4..efbd3261 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -79,8 +79,10 @@ static const dt_ident_t dt_bpf_symbols[] = {
> DT_BPF_SYMBOL_ID(BOOTTM, DT_IDENT_SCALAR, DT_CONST_BOOTTM),
> DT_BPF_SYMBOL_ID(NSPEC, DT_IDENT_SCALAR, DT_CONST_NSPEC),
> DT_BPF_SYMBOL_ID(NCPUS, DT_IDENT_SCALAR, DT_CONST_NCPUS),
> - DT_BPF_SYMBOL_ID(TASK_REAL_PARENT, DT_IDENT_SCALAR, DT_CONST_TASK_REAL_PARENT),
> DT_BPF_SYMBOL_ID(TASK_PID, DT_IDENT_SCALAR, DT_CONST_TASK_PID),
> + DT_BPF_SYMBOL_ID(TASK_TGID, DT_IDENT_SCALAR, DT_CONST_TASK_TGID),
> + DT_BPF_SYMBOL_ID(TASK_REAL_PARENT, DT_IDENT_SCALAR, DT_CONST_TASK_REAL_PARENT),
> + DT_BPF_SYMBOL_ID(TASK_COMM, DT_IDENT_SCALAR, DT_CONST_TASK_COMM),
>
> /* End-of-list marker */
> { NULL, }
> diff --git a/libdtrace/dt_state.h b/libdtrace/dt_state.h
> index 6c743b8e..9a82f994 100644
> --- a/libdtrace/dt_state.h
> +++ b/libdtrace/dt_state.h
> @@ -21,9 +21,6 @@ typedef enum dt_state_elem {
> DT_STATE_ACTIVITY = 0, /* activity state of the session */
> DT_STATE_BEGANON, /* cpu BEGIN probe executed on */
> DT_STATE_ENDEDON, /* cpu END probe executed on */
> - DT_STATE_TASK_PARENT_OFF, /* offsetof(struct task_struct, real_parent) */
> - DT_STATE_TASK_TGID_OFF, /* offsetof(struct task_struct, tgid) */
> - DT_STATE_TASK_COMM_OFF, /* offsetof(struct task_struct, comm) */
> DT_STATE_NUM_ELEMS
> } dt_state_elem_t;
>
> @@ -64,10 +61,6 @@ dt_state_set(dtrace_hdl_t *dtp, uint32_t key, uint32_t val)
>
> # define dt_state_get_beganon(dtp) dt_state_get(dtp, DT_STATE_BEGANON)
> # define dt_state_get_endedon(dtp) dt_state_get(dtp, DT_STATE_ENDEDON)
> -
> -# define dt_state_set_offparent(dtp, x) dt_state_set(dtp, DT_STATE_TASK_PARENT_OFF, (x))
> -# define dt_state_set_offtgid(dtp, x) dt_state_set(dtp, DT_STATE_TASK_TGID_OFF, (x))
> -# define dt_state_set_offcomm(dtp, x) dt_state_set(dtp, DT_STATE_TASK_COMM_OFF, (x))
> #endif
>
> #endif /* _DT_STATE_H */
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list