[DTrace-devel] [PATCH] Consolidate offset and size calculations for dctx->mem content
Eugene Loh
eugene.loh at oracle.com
Thu Feb 17 01:35:44 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with...
On 2/16/22 3:51 PM, Kris Van Hees via DTrace-devel wrote:
> Rather than duplicating expressions that calculate offsets and sizes of
> components of the dctx->mem content, express them using macros.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_bpf.c | 14 ++------------
> libdtrace/dt_cg.c | 8 ++++----
> libdtrace/dt_dctx.h | 43 +++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 8b551354..635c74cb 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -235,7 +235,6 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> size_t strsize = dtp->dt_options[DTRACEOPT_STRSIZE];
> uint8_t *buf, *end;
> char *strtab;
> - size_t strdatasz = P2ROUNDUP(strsize + 1, 8);
>
> /* If we already created the global maps, return success. */
> if (dt_gmap_done)
> @@ -295,21 +294,12 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> * - 8 bytes padding for trace buffer alignment purposes
> * - maximum trace buffer record size, rounded up to the nearest
> * multiple of 8
> - * - the greater of:
> - * - the maximum stack trace size
> - * - DT_TSTRING_SLOTS times the maximum space needed to
> - * store a string
> - * - size of the internal strtok() state
> - * - 8 bytes for the offset index
> - * - the maximum string size
> - * - a byte for the terminating NULL char
> + * - size of dctx->mem (see dt_dctx.h)
> */
> memsz = roundup(sizeof(dt_mstate_t), 8) +
> 8 +
> roundup(dtp->dt_maxreclen, 8) +
> - MAX(sizeof(uint64_t) * dtp->dt_options[DTRACEOPT_MAXFRAMES],
> - DT_TSTRING_SLOTS * strdatasz) +
> - sizeof(uint64_t) + dtp->dt_options[DTRACEOPT_STRSIZE] + 1;
> + DMEM_SIZE(dtp);
> if (create_gmap(dtp, "mem", BPF_MAP_TYPE_PERCPU_ARRAY,
> sizeof(uint32_t), memsz, 1) == -1)
> return -1; /* dt_errno is set for us */
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index eca50b29..64089da5 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -158,7 +158,7 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> * Store -1 to the strtok internal-state offset to indicate
> * that strtok internal state is not yet initialized.
> */
> - emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMEM_STRTOK, -1));
> + emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMEM_STRTOK(dtp), -1));
>
> /*
> * Store pointer to BPF map "name" in the DTrace context field "fld" at
> @@ -3917,7 +3917,7 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>
> /* the 8-byte prefix is the offset, which we initialize to 0 */
> emit(dlp, BPF_MOV_REG(BPF_REG_1, reg));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DMEM_STRTOK));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DMEM_STRTOK(dtp)));
> emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_1, 0, 0));
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8));
> emit(dlp, BPF_MOV_IMM(BPF_REG_2, dtp->dt_options[DTRACEOPT_STRSIZE] + 1));
> @@ -3932,7 +3932,7 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> } else {
> /* NULL string: error if internal state is uninitialized */
> emit(dlp, BPF_MOV_REG(BPF_REG_0, reg));
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMEM_STRTOK));
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMEM_STRTOK(dtp)));
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1));
> dt_cg_check_notnull(dlp, drp, BPF_REG_0);
> }
> @@ -3958,7 +3958,7 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>
> emit(dlp, BPF_MOV_REG(BPF_REG_1, dnp->dn_reg));
> emit(dlp, BPF_MOV_REG(BPF_REG_2, reg));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DMEM_STRTOK));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DMEM_STRTOK(dtp)));
> emit(dlp, BPF_MOV_REG(BPF_REG_3, del->dn_reg));
> dt_regset_free(drp, del->dn_reg);
> if (del->dn_tstring)
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index 178f4dd6..c8e4abef 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -55,13 +55,48 @@ typedef struct dt_dctx {
> #define DCTX_LVARS offsetof(dt_dctx_t, lvars)
> #define DCTX_SIZE ((int16_t)sizeof(dt_dctx_t))
>
> +/*
> + * The dctx->mem pointer references a block of memory that contains:
> + *
> + * +----------------+----------------+
> + * 0 -> | Stack | tstring |
> + * | trace | storage |
> + * | storage | |
> + * +----------------+----------------+
> + * DMEM_STRTOK -> | strtok() internal state |
> + * +---------------------------------+
I guess fine, though it takes a second to figure out that this is not a
two-dimensional figure. One option is:
+----------------+----------------+
0 -> | Stack : tstring |
| trace or storage |
| storage : |
+----------------+----------------+
DMEM_STRTOK -> | strtok() internal state |
+---------------------------------+
Also, putting a "DMEM_SIZE ->" in there would be helpful.
> + *
> + * The size of the dctx->mem memory blovk is the sum of:
blovk
> + * - the greater of:
> + * - the maximum stack trace size
> + * - DT_TSTRING_SLOTS times the maximum space needed to
> + * store a string
Well, the max to store a string... rounded up to a multiple of 8. Maybe
that's a small detail, but if the comments are there they might as well
be right. Or just get rid of a bunch of the comments since it's unclear
they explain anything that's in the code.
> + * - size of the internal strtok() state
> + * - 8 bytes for the offset index
Instead of "8 bytes" just say sizeof(uint64_t) to conform to the actual
code.
> + * - the maximum string size
> + * - a byte for the terminating NULL char
> + */
> +
> +/*
> + * Macros for the sizes of the components of dctx->mem.
> + */
> +#define DMEM_STACK_SZ(dtp) \
> + (sizeof(uint64_t) * (dtp)->dt_options[DTRACEOPT_MAXFRAMES])
> +#define DMEM_TSTR_SZ(dtp) \
> + (DT_TSTRING_SLOTS * \
> + P2ROUNDUP((dtp)->dt_options[DTRACEOPT_STRSIZE] + 1, 8))
> +#define DMEM_STRTOK_SZ(dtp) \
> + (sizeof(uint64_t) + (dtp)->dt_options[DTRACEOPT_STRSIZE] + 1)
> /*
> * Macro to determine the offset from mem to the strtok internal state.
> */
Probably want a blank line between the #define DMEM_STRTOK_SZ and the
comment.
> -#define DMEM_STRTOK MAX(sizeof(uint64_t) * \
> - dtp->dt_options[DTRACEOPT_MAXFRAMES], \
> - DT_TSTRING_SLOTS * \
> - roundup(dtp->dt_options[DTRACEOPT_STRSIZE] + 1, 8))
> +#define DMEM_STRTOK(dtp) \
> + MAX(DMEM_STACK_SZ(dtp), DMEM_TSTR_SZ(dtp))
> +
> +/*
> + * Macro to determine the total size of the mem areaa.
areaa
> + */
> +#define DMEM_SIZE(dtp) (DMEM_STRTOK(dtp) + DMEM_STRTOK_SZ(dtp))
>
> /*
> * Macro to determine the (negative) offset from the frame pointer (%fp) for
More information about the DTrace-devel
mailing list