* [PATCH v3] cg: allow providers to specify a skip count for stack retrieval
@ 2024-05-01 20:52 Kris Van Hees
2024-05-02 0:52 ` Eugene Loh
0 siblings, 1 reply; 2+ messages in thread
From: Kris Van Hees @ 2024-05-01 20:52 UTC (permalink / raw
To: dtrace, dtrace-devel
Some probes cause extra frames to be reported with bpf_get_stack() that
need to be skipped to ensure we report the correct stack trace to the
consumer. FBT probes based on fentry/fexit probes need this.
Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
bpf/get_bvar.c | 13 +++++++++----
libdtrace/dt_bpf.h | 1 +
libdtrace/dt_cc.c | 3 +++
libdtrace/dt_cg.c | 11 +++++++++--
libdtrace/dt_dlibs.c | 1 +
libdtrace/dt_prov_fbt.c | 1 +
libdtrace/dt_provider.h | 1 +
7 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
index 71eef277..ea5dc6b1 100644
--- a/bpf/get_bvar.c
+++ b/bpf/get_bvar.c
@@ -28,6 +28,7 @@ extern uint64_t STBSZ;
extern uint64_t STKSIZ;
extern uint64_t BOOTTM;
extern uint64_t STACK_OFF;
+extern uint64_t STACK_SKIP;
#define error(dctx, fault, illval) \
({ \
@@ -64,12 +65,14 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
case DIF_VAR_STACKDEPTH:
case DIF_VAR_USTACKDEPTH: {
uint32_t bufsiz = (uint32_t) (uint64_t) (&STKSIZ);
- uint64_t flags = 0 & BPF_F_SKIP_FIELD_MASK;
+ uint64_t flags;
char *buf = dctx->mem + (uint64_t)(&STACK_OFF);
uint64_t stacksize;
if (id == DIF_VAR_USTACKDEPTH)
- flags |= BPF_F_USER_STACK;
+ flags = BPF_F_USER_STACK;
+ else
+ flags = (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK;
stacksize = bpf_get_stack(dctx->ctx, buf, bufsiz, flags);
if (stacksize < 0)
@@ -89,11 +92,13 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
}
case DIF_VAR_CALLER:
case DIF_VAR_UCALLER: {
- uint64_t flags = 0 & BPF_F_SKIP_FIELD_MASK;
+ uint64_t flags;
uint64_t buf[2] = { 0, };
if (id == DIF_VAR_UCALLER)
- flags |= BPF_F_USER_STACK;
+ flags = BPF_F_USER_STACK;
+ else
+ flags = (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK;
if (bpf_get_stack(dctx->ctx, buf, sizeof(buf), flags) < 0)
return 0;
diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
index 3b9fc633..5b2df264 100644
--- a/libdtrace/dt_bpf.h
+++ b/libdtrace/dt_bpf.h
@@ -55,6 +55,7 @@ extern "C" {
#define DT_CONST_RODATA_SIZE 21
#define DT_CONST_ZERO_OFF 22
#define DT_CONST_STACK_OFF 23
+#define DT_CONST_STACK_SKIP 24
#define DT_BPF_LOG_SIZE_DEFAULT (UINT32_MAX >> 8)
#define DT_BPF_LOG_SIZE_SMALL 4096
diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
index cb07b8d6..d1ee3843 100644
--- a/libdtrace/dt_cc.c
+++ b/libdtrace/dt_cc.c
@@ -1193,6 +1193,9 @@ dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t *prp, dtrace_difo_t *dp,
case DT_CONST_STACK_OFF:
nrp->dofr_data = DMEM_STACK(dtp);
continue;
+ case DT_CONST_STACK_SKIP:
+ nrp->dofr_data = prp->prov->impl->stack_skip;
+ continue;
default:
/* probe name -> value is probe id */
if (strchr(idp->di_name, ':') != NULL)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 27246a40..a1c24e37 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -2597,6 +2597,9 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
uint64_t arg;
int nframes, stacksize, prefsz, align = sizeof(uint64_t);
uint_t lbl_valid = dt_irlist_label(dlp);
+ dt_ident_t *skip = dt_dlib_get_var(dtp, "STACK_SKIP");
+
+ assert(skip != NULL);
/* Get sizing information from dnp->dn_arg. */
arg = dt_cg_stack_arg(dtp, dnp, kind);
@@ -2644,8 +2647,12 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off + prefsz));
}
emit(dlp, BPF_MOV_IMM(BPF_REG_3, stacksize));
- emit(dlp, BPF_MOV_IMM(BPF_REG_4, (0 & BPF_F_SKIP_FIELD_MASK)
- | (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
+ if (kind == DTRACEACT_USTACK)
+ emit(dlp, BPF_MOV_IMM(BPF_REG_4, BPF_F_USER_STACK));
+ else {
+ emite(dlp, BPF_MOV_IMM(BPF_REG_4, -1), skip);
+ emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_4, BPF_F_SKIP_FIELD_MASK));
+ }
dt_regset_xalloc(drp, BPF_REG_0);
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
dt_regset_free_args(drp);
diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
index fe22616a..bc883e11 100644
--- a/libdtrace/dt_dlibs.c
+++ b/libdtrace/dt_dlibs.c
@@ -96,6 +96,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
DT_BPF_SYMBOL_ID(RODATA_SIZE, DT_IDENT_SCALAR, DT_CONST_RODATA_SIZE),
DT_BPF_SYMBOL_ID(ZERO_OFF, DT_IDENT_SCALAR, DT_CONST_ZERO_OFF),
DT_BPF_SYMBOL_ID(STACK_OFF, DT_IDENT_SCALAR, DT_CONST_STACK_OFF),
+ DT_BPF_SYMBOL_ID(STACK_SKIP, DT_IDENT_SCALAR, DT_CONST_STACK_SKIP),
/* End-of-list marker */
{ NULL, }
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 0ddffa20..fa888ed8 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -431,6 +431,7 @@ static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
dt_provimpl_t dt_fbt_fprobe = {
.name = prvname,
.prog_type = BPF_PROG_TYPE_TRACING,
+ .stack_skip = 4,
.populate = &populate,
.load_prog = &fprobe_prog_load,
.trampoline = &fprobe_trampoline,
diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
index a24b1d00..17b1844c 100644
--- a/libdtrace/dt_provider.h
+++ b/libdtrace/dt_provider.h
@@ -44,6 +44,7 @@ typedef struct dt_argdesc {
typedef struct dt_provimpl {
const char *name; /* provider generic name */
int prog_type; /* BPF program type */
+ uint32_t stack_skip; /* # of stack frames to skip */
int (*populate)(dtrace_hdl_t *dtp); /* register probes */
int (*provide)(dtrace_hdl_t *dtp, /* provide probes */
const dtrace_probedesc_t *pdp);
--
2.42.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] cg: allow providers to specify a skip count for stack retrieval
2024-05-01 20:52 [PATCH v3] cg: allow providers to specify a skip count for stack retrieval Kris Van Hees
@ 2024-05-02 0:52 ` Eugene Loh
0 siblings, 0 replies; 2+ messages in thread
From: Eugene Loh @ 2024-05-02 0:52 UTC (permalink / raw
To: dtrace, dtrace-devel
Looks good to me (famous last words), though I confess I do not know:
what tests show the problem/fix?
Assuming there is test coverage of the problem/fix... ideally
stack/ustack, fbt/non-fbt, caller, stackdepth:
Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
On 5/1/24 16:52, Kris Van Hees wrote:
> Some probes cause extra frames to be reported with bpf_get_stack() that
> need to be skipped to ensure we report the correct stack trace to the
> consumer. FBT probes based on fentry/fexit probes need this.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> bpf/get_bvar.c | 13 +++++++++----
> libdtrace/dt_bpf.h | 1 +
> libdtrace/dt_cc.c | 3 +++
> libdtrace/dt_cg.c | 11 +++++++++--
> libdtrace/dt_dlibs.c | 1 +
> libdtrace/dt_prov_fbt.c | 1 +
> libdtrace/dt_provider.h | 1 +
> 7 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
> index 71eef277..ea5dc6b1 100644
> --- a/bpf/get_bvar.c
> +++ b/bpf/get_bvar.c
> @@ -28,6 +28,7 @@ extern uint64_t STBSZ;
> extern uint64_t STKSIZ;
> extern uint64_t BOOTTM;
> extern uint64_t STACK_OFF;
> +extern uint64_t STACK_SKIP;
>
> #define error(dctx, fault, illval) \
> ({ \
> @@ -64,12 +65,14 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
> case DIF_VAR_STACKDEPTH:
> case DIF_VAR_USTACKDEPTH: {
> uint32_t bufsiz = (uint32_t) (uint64_t) (&STKSIZ);
> - uint64_t flags = 0 & BPF_F_SKIP_FIELD_MASK;
> + uint64_t flags;
> char *buf = dctx->mem + (uint64_t)(&STACK_OFF);
> uint64_t stacksize;
>
> if (id == DIF_VAR_USTACKDEPTH)
> - flags |= BPF_F_USER_STACK;
> + flags = BPF_F_USER_STACK;
> + else
> + flags = (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK;
>
> stacksize = bpf_get_stack(dctx->ctx, buf, bufsiz, flags);
> if (stacksize < 0)
> @@ -89,11 +92,13 @@ noinline uint64_t dt_get_bvar(const dt_dctx_t *dctx, uint32_t id, uint32_t idx)
> }
> case DIF_VAR_CALLER:
> case DIF_VAR_UCALLER: {
> - uint64_t flags = 0 & BPF_F_SKIP_FIELD_MASK;
> + uint64_t flags;
> uint64_t buf[2] = { 0, };
>
> if (id == DIF_VAR_UCALLER)
> - flags |= BPF_F_USER_STACK;
> + flags = BPF_F_USER_STACK;
> + else
> + flags = (uint64_t)(&STACK_SKIP) & BPF_F_SKIP_FIELD_MASK;
>
> if (bpf_get_stack(dctx->ctx, buf, sizeof(buf), flags) < 0)
> return 0;
> diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
> index 3b9fc633..5b2df264 100644
> --- a/libdtrace/dt_bpf.h
> +++ b/libdtrace/dt_bpf.h
> @@ -55,6 +55,7 @@ extern "C" {
> #define DT_CONST_RODATA_SIZE 21
> #define DT_CONST_ZERO_OFF 22
> #define DT_CONST_STACK_OFF 23
> +#define DT_CONST_STACK_SKIP 24
>
> #define DT_BPF_LOG_SIZE_DEFAULT (UINT32_MAX >> 8)
> #define DT_BPF_LOG_SIZE_SMALL 4096
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index cb07b8d6..d1ee3843 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -1193,6 +1193,9 @@ dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t *prp, dtrace_difo_t *dp,
> case DT_CONST_STACK_OFF:
> nrp->dofr_data = DMEM_STACK(dtp);
> continue;
> + case DT_CONST_STACK_SKIP:
> + nrp->dofr_data = prp->prov->impl->stack_skip;
> + continue;
> default:
> /* probe name -> value is probe id */
> if (strchr(idp->di_name, ':') != NULL)
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 27246a40..a1c24e37 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2597,6 +2597,9 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> uint64_t arg;
> int nframes, stacksize, prefsz, align = sizeof(uint64_t);
> uint_t lbl_valid = dt_irlist_label(dlp);
> + dt_ident_t *skip = dt_dlib_get_var(dtp, "STACK_SKIP");
> +
> + assert(skip != NULL);
>
> /* Get sizing information from dnp->dn_arg. */
> arg = dt_cg_stack_arg(dtp, dnp, kind);
> @@ -2644,8 +2647,12 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off + prefsz));
> }
> emit(dlp, BPF_MOV_IMM(BPF_REG_3, stacksize));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_4, (0 & BPF_F_SKIP_FIELD_MASK)
> - | (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
> + if (kind == DTRACEACT_USTACK)
> + emit(dlp, BPF_MOV_IMM(BPF_REG_4, BPF_F_USER_STACK));
> + else {
> + emite(dlp, BPF_MOV_IMM(BPF_REG_4, -1), skip);
> + emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_4, BPF_F_SKIP_FIELD_MASK));
> + }
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
> dt_regset_free_args(drp);
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index fe22616a..bc883e11 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -96,6 +96,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
> DT_BPF_SYMBOL_ID(RODATA_SIZE, DT_IDENT_SCALAR, DT_CONST_RODATA_SIZE),
> DT_BPF_SYMBOL_ID(ZERO_OFF, DT_IDENT_SCALAR, DT_CONST_ZERO_OFF),
> DT_BPF_SYMBOL_ID(STACK_OFF, DT_IDENT_SCALAR, DT_CONST_STACK_OFF),
> + DT_BPF_SYMBOL_ID(STACK_SKIP, DT_IDENT_SCALAR, DT_CONST_STACK_SKIP),
>
> /* End-of-list marker */
> { NULL, }
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 0ddffa20..fa888ed8 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -431,6 +431,7 @@ static void kprobe_detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> dt_provimpl_t dt_fbt_fprobe = {
> .name = prvname,
> .prog_type = BPF_PROG_TYPE_TRACING,
> + .stack_skip = 4,
> .populate = &populate,
> .load_prog = &fprobe_prog_load,
> .trampoline = &fprobe_trampoline,
> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> index a24b1d00..17b1844c 100644
> --- a/libdtrace/dt_provider.h
> +++ b/libdtrace/dt_provider.h
> @@ -44,6 +44,7 @@ typedef struct dt_argdesc {
> typedef struct dt_provimpl {
> const char *name; /* provider generic name */
> int prog_type; /* BPF program type */
> + uint32_t stack_skip; /* # of stack frames to skip */
> int (*populate)(dtrace_hdl_t *dtp); /* register probes */
> int (*provide)(dtrace_hdl_t *dtp, /* provide probes */
> const dtrace_probedesc_t *pdp);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-05-02 1:45 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 20:52 [PATCH v3] cg: allow providers to specify a skip count for stack retrieval Kris Van Hees
2024-05-02 0:52 ` Eugene Loh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.