[DTrace-devel] [PATCH 12/12] Implement TLS variables

[Kris Van Hees]

On Tue, Nov 23, 2021 at 08:52:45PM -0500, Eugene Loh via DTrace-devel wrote:
> On 11/23/21 2:45 AM, Kris Van Hees wrote:
> 
> > On Mon, Nov 22, 2021 at 06:08:18PM -0500, Eugene Loh via DTrace-devel wrote:
> > > On 11/20/21 12:44 AM, Kris Van Hees via DTrace-devel wrote:
> > > > diff --git a/bpf/get_tvar.c b/bpf/get_tvar.c
> > > > @@ -10,12 +10,40 @@
> > > >    # define noinline	__attribute__((noinline))
> > > >    #endif
> > > > -extern struct bpf_map_def tvars;
> > > > +extern struct bpf_map_def dvars;
> > > > +extern uint64_t NCPUS;
> > > > -noinline uint64_t dt_get_tvar(uint32_t id)
> > > > +noinline void *dt_get_tvar(uint32_t id)
> > > >    {
> > > > -	uint64_t	*val;
> > > > +	uint64_t	key;
> > > > +	uint64_t	dflt_key = 0;;
> Double semicolon.

Yup

> > > > +	void		*val;
> > > > -	val = bpf_map_lookup_elem(&tvars, &id);
> > > > -	return val ? *val : 0;
> > > > +	key = bpf_get_current_pid_tgid();
> > > > +	key &= 0x00000000ffffffffUL;
> > > > +	if (key == 0)
> > > > +		key = bpf_get_smp_processor_id();
> > > > +	else
> > > > +		key += (uint32_t)(uint64_t)&NCPUS;
> > > > +
> > > > +	key++;
> > > > +	key = (key << 32) | id;
> > > > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > > > @@ -213,36 +213,21 @@ populate_probes_map(dtrace_hdl_t *dtp, int fd)
> > > >     *		use.
> > > >     * - gvars:	Global variables map.  This is a global map with a singleton
> > > >     *		element (key 0) addressed by variable offset.
> > > > + * - dvars:	Dynamic variables map.  This is a global hash map index with
> > > > + *		a unique numeric identifier for each variable.
> > > Well, variable/pid combo.
> s/index/indexed/

yes

> and again not just per variable but also per pid

No, we look at it as distict variables, and thus each numeric identifier relates
to a single variable.  So, a single variable name (identifier) maps to multiple
variables (internally).

> > > > @@ -371,10 +354,19 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> > > >    			sizeof(uint32_t), lvarsz, 1) == -1)
> > > >    		return -1;		/* dt_errno is set for us */
> > > > -	if (tvarc > 0 &&
> > > > -	    create_gmap(dtp, "tvars", BPF_MAP_TYPE_ARRAY,
> > > > -			sizeof(uint32_t), sizeof(uint64_t), tvarc) == -1)
> > > > -		return -1;		/* dt_errno is set for us */
> > > > +	if (dvarc > 0) {
> > > Speaking of which, is there a plan to get fancier on the sizing? We're
> > > allocating string space for everyone.  In many cases, that will be 256/4 or
> > > 256/8 times too much.  Meanwhile, for some structs, I guess it'll be too
> > > small.  Doesn't need to be fixed in the first patch, but maybe worth
> > > mentioning?
> > This is the challenge of doing tracing and supporting dynamic variables.  You
> > cannot really get into implementing fancy memory management.  And yes, later on
> > we will need to make the size be based on the largest datatype a variable can
> > have rather than strsize, so indeed it can be larger than strsize.
> But this should be mentioned in the commit message or somewhere.  If there
> are major limitations, they should be acknowledged.

Sure, I will add it to the commmit message.  Calling it a major limitation is
quite a stretch though.

> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > > @@ -2351,23 +2377,48 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> > > >    	/* TLS var */
> > > >    	varid = idp->di_id - DIF_VAR_OTHER_UBASE;
> > > > -	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_tvar");
> > > > +	size = idp->di_size;
> > > > +
> > > > +	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_tvar");
> > > >    	assert(idp != NULL);
> > > >    	if (dt_regset_xalloc_args(drp) == -1)
> > > >    		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > > -	/*
> > > > -	 * We assign the varid in the instruction preceding the call because
> > > > -	 * the disassembler expects this sequence in support for annotating
> > > > -	 * the disassembly with variables names.
> > > > -	 */
> > > > -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
> > > >    	emit(dlp,  BPF_MOV_IMM(BPF_REG_1, varid));
> > > >    	dt_regset_xalloc(drp, BPF_REG_0);
> > > >    	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> > > > -	dt_regset_free(drp, BPF_REG_0);
> > > >    	dt_regset_free_args(drp);
> > > > +
> > > > +	if ((reg = dt_regset_alloc(drp)) == -1)
> > > > +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > > +
> > > > +	emit(dlp, BPF_MOV_REG(reg, BPF_REG_0));
> > > > +	dt_regset_free(drp, BPF_REG_0);
> > > > +
> > > > +	dt_cg_check_notnull(dlp, drp, reg);
> > > > +
> > > > +	if (dnp->dn_flags & DT_NF_REF) {
> > > > +		size_t		srcsz;
> > > > +
> > > > +		/*
> > > > +		 * Determine the amount of data to be copied.  It is
> > > > +		 * the lesser of the size of the identifier and the
> > > > +		 * size of the data being copied in.
> > > > +		 */
> > > > +		srcsz = dt_node_type_size(dnp->dn_right);
> > > > +		if (dt_node_is_string(dnp))
> > > > +			srcsz += DT_STRLEN_BYTES;
> > > > +		size = MIN(srcsz, size);
> > > I think neither "srcsz = dt_node_type_size(dnp->dn_right)" nor "size =
> > > idp->di_size" includes the NUL terminating byte.  Same with the pre-existing
> > > gvar/lvar code.
> > This is actually an interesting situation because dt_node_type_size() does
> > include the terminating NUL byte for string constants but not for regular
> > string types.  And di_size generally will not contain it either.  And as you
> > say, this applies to gvar and lvar as well.  So, I am inclined to leave it as
> > is in this patch and simply fix it for all three cases with a single patch.
> How do these bugs get tracked?

You file a bug for it :)

> > > > +
> > > > +		dt_cg_memcpy(dlp, drp, reg, dnp->dn_reg, size);
> > > > +	} else {
> > > > +		assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> > > > +
> > > > +		emit(dlp, BPF_STORE(ldstw[size], reg, 0, dnp->dn_reg));
> > > > +	}
> > > > +
> > > > +	dt_regset_free(drp, reg);
> > > >    }
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel