* [PATCH] cg: allow providers to specify a skip count for stack retrieval
@ 2024-05-01 18:22 Kris Van Hees
2024-05-01 19:40 ` Eugene Loh
0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2024-05-01 18:22 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>
---
libdtrace/dt_cg.c | 6 ++++--
libdtrace/dt_prov_fbt.c | 1 +
libdtrace/dt_provider.h | 1 +
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 27246a40..36975406 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -2595,7 +2595,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
dt_irlist_t *dlp = &pcb->pcb_ir;
dt_regset_t *drp = pcb->pcb_regs;
uint64_t arg;
- int nframes, stacksize, prefsz, align = sizeof(uint64_t);
+ int nframes, stacksize, prefsz, skip;
+ int align = sizeof(uint64_t);
uint_t lbl_valid = dt_irlist_label(dlp);
/* Get sizing information from dnp->dn_arg. */
@@ -2603,6 +2604,7 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
prefsz = kind == DTRACEACT_USTACK ? sizeof(uint64_t) : 0;
nframes = DTRACE_USTACK_NFRAMES(arg);
stacksize = nframes * sizeof(uint64_t);
+ skip = yypcb->pcb_probe->prov->impl->skip_frames;
/* Handle alignment and reserve space in the output buffer. */
if (reg >= 0) {
@@ -2644,7 +2646,7 @@ 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)
+ emit(dlp, BPF_MOV_IMM(BPF_REG_4, (skip & BPF_F_SKIP_FIELD_MASK)
| (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
dt_regset_xalloc(drp, BPF_REG_0);
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 0ddffa20..dfeecfd1 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,
+ .skip_frames = 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..bf3af0be 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 skip_frames; /* # 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] 3+ messages in thread
* Re: [PATCH] cg: allow providers to specify a skip count for stack retrieval
2024-05-01 18:22 [PATCH] cg: allow providers to specify a skip count for stack retrieval Kris Van Hees
@ 2024-05-01 19:40 ` Eugene Loh
2024-05-01 20:12 ` Kris Van Hees
0 siblings, 1 reply; 3+ messages in thread
From: Eugene Loh @ 2024-05-01 19:40 UTC (permalink / raw)
To: dtrace, dtrace-devel
Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
I haven't played much with stack/fentry/fexit, so I hope this is right
(e.g., stable number of skip frames). Are our tests robust enough to
assure us?
BUT! What about stackdepth in bpf/get_bvar.c?
On 5/1/24 14:22, 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>
> ---
> libdtrace/dt_cg.c | 6 ++++--
> libdtrace/dt_prov_fbt.c | 1 +
> libdtrace/dt_provider.h | 1 +
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 27246a40..36975406 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2595,7 +2595,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_regset_t *drp = pcb->pcb_regs;
> uint64_t arg;
> - int nframes, stacksize, prefsz, align = sizeof(uint64_t);
> + int nframes, stacksize, prefsz, skip;
> + int align = sizeof(uint64_t);
> uint_t lbl_valid = dt_irlist_label(dlp);
>
> /* Get sizing information from dnp->dn_arg. */
> @@ -2603,6 +2604,7 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> prefsz = kind == DTRACEACT_USTACK ? sizeof(uint64_t) : 0;
> nframes = DTRACE_USTACK_NFRAMES(arg);
> stacksize = nframes * sizeof(uint64_t);
> + skip = yypcb->pcb_probe->prov->impl->skip_frames;
>
> /* Handle alignment and reserve space in the output buffer. */
> if (reg >= 0) {
> @@ -2644,7 +2646,7 @@ 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)
> + emit(dlp, BPF_MOV_IMM(BPF_REG_4, (skip & BPF_F_SKIP_FIELD_MASK)
> | (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 0ddffa20..dfeecfd1 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,
> + .skip_frames = 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..bf3af0be 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 skip_frames; /* # 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 [flat|nested] 3+ messages in thread
* Re: [PATCH] cg: allow providers to specify a skip count for stack retrieval
2024-05-01 19:40 ` Eugene Loh
@ 2024-05-01 20:12 ` Kris Van Hees
0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2024-05-01 20:12 UTC (permalink / raw)
To: Eugene Loh; +Cc: dtrace, dtrace-devel
On Wed, May 01, 2024 at 03:40:09PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
>
> I haven't played much with stack/fentry/fexit, so I hope this is right
> (e.g., stable number of skip frames). Are our tests robust enough to assure
> us?
>
> BUT! What about stackdepth in bpf/get_bvar.c?
Ah yes, need to adjust that one also. Thanks!
Incidentally, caller also needs to be fixed in this way.
AND this skip should NOT be applied to ustack() (nor ucaller and ustackdepth).
v2 coming soon :)
> On 5/1/24 14:22, 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>
> > ---
> > libdtrace/dt_cg.c | 6 ++++--
> > libdtrace/dt_prov_fbt.c | 1 +
> > libdtrace/dt_provider.h | 1 +
> > 3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 27246a40..36975406 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2595,7 +2595,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> > dt_irlist_t *dlp = &pcb->pcb_ir;
> > dt_regset_t *drp = pcb->pcb_regs;
> > uint64_t arg;
> > - int nframes, stacksize, prefsz, align = sizeof(uint64_t);
> > + int nframes, stacksize, prefsz, skip;
> > + int align = sizeof(uint64_t);
> > uint_t lbl_valid = dt_irlist_label(dlp);
> > /* Get sizing information from dnp->dn_arg. */
> > @@ -2603,6 +2604,7 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> > prefsz = kind == DTRACEACT_USTACK ? sizeof(uint64_t) : 0;
> > nframes = DTRACE_USTACK_NFRAMES(arg);
> > stacksize = nframes * sizeof(uint64_t);
> > + skip = yypcb->pcb_probe->prov->impl->skip_frames;
> > /* Handle alignment and reserve space in the output buffer. */
> > if (reg >= 0) {
> > @@ -2644,7 +2646,7 @@ 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)
> > + emit(dlp, BPF_MOV_IMM(BPF_REG_4, (skip & BPF_F_SKIP_FIELD_MASK)
> > | (kind == DTRACEACT_USTACK ? BPF_F_USER_STACK : 0)));
> > dt_regset_xalloc(drp, BPF_REG_0);
> > emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 0ddffa20..dfeecfd1 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,
> > + .skip_frames = 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..bf3af0be 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 skip_frames; /* # 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 [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-01 20:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 18:22 [PATCH] cg: allow providers to specify a skip count for stack retrieval Kris Van Hees
2024-05-01 19:40 ` Eugene Loh
2024-05-01 20:12 ` Kris Van Hees
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox