[PATCH v3 1/2] print() action: identify ctf object used for print

[PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

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@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

[PATCH v3 2/2] unittest/print: add test covering module-defined type

[Alan Maguire]

print() action tests do not cover module-defined types.  Add a test
using a module-defined type that has not changed recently.

struct tun_page was chosen since it is identical in UEK6U3 and upstream
and declared in tun.c which is a module on most distros.

Added tun to test/modules as suggested by Eugene.

Suggested-by: Nick Alcock <nick.alcock@oracle.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Eugene Loh <eugene.loh@oracle.com>

---
 test/modules                            |  1 +
 test/unittest/print/tst.print.modtype.d | 18 ++++++++++++++++++
 test/unittest/print/tst.print.modtype.r |  6 ++++++
 3 files changed, 25 insertions(+)
 create mode 100644 test/unittest/print/tst.print.modtype.d
 create mode 100644 test/unittest/print/tst.print.modtype.r

diff --git a/test/modules b/test/modules
index 53dc5544..0f01d6e0 100644
--- a/test/modules
+++ b/test/modules
@@ -2,3 +2,4 @@ ext4
 isofs
 nfs
 rds
+tun
diff --git a/test/unittest/print/tst.print.modtype.d b/test/unittest/print/tst.print.modtype.d
new file mode 100644
index 00000000..ed68b492
--- /dev/null
+++ b/test/unittest/print/tst.print.modtype.d
@@ -0,0 +1,18 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+/* @@nosort */
+
+#pragma D option quiet
+
+BEGIN
+{
+	tun_page = (struct tun_page *)alloca(sizeof (struct tun_page));
+	tun_page->page = (struct page *)0xfeedfacefeedface;
+	tun_page->count = 123;
+	print(tun_page);
+	exit(0);
+}
diff --git a/test/unittest/print/tst.print.modtype.r b/test/unittest/print/tst.print.modtype.r
new file mode 100644
index 00000000..b9736607
--- /dev/null
+++ b/test/unittest/print/tst.print.modtype.r
@@ -0,0 +1,6 @@
+{ptr} = *
+                                            (struct tun_page) {
+                                             .page = (struct page *){ptr},
+                                             .count = (int)123,
+                                            }
+
-- 
2.39.3

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

On 26/04/2024 14:33, 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@oracle.com>

ping on this one; I know things are busy with USDT right now but would
be good to land a fix for this issue if someone has the cycles to take a
look. If the BTF identification scheme doesn't work we can try something
else of course but above id scheme seems to be a reasonable compromise
between robustness+speed. Thanks!

Alan

> ---
>  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

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Kris Van Hees]

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@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
> 

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

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@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
>>

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Kris Van Hees]

On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
> 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.

I mistakenly thought that we already included code in DTrace that performs
type copying, but I cannot find it so perhaps I was mistaken (or it is more
obscure).

But then something else occured to me...

This is all compile-type-known data.  In other words, none of this is passed
in the actual trace buffer.  That means that we have a lot less worries about
how to store this information.  So, look below at your patch and some comments
that I think make it even easier.

> 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@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);

Here you add a data item in the trace buffer that will actually not store
anything at all.  That got me thinking...  And then you store the data size
both as the specified size of the data item *and* as extra argument.  That use
of the extra argument is unnecessary since the size is already being added
to the record as dtrd_size (which gets its value from the 4th arg to
dt_rec_add().  That means we can use the dtrd_arg for something else.

So...  how about you do the following:

	/* reserve space for addr and data (modname and type as extra arg) */
	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
	data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
			      size, 8, NULL, type);

And then the consume code can perform a module lookup by name based on the arg
that is attached to the 1st record, and get the type from the arg that is
attached to the 2nd record.

This also avoids the complication of modules possibly getting unloaded (even if
that is quite rare) because that will mean that the module cannot be found and
we can therefore take appropriate action (unknown type so just dump raw data
or something like that).

> >>  
> >> 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
> >>

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

On 01/07/2024 17:50, Kris Van Hees wrote:
> On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
>> 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.
> 
> I mistakenly thought that we already included code in DTrace that performs
> type copying, but I cannot find it so perhaps I was mistaken (or it is more
> obscure).
> 
> But then something else occured to me...
> 
> This is all compile-type-known data.  In other words, none of this is passed
> in the actual trace buffer.  That means that we have a lot less worries about
> how to store this information.  So, look below at your patch and some comments
> that I think make it even easier.
> 

Thanks; more below..

