[DTrace-devel] [PATCH v3 1/2] print() action: identify ctf object used for print
Alan Maguire
alan.maguire at oracle.com
Mon Jul 1 08:06:32 UTC 2024
On 28/06/2024 22:55, Kris Van Hees wrote:
> 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.
> in
> 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
Yeah, I think (as well as modules going away) the other worry is record
corruption, so dealing in direct pointers is too risky I think.
> 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?
>
I looked to see if there any CTF APIs to simplify recursive type
copying, and I couldn't find any and that's my worry - this sort of
copying could get complicated fast. The other issue is size - a lot of
the kernel types will pull in a massive number of dependenent types
(print()ing a skb will pull in sockets, task_structs etc). Duplicating
all of that in the D dict would be expensive I suspect. The other danger
is introducing multiple types of the same name into the D dict might
complicate things for other consumers of it.
The worst case scenario in the current patch series - that the module
goes away - means we can't print the type info, but that's not the end
of the world really. Certainly in the kernel case most modules tend to
stay loaded.
> 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