[DTrace-devel] [PATCH 16/20] Build the global strtab once

[Eugene Loh]

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

Looks good, but now the comment block preceding the changed code in 
libdtrace/dt_bpf.c needs to be updated.

And in libdtrace/dt_as.c, in the associated comment block, might as well 
fix s/ny/by/.

And as I mentioned in my review of "Load the global string table data 
into the strtab BPF map", should these two patches be squashed?

On 6/2/21 1:48 AM, Kris Van Hees wrote:
> The global table of string constants was being re-created after every
> compilation even though we only ever use it once, when we create the
> BPF maps (right before loading BPF programs).
>
> Instead, we now generate the global strtab during the BPF maps
> creation.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_as.c  | 15 +++------------
>   libdtrace/dt_bpf.c | 17 +++++++----------
>   2 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
> index 760f281c..d88fe65c 100644
> --- a/libdtrace/dt_as.c
> +++ b/libdtrace/dt_as.c
> @@ -567,15 +567,9 @@ fail:
>   	 * We keep the string table around (dtp->dt_csstab) because further
>   	 * compilations may add more strings.  We will load a consolidated
>   	 * compiled string table into a strtab BPF map so it can be used ny
> -	 * all BPF programs we will be loading.  Since each DIFO will have a
> -	 * string table that comprises the strings of all DIFOs before it along
> -	 * with any new ones, we simply assign the latest (and most complete)
> -	 * string table to dtp->dt_strtab (and the size as dtp->dt_strlen), so
> -	 * that when we are ready to load and execute programs, we know we have
> -	 * the latest and greatest string table to work with.
> -	 *
> -	 * Note that this means that we should *not* free dtp->dt_strtab later
> -	 * because it will be free'd when the DIFO is destroyed.
> +	 * all BPF programs we will be loading.  Therefore, each DIFO will have
> +	 * a string table that comprises the strings of all DIFOs before it
> +	 * along with any new ones.
>   	 */
>   	dp->dtdo_strlen = dt_strtab_size(dtp->dt_ccstab);
>   	if (dp->dtdo_strlen > 0) {
> @@ -586,9 +580,6 @@ fail:
>   		dt_strtab_write(dtp->dt_ccstab,
>   				(dt_strtab_write_f *)dt_strtab_copystr,
>   				dp->dtdo_strtab);
> -
> -		dtp->dt_strtab = dp->dtdo_strtab;
> -		dtp->dt_strlen = dp->dtdo_strlen;
>   	} else
>   		dp->dtdo_strtab = NULL;
>   
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 41bd8ecd..067bf0d5 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -256,16 +256,13 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>   	 * 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);
> -	}
> +	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);