>> 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@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);
> 
> Here you add a data item in the trace buffer that will actually not store
> anything at all.  That got me thinking...  And then you store the data size
> both as the specified size of the data item *and* as extra argument.  That use
> of the extra argument is unnecessary since the size is already being added
> to the record as dtrd_size (which gets its value from the 4th arg to
> dt_rec_add().  That means we can use the dtrd_arg for something else.
> 
> So...  how about you do the following:
> 
> 	/* reserve space for addr and data (modname and type as extra arg) */
> 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);

Maybe I'm misunderstanding here but isn't there a concern if dmp goes
away before we collect trace data this string will no longer be valid? I
_think_ if we were to store the module name we'd need to store the
string itself in the trace record. The module id approach was an attempt
to remove the need to store and retrieve full strings while not exposing
us to the potential risk of invalid pointer dereference. Sounds like the
trace record storage implementation was inefficient but that was the
intent at least.


> 	data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> 			      size, 8, NULL, type);
> 
> And then the consume code can perform a module lookup by name based on the arg
> that is attached to the 1st record, and get the type from the arg that is
> attached to the 2nd record.
> 
> This also avoids the complication of modules possibly getting unloaded (even if
> that is quite rare) because that will mean that the module cannot be found and
> we can therefore take appropriate action (unknown type so just dump raw data
> or something like that).
> 

See above; my concern is recording the module name via string could
result in ending up with invalid data in the case of a module unload.

Alan

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Kris Van Hees]

On Mon, Jul 01, 2024 at 06:21:19PM +0100, Alan Maguire wrote:
> On 01/07/2024 17:50, Kris Van Hees wrote:
> > On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
> >> 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.
> > 
> > I mistakenly thought that we already included code in DTrace that performs
> > type copying, but I cannot find it so perhaps I was mistaken (or it is more
> > obscure).
> > 
> > But then something else occured to me...
> > 
> > This is all compile-type-known data.  In other words, none of this is passed
> > in the actual trace buffer.  That means that we have a lot less worries about
> > how to store this information.  So, look below at your patch and some comments
> > that I think make it even easier.
> > 
> 
> Thanks; more below..
> 
> >> 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@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);
> > 
> > Here you add a data item in the trace buffer that will actually not store
> > anything at all.  That got me thinking...  And then you store the data size
> > both as the specified size of the data item *and* as extra argument.  That use
> > of the extra argument is unnecessary since the size is already being added
> > to the record as dtrd_size (which gets its value from the 4th arg to
> > dt_rec_add().  That means we can use the dtrd_arg for something else.
> > 
> > So...  how about you do the following:
> > 
> > 	/* reserve space for addr and data (modname and type as extra arg) */
> > 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> > 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
> 
> Maybe I'm misunderstanding here but isn't there a concern if dmp goes
> away before we collect trace data this string will no longer be valid? I
> _think_ if we were to store the module name we'd need to store the
> string itself in the trace record. The module id approach was an attempt
> to remove the need to store and retrieve full strings while not exposing
> us to the potential risk of invalid pointer dereference. Sounds like the
> trace record storage implementation was inefficient but that was the
> intent at least.

Easy... replace
	(uint64_t)dmp->dm_name
above with
	(uint64_t)strdup(dmp->dm_name)

> > 	data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> > 			      size, 8, NULL, type);
> > 
> > And then the consume code can perform a module lookup by name based on the arg
> > that is attached to the 1st record, and get the type from the arg that is
> > attached to the 2nd record.
> > 
> > This also avoids the complication of modules possibly getting unloaded (even if
> > that is quite rare) because that will mean that the module cannot be found and
> > we can therefore take appropriate action (unknown type so just dump raw data
> > or something like that).
> > 
> 
> See above; my concern is recording the module name via string could
> result in ending up with invalid data in the case of a module unload.
> 
> Alan

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

On 01/07/2024 18:47, Kris Van Hees wrote:
> On Mon, Jul 01, 2024 at 06:21:19PM +0100, Alan Maguire wrote:
>> On 01/07/2024 17:50, Kris Van Hees wrote:
>>> On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
>>>> 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.
>>>
>>> I mistakenly thought that we already included code in DTrace that performs
>>> type copying, but I cannot find it so perhaps I was mistaken (or it is more
>>> obscure).
>>>
>>> But then something else occured to me...
>>>
>>> This is all compile-type-known data.  In other words, none of this is passed
>>> in the actual trace buffer.  That means that we have a lot less worries about
>>> how to store this information.  So, look below at your patch and some comments
>>> that I think make it even easier.
>>>
>>
>> Thanks; more below..
>>
>>>> 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@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);
>>>
>>> Here you add a data item in the trace buffer that will actually not store
>>> anything at all.  That got me thinking...  And then you store the data size
>>> both as the specified size of the data item *and* as extra argument.  That use
>>> of the extra argument is unnecessary since the size is already being added
>>> to the record as dtrd_size (which gets its value from the 4th arg to
>>> dt_rec_add().  That means we can use the dtrd_arg for something else.
>>>
>>> So...  how about you do the following:
>>>
>>> 	/* reserve space for addr and data (modname and type as extra arg) */
>>> 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>>> 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
>>
>> Maybe I'm misunderstanding here but isn't there a concern if dmp goes
>> away before we collect trace data this string will no longer be valid? I
>> _think_ if we were to store the module name we'd need to store the
>> string itself in the trace record. The module id approach was an attempt
>> to remove the need to store and retrieve full strings while not exposing
>> us to the potential risk of invalid pointer dereference. Sounds like the
>> trace record storage implementation was inefficient but that was the
>> intent at least.
> 
> Easy... replace
> 	(uint64_t)dmp->dm_name
> above with
> 	(uint64_t)strdup(dmp->dm_name)
> 

Right but then I've got to

- allocate a string per print() action
- free them later (presumably on DTrace cleanup?)
- when consuming the record, I've got to compute the hash on the name
string to do the module lookup by name, rather than by id where it is
pre-computed at cg time. This adds overhead to each consume action.

If you insist I'll do it as this needs to get fixed, but I do think the
module id approach is cleaner.

Alan

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Kris Van Hees]

On Tue, Jul 02, 2024 at 01:30:11PM +0100, Alan Maguire wrote:
> On 01/07/2024 18:47, Kris Van Hees wrote:
> > On Mon, Jul 01, 2024 at 06:21:19PM +0100, Alan Maguire wrote:
> >> On 01/07/2024 17:50, Kris Van Hees wrote:
> >>> On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
> >>>> 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.
> >>>
> >>> I mistakenly thought that we already included code in DTrace that performs
> >>> type copying, but I cannot find it so perhaps I was mistaken (or it is more
> >>> obscure).
> >>>
> >>> But then something else occured to me...
> >>>
> >>> This is all compile-type-known data.  In other words, none of this is passed
> >>> in the actual trace buffer.  That means that we have a lot less worries about
> >>> how to store this information.  So, look below at your patch and some comments
> >>> that I think make it even easier.
> >>>
> >>
> >> Thanks; more below..
> >>
> >>>> 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@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);
> >>>
> >>> Here you add a data item in the trace buffer that will actually not store
> >>> anything at all.  That got me thinking...  And then you store the data size
> >>> both as the specified size of the data item *and* as extra argument.  That use
> >>> of the extra argument is unnecessary since the size is already being added
> >>> to the record as dtrd_size (which gets its value from the 4th arg to
> >>> dt_rec_add().  That means we can use the dtrd_arg for something else.
> >>>
> >>> So...  how about you do the following:
> >>>
> >>> 	/* reserve space for addr and data (modname and type as extra arg) */
> >>> 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> >>> 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
> >>
> >> Maybe I'm misunderstanding here but isn't there a concern if dmp goes
> >> away before we collect trace data this string will no longer be valid? I
> >> _think_ if we were to store the module name we'd need to store the
> >> string itself in the trace record. The module id approach was an attempt
> >> to remove the need to store and retrieve full strings while not exposing
> >> us to the potential risk of invalid pointer dereference. Sounds like the
> >> trace record storage implementation was inefficient but that was the
> >> intent at least.
> > 
> > Easy... replace
> > 	(uint64_t)dmp->dm_name
> > above with
> > 	(uint64_t)strdup(dmp->dm_name)
> > 
> 
> Right but then I've got to
> 
> - allocate a string per print() action
> - free them later (presumably on DTrace cleanup?)

These do not really pose an issue.  Unless there are going to be a very large
amount of print actions, but then again, we have similar situations elsewhere.

> - when consuming the record, I've got to compute the hash on the name
> string to do the module lookup by name, rather than by id where it is
> pre-computed at cg time. This adds overhead to each consume action.

I did a comparitive test between your v3 patch and a modified one that stores
a copy of the module name as dtrd_arg to the data record.  I honestly did not
notice a difference between the two approaches in terms of performance.  While
the use of a string causes a hash value to be calculated, using an id also
incurs additional cost in its processing.  From the looks of it, it is a bit
of a trade-off where we incur a higher cost.

My preference would be for using the name primarily because it does seem to
provide the same level of performance, but without adding additional support
in htab to make dm_id work, and also without adding the dm_id management code.

> If you insist I'll do it as this needs to get fixed, but I do think the
> module id approach is cleaner.
> 
> Alan

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Alan Maguire]

