[DTrace-devel] [PATCH v3 1/2] print() action: identify ctf object used for print
Kris Van Hees
kris.van.hees at oracle.com
Fri Jun 28 21:55:33 UTC 2024
Looking at this again, I am wondering why we don't just add type info to the
data record? Since we know the type of the data items being added to trace
records (and type is expressed as ctfp and type id), we might as well store
that in the record. And that would make this entire situation pretty much a
non-issue because the record would already comtain all that we need to know
to consume it and print it. No need to pass a module id.
If the issue is that modules could get unloaded and thereby some pointers
might become invalid, then perhaps the solution would be to always copy the
type to the D dict (I think that is the right one - I always mix C and D).
And since that one is always around, there would be no need to associate an
id with the module and pass that.
Perhaps doing such a copy is the best action here? And we may need to look
at other cases where typed data from modules might be stored, and if there are
any others, they may also be subject to possible issues when modules are
unloaded and in that case would require a similar type copying action?
On Fri, Apr 26, 2024 at 02:33:26PM +0100, Alan Maguire wrote:
> when generating code for print() action we need to identify source
> of CTF; is it shared CTF, cdefs or ddefs or another module?
> Introduce a module id scheme where the module id can be stored
> at cg time for later retrieval at print time; this allows us
> to later look up the module and its associated CTF without relying
> on possibly freed pointers.
>
> Under the hood, the id scheme uses a monotonic counter coupled
> with the hash of the modules name; the latter allows us to quickly
> look up the right bucket via dt_htab_find_hval().
>
> This fixes an issue encountered when using a shared kernel type
> with alloca()ed memory; DTrace assumed the type was in ddefs
> and it wasn't so it wasn't displayed. This change fixes that and
> all tests continue to pass. Also tested that it works with
> kernel module-defined types now:
>
> dtrace: description 'fbt::ieee80211_rx_napi:entry ' matched 1 probe
> CPU ID FUNCTION:NAME
> 7 123161 ieee80211_rx_napi:entry 0xffff9f8980fa08e0 = *
> (struct ieee80211_hw) {
> .conf = (struct ieee80211_conf) {
> .listen_interval = (u16)10,
> .long_frame_max_tx_count = (u8)4,
> .short_frame_max_tx_count = (u8)7,
> },
>
> Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
> ---
> libdtrace/dt_cg.c | 7 ++++++-
> libdtrace/dt_consume.c | 7 +++++--
> libdtrace/dt_htab.c | 17 +++++++++++++++++
> libdtrace/dt_htab.h | 2 ++
> libdtrace/dt_impl.h | 1 +
> libdtrace/dt_module.c | 30 ++++++++++++++++++++++++++++++
> libdtrace/dt_module.h | 1 +
> libdtrace/dt_printf.c | 12 +++++++++---
> libdtrace/dt_printf.h | 2 +-
> 9 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 27246a40..30cccdaf 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -20,6 +20,7 @@
> #include <dt_dctx.h>
> #include <dt_cg.h>
> #include <dt_grammar.h>
> +#include <dt_module.h>
> #include <dt_parser.h>
> #include <dt_printf.h>
> #include <dt_provider.h>
> @@ -2832,7 +2833,9 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> ctf_id_t type = addr->dn_type;
> char n[DT_TYPE_NAMELEN];
> size_t size;
> + dt_module_t *dmp;
>
> + dmp = dt_module_lookup_by_ctf(dtp, fp);
> type = ctf_type_reference(fp, type);
> if (type == CTF_ERR)
> longjmp(yypcb->pcb_jmpbuf, EDT_CTF);
> @@ -2842,9 +2845,11 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> "print( ) argument #1 reference has type '%s' with size 0; cannot print( ) it.\n",
> ctf_type_name(fp, type, n, sizeof(n)));
>
> - /* reserve space for addr/type, data/size */
> + /* reserve space for addr/type, module identification, data/size */
> addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> sizeof(uint64_t), 8, NULL, type);
> + dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> + sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_id);
> data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> size, 8, NULL, size);
>
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index dec2314b..432b4e12 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1987,9 +1987,11 @@ static int
> dt_print_print(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> const caddr_t buf)
> {
> - dtrace_recdesc_t *data_rec = rec + 1;
> + dtrace_recdesc_t *ctf_rec = rec + 1;
> + dtrace_recdesc_t *data_rec = rec + 2;
> size_t max_size = dtp->dt_options[DTRACEOPT_PRINTSIZE];
> size_t size = (size_t)data_rec->dtrd_arg;
> + unsigned long dm_id = (unsigned long)ctf_rec->dtrd_arg;
> uint64_t printaddr;
>
> if (size > max_size)
> @@ -1998,7 +2000,8 @@ dt_print_print(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> if (dt_read_scalar(buf, rec, &printaddr) < 0)
> return dt_set_errno(dtp, EDT_PRINT);
>
> - return dt_print_type(dtp, fp, printaddr, (ctf_id_t)rec->dtrd_arg,
> + return dt_print_type(dtp, fp, printaddr, dm_id,
> + (ctf_id_t)rec->dtrd_arg,
> (caddr_t)buf + data_rec->dtrd_offset, size);
> }
>
> diff --git a/libdtrace/dt_htab.c b/libdtrace/dt_htab.c
> index 478c728a..3c167cb5 100644
> --- a/libdtrace/dt_htab.c
> +++ b/libdtrace/dt_htab.c
> @@ -229,6 +229,23 @@ void *dt_htab_find(const dt_htab_t *htab, const void *entry,
> return ent;
> }
>
> +/* Find an entry in the hashtable, using the provided callback function as a
> + * secondary comparison function to differentiate between entries in a bucket.
> + * A pre-computed hash value is passed in.
> + */
> +void *dt_htab_find_hval(const dt_htab_t *htab, uint32_t hval,
> + dt_htab_ecmp_fn *cmpf, void *arg)
> +{
> + int idx = hval & htab->mask;
> + dt_hbucket_t *bucket;
> +
> + for (bucket = htab->tab[idx]; bucket; bucket = bucket->next) {
> + if (cmpf(bucket->head, arg))
> + return bucket->head;
> + }
> + return NULL;
> +}
> +
> /*
> * Remove an entry from the hashtable. If we are deleting the last entry in a
> * bucket, get rid of the bucket.
> diff --git a/libdtrace/dt_htab.h b/libdtrace/dt_htab.h
> index d39ae65e..1ce5a80d 100644
> --- a/libdtrace/dt_htab.h
> +++ b/libdtrace/dt_htab.h
> @@ -100,6 +100,8 @@ extern void *dt_htab_lookup(const dt_htab_t *htab, const void *entry);
> typedef int dt_htab_ecmp_fn(const void *entry, void *arg);
> extern void *dt_htab_find(const dt_htab_t *htab, const void *entry,
> dt_htab_ecmp_fn *cmpf, void *arg);
> +extern void *dt_htab_find_hval(const dt_htab_t *htab, uint32_t hval,
> + dt_htab_ecmp_fn *cmpf, void *arg);
> extern size_t dt_htab_entries(const dt_htab_t *htab);
> extern int dt_htab_delete(dt_htab_t *htab, void *entry);
> extern void *dt_htab_next(const dt_htab_t *htab, dt_htab_next_t **it);
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 2dfb64d6..2bd1dafe 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -136,6 +136,7 @@ typedef struct dt_module {
> dt_list_t dm_list; /* list forward/back pointers */
> char dm_name[DTRACE_MODNAMELEN]; /* string name of module */
> char dm_file[PATH_MAX]; /* file path of module */
> + unsigned long dm_id; /* id of module */
> uint_t dm_flags; /* module flags (see below) */
> struct dt_hentry dm_he; /* htab links */
> dtrace_hdl_t *dm_dtp; /* backpointer to the dtrace instance */
> diff --git a/libdtrace/dt_module.c b/libdtrace/dt_module.c
> index 1e730426..7697f73b 100644
> --- a/libdtrace/dt_module.c
> +++ b/libdtrace/dt_module.c
> @@ -35,6 +35,8 @@
> #define KSYM_NAME_MAX 128 /* from kernel/scripts/kallsyms.c */
> #define GZCHUNKSIZE (1024*512) /* gzip uncompression chunk size */
>
> +unsigned int module_idx = 0;
> +
> static void
> dt_module_unload(dtrace_hdl_t *dtp, dt_module_t *dmp);
>
> @@ -50,6 +52,12 @@ dt_module_hval(const dt_module_t *mod)
> return str2hval(mod->dm_name, 0);
> }
>
> +/* id is combination of hash and index */
> +static void dt_module_init_id(dt_module_t *dmp)
> +{
> + dmp->dm_id = ((unsigned long)dt_module_hval(dmp)) << 32 | module_idx++;
> +}
> +
> static int
> dt_module_cmp(const dt_module_t *p, const dt_module_t *q)
> {
> @@ -150,6 +158,7 @@ dt_module_create(dtrace_hdl_t *dtp, const char *name)
>
> memset(dmp, 0, sizeof(dt_module_t));
> strlcpy(dmp->dm_name, name, sizeof(dmp->dm_name));
> + dt_module_init_id(dmp);
> if (dt_htab_insert(dtp->dt_mods, dmp) < 0) {
> free(dmp);
> return NULL;
> @@ -193,6 +202,27 @@ dt_module_lookup_by_ctf(dtrace_hdl_t *dtp, ctf_file_t *ctfp)
> return ctfp ? ctf_getspecific(ctfp) : NULL;
> }
>
> +static int
> +dt_module_idmatch(const dt_module_t *m1, dt_module_t *m2)
> +{
> + return m1->dm_id == m2->dm_id;
> +}
> +
> +/* High-order 32 bits of id are hash value, low order the module index.
> + * This allows us to limit cost of search based on bucket size.
> + */
> +dt_module_t *
> +dt_module_lookup_by_id(dtrace_hdl_t *dtp, unsigned long id)
> +{
> + uint32_t hval = id >> 32;
> + dt_module_t tmpl;
> +
> + tmpl.dm_id = id;
> +
> + return dt_htab_find_hval(dtp->dt_mods, hval, (dt_htab_ecmp_fn *)dt_module_idmatch,
> + &tmpl);
> +}
> +
> static int
> dt_module_init_elf(dtrace_hdl_t *dtp, dt_module_t *dmp)
> {
> diff --git a/libdtrace/dt_module.h b/libdtrace/dt_module.h
> index 56df17a6..68dadfc6 100644
> --- a/libdtrace/dt_module.h
> +++ b/libdtrace/dt_module.h
> @@ -18,6 +18,7 @@ extern dt_module_t *dt_module_create(dtrace_hdl_t *, const char *);
>
> extern dt_module_t *dt_module_lookup_by_name(dtrace_hdl_t *, const char *);
> extern dt_module_t *dt_module_lookup_by_ctf(dtrace_hdl_t *, ctf_file_t *);
> +extern dt_module_t *dt_module_lookup_by_id(dtrace_hdl_t *, unsigned long);
>
> extern ctf_file_t *dt_module_getctf(dtrace_hdl_t *, dt_module_t *);
> extern dt_ident_t *dt_module_extern(dtrace_hdl_t *, dt_module_t *,
> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
> index 50842216..108adc23 100644
> --- a/libdtrace/dt_printf.c
> +++ b/libdtrace/dt_printf.c
> @@ -14,6 +14,7 @@
> #include <limits.h>
> #include <port.h>
>
> +#include <dt_module.h>
> #include <dt_printf.h>
> #include <dt_string.h>
> #include <dt_impl.h>
> @@ -2236,13 +2237,18 @@ err:
>
> int
> dt_print_type(dtrace_hdl_t *dtp, FILE *fp, uint64_t printaddr,
> - ctf_id_t type, caddr_t data, size_t size)
> + unsigned long dm_id, ctf_id_t type, caddr_t data, size_t size)
> {
> struct dt_visit_arg dva;
> + dt_module_t *dmp = dt_module_lookup_by_id(dtp, dm_id);
>
> + if (!dmp) {
> + dt_dprintf("error retrieving CTF for print() action\n");
> + return dt_set_errno(dtp, EDT_CTF);
> + }
> + dva.dv_ctfp = dt_module_getctf(dtp, dmp);
> dva.dv_dtp = dtp;
> dva.dv_fp = fp;
> - dva.dv_ctfp = dtp->dt_ddefs->dm_ctfp;
> dva.dv_data = data;
> dva.dv_size = size;
> dva.dv_last_depth = 0;
> @@ -2260,5 +2266,5 @@ dt_print_type(dtrace_hdl_t *dtp, FILE *fp, uint64_t printaddr,
> if (dt_print_close_parens(&dva, 0) < 0)
> return -1;
>
> - return 2;
> + return 3;
> }
> diff --git a/libdtrace/dt_printf.h b/libdtrace/dt_printf.h
> index 9771c4a8..8d55d82b 100644
> --- a/libdtrace/dt_printf.h
> +++ b/libdtrace/dt_printf.h
> @@ -108,7 +108,7 @@ extern int dt_print_ustack(dtrace_hdl_t *, FILE *,
> const char *, caddr_t, uint64_t);
> extern int dt_print_mod(dtrace_hdl_t *, FILE *, const char *, caddr_t);
> extern int dt_print_umod(dtrace_hdl_t *, FILE *, const char *, caddr_t);
> -extern int dt_print_type(dtrace_hdl_t *, FILE *, uint64_t, ctf_id_t,
> +extern int dt_print_type(dtrace_hdl_t *, FILE *, uint64_t, unsigned long, ctf_id_t,
> caddr_t, size_t);
>
> #ifdef __cplusplus
> --
> 2.39.3
>
More information about the DTrace-devel
mailing list