[DTrace-devel] [PATCH 1/3] Revert "Exit properly when all dtrace -c processes end, even if waitfd() failed."
Kris Van Hees
kris.van.hees at oracle.com
Wed Nov 8 05:34:00 UTC 2023
On Tue, Oct 31, 2023 at 01:51:39PM +0000, Nick Alcock wrote:
> This reverts commit e856890a68c5b2461f30499295f4f8583b8efed4.
>
> We will no longer need it when waitfd is gone.
>
> This also removes a whole pile of complexity where we might sometimes have
> to synchronously check to see if the process has exited, simply because
> we couldn't rely on waitfd() working everywhere.
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_consume.c | 6 +--
> libdtrace/dt_proc.c | 95 ++++--------------------------------------
> libdtrace/dt_proc.h | 2 -
> 3 files changed, 11 insertions(+), 92 deletions(-)
>
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 1edca2925d840..f5dfc373caea7 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -2956,12 +2956,10 @@ dt_consume_proc_exits(dtrace_hdl_t *dtp)
> dt_proc_notify_t *dprn;
>
> /*
> - * Make sure that any synchronous notifications of process exit are
> - * received. Regardless of why we awaken, iterate over any pending
> - * notifications and process them.
> + * Iterate over any pending notifications that may have accumulated
> + * while we were asleep, and process them.
> */
> pthread_mutex_lock(&dph->dph_lock);
> - dt_proc_enqueue_exits(dtp);
>
> while ((dprn = dph->dph_notify) != NULL) {
> if (dtp->dt_prochdlr != NULL) {
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index bb0253f1b6b2a..ca586b1174ccf 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -129,7 +129,7 @@ dt_unwinder_pad(struct ps_prochandle *unused)
>
> static void
> dt_proc_notify(dtrace_hdl_t *dtp, dt_proc_hash_t *dph, dt_proc_t *dpr,
> - pid_t pid, const char *msg, int lock, int broadcast)
> + pid_t pid, const char *msg)
> {
> dt_proc_notify_t *dprn = dt_alloc(dtp, sizeof(dt_proc_notify_t));
>
> @@ -144,82 +144,14 @@ dt_proc_notify(dtrace_hdl_t *dtp, dt_proc_hash_t *dph, dt_proc_t *dpr,
> strlcpy(dprn->dprn_errmsg, msg,
> sizeof(dprn->dprn_errmsg));
>
> - if (lock)
> - pthread_mutex_lock(&dph->dph_lock);
> + pthread_mutex_lock(&dph->dph_lock);
>
> dprn->dprn_next = dph->dph_notify;
> dprn->dprn_pid = pid;
> dph->dph_notify = dprn;
>
> - if (broadcast)
> - eventfd_write(dtp->dt_proc_fd, 1);
> - if (lock)
> - pthread_mutex_unlock(&dph->dph_lock);
> - }
> -}
> -
> -/*
> - * Wait for all processes in the dtp's hash table that are marked as dpr_created
> - * but have no controlling thread. (These processes are counted in the dph, so
> - * in the common case that there are none, this function can exit early and do
> - * nothing.)
> - *
> - * Must be called under the dph_lock by the function that scans the notification
> - * table, since no notifications are sent, only enqueued.
> - */
> -void
> -dt_proc_enqueue_exits(dtrace_hdl_t *dtp)
> -{
> - dt_proc_hash_t *dph = dtp->dt_procs;
> - dt_proc_t *dpr;
> -
> - if (!dph->dph_noninvasive_created)
> - return;
> -
> - assert(MUTEX_HELD(&dph->dph_lock));
> - /*
> - * We can cheat here and save time by not taking out the
> - * dt_proc_lock, because in the case we are interested in there is
> - * no control thread to lock against, and in the only other case where
> - * dpr_done can end up set and dpr_tid be NULL, the control thread has
> - * died and the process has already been waited for.
> - *
> - * We also know that the process cannot be ptrace()d in this case, so
> - * there is no danger of a return indicating a trace stop.
> - */
> - for (dpr = dt_list_next(&dph->dph_lrulist);
> - dpr != NULL; dpr = dt_list_next(dpr)) {
> - if (dpr->dpr_tid == 0 && dpr->dpr_done && dpr->dpr_proc) {
> - int exited = 0;
> - pid_t pid = Pgetpid(dpr->dpr_proc);
> - siginfo_t info;
> -
> - info.si_pid = 0;
> - /*
> - * We treat -ECHILD like a process exit, and consider
> - * other errors a real problem.
> - */
> - if (waitid(P_PID, pid, &info, WNOHANG | WEXITED) < 0) {
> - if (errno != -ECHILD)
> - dt_dprintf("Error waitid()ing for child "
> - "%i: %s\n", Pgetpid(dpr->dpr_proc),
> - strerror(errno));
> - else
> - exited = 1;
> - }
> -
> - if (info.si_pid != 0)
> - exited = 1;
> -
> - if (exited) {
> - dt_proc_notify(dtp, dph, dpr, pid, NULL,
> - B_FALSE, B_FALSE);
> - Prelease(dpr->dpr_proc,
> - dpr->dpr_created ? PS_RELEASE_KILL :
> - PS_RELEASE_NORMAL);
> - dph->dph_noninvasive_created--;
> - }
> - }
> + eventfd_write(dtp->dt_proc_fd, 1);
> + pthread_mutex_unlock(&dph->dph_lock);
> }
> }
>
> @@ -417,7 +349,7 @@ dt_proc_scan(dtrace_hdl_t *dtp, dt_proc_t *dpr)
> Pupdate_syms(dpr->dpr_proc);
> if (dt_pid_create_probes_module(dtp, dpr) != 0)
> dt_proc_notify(dtp, dtp->dt_procs, dpr, dpr->dpr_pid,
> - dpr->dpr_errmsg, B_TRUE, B_TRUE);
> + dpr->dpr_errmsg);
> }
>
> /*
> @@ -918,9 +850,7 @@ dt_proc_control(void *arg)
> /*
> * If this was a noninvasive grab, quietly exit without calling
> * the cleanup handlers: the process is running, but does not
> - * need a monitoring thread. Process termination detection is
> - * handled in the parent process, via the subreaper already set
> - * up.
> + * need a monitoring thread.
> */
> if (noninvasive && !Ptraceable(dpr->dpr_proc)) {
> dt_dprintf("%i: noninvasive grab, control thread "
> @@ -948,9 +878,6 @@ dt_proc_control(void *arg)
> * Pcreate()d it, dpr_created is still set, so it will still get
> * killed on dtrace exit. If even that fails, there's nothing
> * we can do but hope.
> - *
> - * The dt_consume_proc_exits() function, called by
> - * dtrace_consume(), checks for termination of such processes * (since nothing else will).
> */
> Prelease(dpr->dpr_proc, PS_RELEASE_NORMAL);
> if ((dpr->dpr_proc = Pgrab(dpr->dpr_pid, 2, 0,
> @@ -959,7 +886,6 @@ dt_proc_control(void *arg)
> (long)dpr->dpr_pid, strerror(err));
> }
>
> - dtp->dt_procs->dph_noninvasive_created++;
> pthread_exit(NULL);
> }
>
> @@ -1400,8 +1326,7 @@ dt_proc_control_cleanup(void *arg)
> */
> if (!suiciding && dpr->dpr_notifiable)
> dt_proc_notify(dpr->dpr_hdl, dpr->dpr_hdl->dt_procs,
> - dpr->dpr_proc ? dpr : NULL, dpr->dpr_pid, NULL,
> - B_TRUE, B_TRUE);
> + dpr->dpr_proc ? dpr : NULL, dpr->dpr_pid, NULL);
>
> /*
> * Signal on the cv, waking up any waiters once the lock is released.
> @@ -1611,14 +1536,12 @@ dt_proc_destroy(dtrace_hdl_t *dtp, dt_proc_t *dpr)
> */
> proxy_quit(dpr, 0);
> dpr->dpr_lock_holder = pthread_self();
> - } else if (dpr->dpr_proc) {
> + } else {
> /*
> * The process control thread is already dead, but try to clean
> * the process up anyway, just in case it survived to this
> * point. This can happen e.g. if the process was noninvasively
> * grabbed and its control thread suicided.)
> - *
> - * It might be cleaned up already by dt_consume_proc_exits().
> */
> Prelease(dpr->dpr_proc, dpr->dpr_created ? PS_RELEASE_KILL :
> PS_RELEASE_NORMAL);
> @@ -1760,7 +1683,7 @@ dt_proc_create(dtrace_hdl_t *dtp, const char *file, char *const *argv,
> }
>
> /*
> - * Newly-created processes should be invasively grabbed.
> + * Newly-created processes must be invasively grabbed.
> */
> if (flags & DTRACE_PROC_SHORTLIVED)
> flags &= ~DTRACE_PROC_SHORTLIVED;
> diff --git a/libdtrace/dt_proc.h b/libdtrace/dt_proc.h
> index 77e65145df7a1..a08922bc68483 100644
> --- a/libdtrace/dt_proc.h
> +++ b/libdtrace/dt_proc.h
> @@ -138,7 +138,6 @@ typedef struct dt_proc_hash {
> uint_t dph_lrulim; /* limit on number of procs to hold */
> uint_t dph_lrucnt; /* count of cached process handles */
> uint_t dph_hashlen; /* size of hash chains array */
> - uint_t dph_noninvasive_created; /* count of noninvasive -c procs */
> dt_proc_t *dph_hash[1]; /* hash chains array */
> } dt_proc_hash_t;
>
> @@ -147,7 +146,6 @@ extern void dt_proc_release_unlock(dtrace_hdl_t *, pid_t);
> extern void dt_proc_lock(dt_proc_t *dpr);
> extern void dt_proc_unlock(dt_proc_t *dpr);
> extern dt_proc_t *dt_proc_lookup(dtrace_hdl_t *, pid_t);
> -extern void dt_proc_enqueue_exits(dtrace_hdl_t *dtp);
>
> /*
> * Proxies for operations in libproc, respecting the execve-retry protocol.
> --
> 2.42.0.271.g85384428f1
>
>
More information about the DTrace-devel
mailing list