Re: [DTrace-devel] [PATCH] Move proc lock to where we actually find a USDT process

[Kris Van Hees <kris.van.hees@oracle.com>]

On Mon, Jan 06, 2025 at 10:55:25AM -0500, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The function dt_pid_create_usdt_probes_proc() creates USDT probes
> for a specific process.  It is called in one of two ways:
> 
>     dt_proc_scan(dtp, dpr) ->
>     dt_pid_create_probes_module(dtp, dpr)
> 
>         The process has been locked and we are updating its
>         USDT probes.
> 
>     dt_pid_create_usdt_probes(pdp, dtp, pcb)
> 
>         Here, we look for any pids that might have USDT probes,
>         calling the pid-specific function for each candidate.
> 
> One problem is that the first code path assumes the process is locked.
> This means the second code path has to lock processes even before we
> know if it has any USDT probes we care about.
> 
> Change dt_pid_create_usdt_probes_proc() so the caller can specify the
> process in either one of two ways:
> 
>     by pid (implying the process has not been locked)
> 
>     by dpr (implying the process has been locked)
> 
> In the first case, the function will lock the process, but only if
> USDT probes have been found.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_pid.c | 70 +++++++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 8110ccead..6db882059 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -782,33 +782,42 @@ validate_dof_record(const char *path, const dof_parsed_t *parsed,
>  
>  
>  /*
> - * Create underlying probes relating to the probespec passed on input.
> + * Create underlying probes relating to the probe description passed on input.
> + * Just set up probes relating to mappings found in this one process.
>   *
> - * dpr must be set and locked.  Just set up probes relating to mappings found
> - * in this one process.
> + * Either the pid must be specified or else dpr must be set and locked.
>   *
>   * Return 0 on success or -1 on error.  (Failure to create specific underlying
>   * probes is not an error.)
>   */
>  static int
> -dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> +dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, pid_t pid, dt_proc_t *dpr,
>  			       dtrace_probedesc_t *pdp, dt_pcb_t *pcb)
>  {
>  	const dt_provider_t *pvp;
>  	int ret = 0;
> +	int dpr_caller;		/* dpr was set by caller */
>  	char *probepath = NULL;
>  	glob_t probeglob = {0};
>  
> -	assert(dpr != NULL && dpr->dpr_proc);
> -	assert(MUTEX_HELD(&dpr->dpr_lock));
> +	if (dpr == NULL) {
> +		assert(pid != -1);
> +		dpr_caller = 0;
> +	} else {
> +		assert(pid == -1);
> +		assert(dpr->dpr_proc);
> +		assert(MUTEX_HELD(&dpr->dpr_lock));
> +		pid = dpr->dpr_pid;
> +		dpr_caller = 1;
> +	}
>  
>  	dt_dprintf("Scanning for usdt probes in %i matching %s:%s:%s\n",
> -		   dpr->dpr_pid, pdp->mod, pdp->fun, pdp->prb);
> +		   pid, pdp->mod, pdp->fun, pdp->prb);
>  
>  	pvp = dt_provider_lookup(dtp, "usdt");
>  	assert(pvp != NULL);
>  
> -	if (Pstate(dpr->dpr_proc) == PS_DEAD)
> +	if (dpr != NULL && Pstate(dpr->dpr_proc) == PS_DEAD)
>  		return 0;
>  
>  	/*
> @@ -835,7 +844,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>  	assert(pvp->impl != NULL && pvp->impl->provide_probe != NULL);
>  
>  	if (asprintf(&probepath, "%s/probes/%i/%s/%s/%s/%s", dtp->dt_dofstash_path,
> -		     dpr->dpr_pid, pdp->prv[0] == '\0' ? "*" : pdp->prv,
> +		     pid, pdp->prv[0] == '\0' ? "*" : pdp->prv,
>  		     pdp->mod[0] == '\0' ? "*" : pdp->mod,
>  		     pdp->fun[0] == '\0' ? "*" : pdp->fun,
>  		     pdp->prb[0] == '\0' ? "*" : pdp->prb) < 0)
> @@ -858,6 +867,19 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>  		return 0;
>  	}
>  
> +	/* Set dpr and grab the process, if necessary. */
> +	if (dpr_caller == 0) {
> +		if (dt_proc_grab_lock(dtp, pid, DTRACE_PROC_WAITING |
> +				      DTRACE_PROC_SHORTLIVED) < 0) {
> +			dt_pid_error(dtp, pcb, NULL, D_PROC_GRAB,
> +			    "failed to grab process %d", (int)pid);
> +			return -1;
> +		}
> +		dpr = dt_proc_lookup(dtp, pid);
> +		assert(dpr != NULL);
> +	}
> +
> +	/* Loop over USDT probes. */
>  	for (size_t i = 0; i < probeglob.gl_pathc; i++) {
>  		char *dof_buf = NULL, *p;
>  		struct stat s;
> @@ -1051,10 +1073,16 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>  		free(path);
>  		free(dof_buf);
>  		globfree(&probeglob);
> +		if (dpr_caller == 0)
> +			dt_proc_release_unlock(dtp, pid);
>  		return -1;
>  	}
>  
>  	globfree(&probeglob);
> +	if (dpr_caller == 0) {
> +		dt_pid_fix_mod(NULL, pdp, dtp, pid);
> +		dt_proc_release_unlock(dtp, pid);
> +	}
>  	return ret;
>  
>  scan_err:
> @@ -1237,7 +1265,6 @@ dt_pid_create_usdt_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_t *
>  			  + strlen(dtp->dt_dofstash_path)
>  			  + strlen("/probes/");
>  		pid_t pid;
> -		dt_proc_t *dpr;
>  		dtrace_probedesc_t pdptmp;
>  
>  		/* Pull out the pid. */
> @@ -1247,28 +1274,15 @@ dt_pid_create_usdt_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_t *
>  		if (!Pexists(pid))
>  			continue;
>  
> -		/* Grab the process. */
> -		if (dt_proc_grab_lock(dtp, pid, DTRACE_PROC_WAITING |
> -				      DTRACE_PROC_SHORTLIVED) < 0) {
> -			dt_pid_error(dtp, pcb, NULL, D_PROC_GRAB,
> -			    "failed to grab process %d", (int)pid);
> -			err = 1;  // FIXME but do we want to set the error if we end up return 0?
> -			continue;
> -		}
> -		dpr = dt_proc_lookup(dtp, pid);
> -		assert(dpr != NULL);
> -
> -		/* Create USDT probes for this process. */
> +		/* Construct the probe descriptor. */
>  		pdptmp.prv = strchr(s, '/') + 1;
>  		pdptmp.mod = pdp->mod[0] == '\0' ? "*" : pdp->mod;
>  		pdptmp.fun = pdp->fun[0] == '\0' ? "*" : pdp->fun;
>  		pdptmp.prb = pdp->prb[0] == '\0' ? "*" : pdp->prb;
> -		if (dt_pid_create_usdt_probes_proc(dtp, dpr, &pdptmp, pcb))
> -			err = 1;
> -
> -		dt_pid_fix_mod(NULL, &pdptmp, dtp, dpr->dpr_pid);
>  
> -		dt_proc_release_unlock(dtp, pid);
> +		/* Create USDT probes for this process. */
> +		if (dt_pid_create_usdt_probes_proc(dtp, pid, NULL, &pdptmp, pcb))
> +			err = 1;
>  	}
>  	free(globpat);
>  	globfree(&globbuf);
> @@ -1341,7 +1355,7 @@ dt_pid_create_probes_module(dtrace_hdl_t *dtp, dt_proc_t *dpr)
>  			 * a USDT provider.
>  			 */
>  			if (strcmp(provname, pdp->prv) != 0) {
> -				if (dt_pid_create_usdt_probes_proc(dtp, dpr, pdp, NULL) < 0)
> +				if (dt_pid_create_usdt_probes_proc(dtp, -1, dpr, pdp, NULL) < 0)
>  					ret = 1;
>  				else
>  					dt_pid_fix_mod(NULL, pdp, dtp, dpr->dpr_pid);
> -- 
> 2.43.5
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel