[DTrace-devel] [PATCH 07/20] Load the global string table data into the strtab BPF map

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

The dt_zalloc() looks perhaps like a memory leak (in case dt_strtab was 
already allocated and now is being grown), but I guess it doesn't matter 
since this code is being rewritten by a subsequent patch.  But that 
raises the issue that it'd be cleaner to squash this patch and "Build 
the global strtab once" together for a cleaner commit history.  After 
all, the changes in this particular patch are so short-lived, soon 
rewritten by that other patch.

FWIW (this being my first foray into strtab stuff), the whole 
dt_strtab_write/dt_strtab_copystr seems rather messy.  I guess it was 
improved by "Provide a standard dt_strtab_copystr() function", but I 
don't understand why the callback wasn't eliminated completely as a part 
of that patch.  It'd be a bit simpler that way.  But I guess that's 
pre-existing messiness and does not need to be addressed as part of the 
current work.

On 6/2/21 1:47 AM, Kris Van Hees wrote:
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_bpf.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 1f31f845..67c36e97 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -198,7 +198,7 @@ int
>   dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>   {
>   	int		gvarsz, lvarsz, tvarc, aggsz;
> -	int		ci_mapfd;
> +	int		ci_mapfd, st_mapfd;
>   	uint32_t	key = 0;
>   
>   	/* If we already created the global maps, return success. */
> @@ -251,8 +251,24 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>   				roundup(dtp->dt_maxreclen, 8), 1) == -1)
>   		return -1;		/* dt_errno is set for us */
>   
> -	if (create_gmap(dtp, "strtab", BPF_MAP_TYPE_ARRAY,
> -			sizeof(uint32_t), dtp->dt_strlen, 1) == -1)
> +	/*
> +	 * We may need to create a final copy of the string table because it is
> +	 * possible entries were added after the last clause was compiled.
> +	 */
> +	if (dtp->dt_strlen < dt_strtab_size(dtp->dt_ccstab)) {
> +		dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
> +		dtp->dt_strtab = dt_zalloc(dtp, dtp->dt_strlen);
> +		if (dtp->dt_strtab == NULL)
> +			return dt_set_errno(dtp, EDT_NOMEM);
> +
> +		dt_strtab_write(dtp->dt_ccstab,
> +				(dt_strtab_write_f *)dt_strtab_copystr,
> +				dtp->dt_strtab);
> +	}
> +
> +	st_mapfd = create_gmap(dtp, "strtab", BPF_MAP_TYPE_ARRAY,
> +			       sizeof(uint32_t), dtp->dt_strlen, 1);
> +	if (st_mapfd == -1)
>   		return -1;		/* dt_errno is set for us */
>   
>   	if (gvarsz > 0 &&
> @@ -273,6 +289,9 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>   	/* Populate the 'cpuinfo' map. */
>   	dt_bpf_map_update(ci_mapfd, &key, dtp->dt_conf.cpus);
>   
> +	/* Populate the 'strtab' map. */
> +	dt_bpf_map_update(st_mapfd, &key, dtp->dt_strtab);
> +
>   	/* Set some task_struct offsets in state. */
>   	if (set_task_offsets(dtp))
>   		return dt_set_errno(dtp, EDT_CTF);