[DTrace-devel] [PATCH v4 1/7] cg: move get_member() to dt_cg.c
Kris Van Hees
kris.van.hees at oracle.com
Wed Jul 9 16:47:58 UTC 2025
Woops, one more.
On Wed, Jul 09, 2025 at 12:28:15PM -0400, Kris Van Hees wrote:
> Some comments below... Sorry - I got distracted by the other patches and
> failed to give this one attention.
>
> On Wed, Jul 09, 2025 at 03:46:54PM +0100, Alan Maguire via DTrace-devel wrote:
> > It will be used by both dt_prov_ip.c and dt_prov_tcp.c.
> >
> > Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
> > Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> > libdtrace/dt_cg.c | 39 ++++++++++++++++++++++++++++++++++++
> > libdtrace/dt_cg.h | 2 ++
> > libdtrace/dt_prov_ip.c | 45 ++++--------------------------------------
> > 3 files changed, 45 insertions(+), 41 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index bd0763d6..d6fc8259 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -1901,6 +1901,45 @@ dt_cg_ctf_offsetof(const char *structname, const char *membername,
> > return (ctm.ctm_offset / NBBY);
> > }
> >
> > +/*
> > + * Retrieve the value of a member in a given struct.
> > + *
> > + * Entry:
> > + * reg = TYPE *ptr
> > + *
> > + * Return:
> > + * %r0 = ptr->member
> > + * Clobbers:
> > + * %r1 .. %r5
> > + */
> > +int
> > +dt_cg_get_member(dt_pcb_t *pcb, const char *name, int reg,
> > + const char *member)
Why is dt_cg_get_member() returning int? Nothing looks at it anyway. ANd
with the change to use dt_cg_ctf_offset(), compilation error will be repotred
from there.
> Given that this is meant to be used from trampoline code (where we currently
> do not do register tracking - i.e. we expect the code writer to make sure that
> no collisions of register use occur), it probably should be named dt_cg_tramp_*
> rather than dt_cg_*. That also signals people looking at this that register
> allocation is done manually, and with care, so we can expect things like %r0
> getting assigned a value here, and that we expect that the passed in reg is NOT
> %r0.
>
> > +{
> > + dtrace_hdl_t *dtp = pcb->pcb_hdl;
> > + dt_irlist_t *dlp = &pcb->pcb_ir;
> > + dtrace_typeinfo_t tt;
> > + ctf_membinfo_t ctm;
> > + size_t size;
> > + uint_t ldop;
> > +
> > + if (dtrace_lookup_by_type(dtp, DTRACE_OBJ_KMODS, name, &tt) == -1 ||
> > + ctf_member_info(tt.dtt_ctfp, tt.dtt_type, member, &ctm) == CTF_ERR)
> > + return -1;
>
> The dt_cg_tramp_get_member name would have me assume that this can get me the
> member of any struct, not just those defined in kernel modules (or the kernel
> proper - which in DTrace terms is a module). So, if the intent is that it can
> only use structs defined at the kernel level, then the function name should
> probably reflect that. If not (and that is what I would prefer), then this
> should do a lookup for DTRACE_OBJ_EVERY, like e.g. dt_cg_ctf_offsetof does.
>
> On that note, perhaps it would be easier to extend dt_cg_ctf_offsetof() to
> take another (optional) arg to put the ldop in, populate that one if a ptr
> is passed to store it in, and then use that here? Because you are essentially
> doing the same lookup, you need the offset, and the size, and so the only
> extra think needed (that dt_cg_ctf_offsetof() could provide easily) is the
> ldop.
>
> So, I think dt_cg_ctf_offsetof() could become:
>
> dt_cg_ctf_offsetof(const char *structname, const char *membername,
> size_t *sizep, uint_t *ldopp, int relaxed)
>
> and instead of:
>
> if (sizep)
> *sizep = ctf_type_size(ctfp, ctm.ctm_type);
>
> it could use:
>
> if (sizep || ldopp) {
> uint_t ldop;
>
> ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, sizep);
> if (ldopp)
> *ldopp = ldop;
> }
>
> and then the dt_cg_tramp_get_member can just use:
>
> offset = dt_cg_ctf_offsetof(name, member, &size, &ldop, 0);
>
> instead of the lookup above, and the dt_cg_ldsize() call below.
>
> > +
> > + ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, &size);
> > +
> > + emit(dlp, BPF_MOV_REG(BPF_REG_3, reg));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
>
> This would use offset.
>
> > + emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> > + emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_BASE));
> > + emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> > + emit(dlp, BPF_LOAD(ldop, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_BASE));
> > +
> > + return 0;
> > +}
> > +
> > static void
> > dt_cg_act_breakpoint(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> > {
> > diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> > index 81b79399..36f587d3 100644
> > --- a/libdtrace/dt_cg.h
> > +++ b/libdtrace/dt_cg.h
> > @@ -44,6 +44,8 @@ extern void dt_cg_tramp_epilogue_advance(dt_pcb_t *pcb, dt_activity_t act);
> > extern void dt_cg_tramp_error(dt_pcb_t *pcb);
> > extern int dt_cg_ctf_offsetof(const char *structname, const char *membername,
> > size_t *sizep, int relaxed);
> > +extern int dt_cg_get_member(dt_pcb_t *pcb, const char *name, int reg,
> > + const char *member);
> > extern uint_t dt_cg_ldsize(dt_node_t *dnp, ctf_file_t *ctfp, ctf_id_t type,
> > ssize_t *ret_size);
> > extern uint_t bpf_ldst_size(ssize_t size, int store);
> > diff --git a/libdtrace/dt_prov_ip.c b/libdtrace/dt_prov_ip.c
> > index c4a3a6e2..37f91e3d 100644
> > --- a/libdtrace/dt_prov_ip.c
> > +++ b/libdtrace/dt_prov_ip.c
> > @@ -62,43 +62,6 @@ static int populate(dtrace_hdl_t *dtp)
> > probe_args, probes);
> > }
> >
> > -/*
> > - * Retrieve the value of a member in a given struct.
> > - *
> > - * Entry:
> > - * reg = TYPE *ptr
> > - *
> > - * Return:
> > - * %r0 = ptr->member
> > - * Clobbers:
> > - * %r1 .. %r5
> > - */
> > -static int get_member(dt_pcb_t *pcb, const char *name, int reg,
> > - const char *member) {
> > - dtrace_hdl_t *dtp = pcb->pcb_hdl;
> > - dt_irlist_t *dlp = &pcb->pcb_ir;
> > - dtrace_typeinfo_t tt;
> > - ctf_membinfo_t ctm;
> > - size_t size;
> > - uint_t ldop;
> > -
> > - if (dtrace_lookup_by_type(dtp, DTRACE_OBJ_KMODS, name, &tt) == -1 ||
> > - ctf_member_info(tt.dtt_ctfp, tt.dtt_type, member, &ctm) == CTF_ERR)
> > - return -1;
> > -
> > - ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, &size);
> > -
> > - emit(dlp, BPF_MOV_REG(BPF_REG_3, reg));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
> > - emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> > - emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> > - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_BASE));
> > - emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> > - emit(dlp, BPF_LOAD(ldop, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_BASE));
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * Generate a BPF trampoline for a SDT probe.
> > *
> > @@ -142,7 +105,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_6));
> >
> > - get_member(pcb, "struct sk_buff", BPF_REG_6, "sk");
> > + dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "sk");
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> >
> > /*
> > @@ -150,11 +113,11 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > * skb_network_header(skb) = (include/linux/ip.h)
> > * skb->head + skb->network_header (include/linux/skbuff.h)
> > */
> > - get_member(pcb, "struct sk_buff", BPF_REG_6, "head");
> > + dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "head");
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(2), BPF_REG_0));
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(4), BPF_REG_0));
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(5), BPF_REG_0));
> > - get_member(pcb, "struct sk_buff", BPF_REG_6, "network_header");
> > + dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "network_header");
> > emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(2), BPF_REG_0));
> > emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(4), BPF_REG_0));
> > emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(5), BPF_REG_0));
> > @@ -168,7 +131,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > else
> > emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(5), 0));
> >
> > - get_member(pcb, "struct sk_buff", BPF_REG_6, "dev");
> > + dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "dev");
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(3), BPF_REG_0));
> >
> > return 0;
> > --
> > 2.39.3
> >
> >
> > _______________________________________________
> > 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