On 02/07/2024 19:59, Kris Van Hees wrote:
> On Tue, Jul 02, 2024 at 01:30:11PM +0100, Alan Maguire wrote:
>> On 01/07/2024 18:47, Kris Van Hees wrote:
>>> On Mon, Jul 01, 2024 at 06:21:19PM +0100, Alan Maguire wrote:
>>>> On 01/07/2024 17:50, Kris Van Hees wrote:
>>>>> On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
>>>>>> 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.
>>>>>
>>>>> I mistakenly thought that we already included code in DTrace that performs
>>>>> type copying, but I cannot find it so perhaps I was mistaken (or it is more
>>>>> obscure).
>>>>>
>>>>> But then something else occured to me...
>>>>>
>>>>> This is all compile-type-known data.  In other words, none of this is passed
>>>>> in the actual trace buffer.  That means that we have a lot less worries about
>>>>> how to store this information.  So, look below at your patch and some comments
>>>>> that I think make it even easier.
>>>>>
>>>>
>>>> Thanks; more below..
>>>>
>>>>>> 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@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);
>>>>>
>>>>> Here you add a data item in the trace buffer that will actually not store
>>>>> anything at all.  That got me thinking...  And then you store the data size
>>>>> both as the specified size of the data item *and* as extra argument.  That use
>>>>> of the extra argument is unnecessary since the size is already being added
>>>>> to the record as dtrd_size (which gets its value from the 4th arg to
>>>>> dt_rec_add().  That means we can use the dtrd_arg for something else.
>>>>>
>>>>> So...  how about you do the following:
>>>>>
>>>>> 	/* reserve space for addr and data (modname and type as extra arg) */
>>>>> 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>>>>> 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
>>>>
>>>> Maybe I'm misunderstanding here but isn't there a concern if dmp goes
>>>> away before we collect trace data this string will no longer be valid? I
>>>> _think_ if we were to store the module name we'd need to store the
>>>> string itself in the trace record. The module id approach was an attempt
>>>> to remove the need to store and retrieve full strings while not exposing
>>>> us to the potential risk of invalid pointer dereference. Sounds like the
>>>> trace record storage implementation was inefficient but that was the
>>>> intent at least.
>>>
>>> Easy... replace
>>> 	(uint64_t)dmp->dm_name
>>> above with
>>> 	(uint64_t)strdup(dmp->dm_name)
>>>
>>
>> Right but then I've got to
>>
>> - allocate a string per print() action
>> - free them later (presumably on DTrace cleanup?)
> 
> These do not really pose an issue.  Unless there are going to be a very large
> amount of print actions, but then again, we have similar situations elsewhere.
> 
>> - when consuming the record, I've got to compute the hash on the name
>> string to do the module lookup by name, rather than by id where it is
>> pre-computed at cg time. This adds overhead to each consume action.
> 
> I did a comparitive test between your v3 patch and a modified one that stores
> a copy of the module name as dtrd_arg to the data record.  I honestly did not
> notice a difference between the two approaches in terms of performance.  While
> the use of a string causes a hash value to be calculated, using an id also
> incurs additional cost in its processing.  From the looks of it, it is a bit
> of a trade-off where we incur a higher cost.
> 
> My preference would be for using the name primarily because it does seem to
> provide the same level of performance, but without adding additional support
> in htab to make dm_id work, and also without adding the dm_id management code.
> 

Okay, sounds good. One thing I'm unclear about still is where to free
the module name string. I _think_ the right answer is to associate it
with the pcb, strdup()ing to a pcb->pcb_modname string which can then be
passed via the arg and later freed via dt_pcb_pop(). Is that the right
approach or should it be freed in a different way? Thanks!

Alan

Re: [PATCH v3 1/2] print() action: identify ctf object used for print

[Kris Van Hees]

On Wed, Jul 03, 2024 at 02:00:55PM +0100, Alan Maguire wrote:
> On 02/07/2024 19:59, Kris Van Hees wrote:
> > On Tue, Jul 02, 2024 at 01:30:11PM +0100, Alan Maguire wrote:
> >> On 01/07/2024 18:47, Kris Van Hees wrote:
> >>> On Mon, Jul 01, 2024 at 06:21:19PM +0100, Alan Maguire wrote:
> >>>> On 01/07/2024 17:50, Kris Van Hees wrote:
> >>>>> On Mon, Jul 01, 2024 at 09:06:32AM +0100, Alan Maguire wrote:
> >>>>>> 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.
> >>>>>
> >>>>> I mistakenly thought that we already included code in DTrace that performs
> >>>>> type copying, but I cannot find it so perhaps I was mistaken (or it is more
> >>>>> obscure).
> >>>>>
> >>>>> But then something else occured to me...
> >>>>>
> >>>>> This is all compile-type-known data.  In other words, none of this is passed
> >>>>> in the actual trace buffer.  That means that we have a lot less worries about
> >>>>> how to store this information.  So, look below at your patch and some comments
> >>>>> that I think make it even easier.
> >>>>>
> >>>>
> >>>> Thanks; more below..
> >>>>
> >>>>>> 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@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);
> >>>>>
> >>>>> Here you add a data item in the trace buffer that will actually not store
> >>>>> anything at all.  That got me thinking...  And then you store the data size
> >>>>> both as the specified size of the data item *and* as extra argument.  That use
> >>>>> of the extra argument is unnecessary since the size is already being added
> >>>>> to the record as dtrd_size (which gets its value from the 4th arg to
> >>>>> dt_rec_add().  That means we can use the dtrd_arg for something else.
> >>>>>
> >>>>> So...  how about you do the following:
> >>>>>
> >>>>> 	/* reserve space for addr and data (modname and type as extra arg) */
> >>>>> 	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
> >>>>> 			      sizeof(uint64_t), 8, NULL, (uint64_t)dmp->dm_name);
> >>>>
> >>>> Maybe I'm misunderstanding here but isn't there a concern if dmp goes
> >>>> away before we collect trace data this string will no longer be valid? I
> >>>> _think_ if we were to store the module name we'd need to store the
> >>>> string itself in the trace record. The module id approach was an attempt
> >>>> to remove the need to store and retrieve full strings while not exposing
> >>>> us to the potential risk of invalid pointer dereference. Sounds like the
> >>>> trace record storage implementation was inefficient but that was the
> >>>> intent at least.
> >>>
> >>> Easy... replace
> >>> 	(uint64_t)dmp->dm_name
> >>> above with
> >>> 	(uint64_t)strdup(dmp->dm_name)
> >>>
> >>
> >> Right but then I've got to
> >>
> >> - allocate a string per print() action
> >> - free them later (presumably on DTrace cleanup?)
> > 
> > These do not really pose an issue.  Unless there are going to be a very large
> > amount of print actions, but then again, we have similar situations elsewhere.
> > 
> >> - when consuming the record, I've got to compute the hash on the name
> >> string to do the module lookup by name, rather than by id where it is
> >> pre-computed at cg time. This adds overhead to each consume action.
> > 
> > I did a comparitive test between your v3 patch and a modified one that stores
> > a copy of the module name as dtrd_arg to the data record.  I honestly did not
> > notice a difference between the two approaches in terms of performance.  While
> > the use of a string causes a hash value to be calculated, using an id also
> > incurs additional cost in its processing.  From the looks of it, it is a bit
> > of a trade-off where we incur a higher cost.
> > 
> > My preference would be for using the name primarily because it does seem to
> > provide the same level of performance, but without adding additional support
> > in htab to make dm_id work, and also without adding the dm_id management code.
> > 
> 
> Okay, sounds good. One thing I'm unclear about still is where to free
> the module name string. I _think_ the right answer is to associate it
> with the pcb, strdup()ing to a pcb->pcb_modname string which can then be
> passed via the arg and later freed via dt_pcb_pop(). Is that the right
> approach or should it be freed in a different way? Thanks!

Ah no, if you do that then it will be freed after compilation of the clause,
which means that the record in the datadesc will have an invalid pointer in it.

Records and associated data are freed in dt_datadesc_release.  There is a loop
already to handle records that hold a printf format.

Which actually got me thinking...  it is a bit of a stretch perhaps, but the
easiest way to do this would actually be to make use of the printf format data
(dt_pfargv_t) which isn't entirely unreasonable because this is a formatted
print and its output is handled by a function in dt_printf.c anyway.  So, in
that case you do something like this in dt_cg_act_print():

	dt_pfargv_t	*pfp = dt_printf_create(dtp, dmp->dm_name);
...
	
	addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
			      sizeof(uint64_t), 8, pfp, type);

and then you could even implement dt_print_type) as a regular printf-style
function without needing dt_print_print() to interpose.  Or alternatively,
use dt_print_print() to get the module name from dtrd_format->pfv_format,
and keep dt_print_type() as is.

Either way, the dt_printf_create() performs the strdup() and dt_printf_destroy()
(called from dt_datadesc_release)) will take care of it getting freed.

This is abusing the format string a little but on the other hand, one could
argue that the module name together with dtrd_arg specifying the ctf type does
in fact specify an output formatting.

	Kris