diff mbox series

[v5,10/12] eu-stacktrace [10/12]: use dwflst_tracker_find_pid

Message ID 20250424214715.306147-11-serhei@serhei.io
State Superseded
Headers
Series [v5,01/12] libebl [1/12]: api for perf register handling, start with x86_64 |

Commit Message

Serhei Makarov April 24, 2025, 9:47 p.m. UTC 
  Changes for v4:

- Separate out libdwfl_stacktrace, as requested.

* * *

Initial minimal change to ensure dwflst_tracker_find_pid is
tested. For now, we keep the additional dwfltab implementation in
stacktrace.c, since it's being used to track statistics.

In future follow-ups, it will be good to switch to storing
eu-stacktrace statistics e.g. in dwfl->process->callbacks_arg. This
requires some additional design to keep the statistics from being lost
when a pid is reused and the corresponding processtracker table entry
is replaced.

* src/stacktrace.c (sysprof_init_dwfl): New function.
  (sysprof_find_dwfl): Rename the existing sysprof_init_dwfl.
  Also use dwflst_tracker_find_pid with callback.
  (sysprof_unwind_cb): Rename the existing sysprof_init_dwfl.
---
 src/stacktrace.c | 63 +++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

Comments

Aaron Merey April 25, 2025, 6 a.m. UTC | #1 
On Thu, Apr 24, 2025 at 5:49 PM Serhei Makarov <serhei@serhei.io> wrote:
>
> Changes for v4:
>
> - Separate out libdwfl_stacktrace, as requested.
>
> * * *
>
> Initial minimal change to ensure dwflst_tracker_find_pid is
> tested. For now, we keep the additional dwfltab implementation in
> stacktrace.c, since it's being used to track statistics.
>
> In future follow-ups, it will be good to switch to storing
> eu-stacktrace statistics e.g. in dwfl->process->callbacks_arg. This
> requires some additional design to keep the statistics from being lost
> when a pid is reused and the corresponding processtracker table entry
> is replaced.
>
> * src/stacktrace.c (sysprof_init_dwfl): New function.
>   (sysprof_find_dwfl): Rename the existing sysprof_init_dwfl.
>   Also use dwflst_tracker_find_pid with callback.
>   (sysprof_unwind_cb): Rename the existing sysprof_init_dwfl.
> ---
>  src/stacktrace.c | 63 +++++++++++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/src/stacktrace.c b/src/stacktrace.c
> index c0c9929d..dd58ef3b 100644
> --- a/src/stacktrace.c
> +++ b/src/stacktrace.c
> @@ -96,7 +96,7 @@
>  #include ELFUTILS_HEADER(ebl)
>  /* #include ELFUTILS_HEADER(dwfl) */
>  #include "../libdwfl/libdwflP.h"
> -/* XXX: Private header needed for find_procfile, sysprof_init_dwfl,
> +/* XXX: Private header needed for find_procfile, sysprof_find_dwfl,
>     sample_set_initial_registers. */
>  #include ELFUTILS_HEADER(dwfl_stacktrace)
>
> @@ -1014,7 +1014,34 @@ find_procfile (Dwfl *dwfl, pid_t *pid, Elf **elf, int *elf_fd)
>  }
>
>  Dwfl *
> -sysprof_init_dwfl (struct sysprof_unwind_info *sui,
> +sysprof_init_dwfl (Dwflst_Process_Tracker *cb_tracker,

Just a nit but I would include _cb or _callback in the name to make it
more clear this function is a callback. Otherwise LGTM.

> +                   pid_t pid,
> +                   void *arg __attribute__ ((unused)))
> +{
> +  Dwfl *dwfl = dwflst_tracker_dwfl_begin (cb_tracker);
> +
> +  int err = dwfl_linux_proc_report (dwfl, pid);
> +  if (err < 0)
> +    {
> +      if (show_failures)
> +       fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
> +               (long long) pid, dwfl_errmsg (-1));
> +      return NULL;
> +    }
> +  err = dwfl_report_end (dwfl, NULL, NULL);
> +  if (err != 0)
> +    {
> +      if (show_failures)
> +       fprintf(stderr, "dwfl_report_end pid %lld: %s",
> +               (long long) pid, dwfl_errmsg (-1));
> +      return NULL;
> +    }
> +
> +  return dwfl;
> +}
> +
> +Dwfl *
> +sysprof_find_dwfl (struct sysprof_unwind_info *sui,
>                    SysprofCaptureStackUser *ev,
>                    SysprofCaptureUserRegs *regs)
>  {
> @@ -1027,42 +1054,24 @@ sysprof_init_dwfl (struct sysprof_unwind_info *sui,
>    if (regs->n_regs < EXPECTED_REGS) /* XXX expecting everything except FLAGS */
>      {
>        if (show_failures)
> -       fprintf(stderr, N_("sysprof_init_dwfl: n_regs=%d, expected %d\n"),
> +       fprintf(stderr, N_("sysprof_find_dwfl: n_regs=%d, expected %d\n"),
>                 regs->n_regs, EXPECTED_REGS);
>        return NULL;
>      }
>
> -  Dwfl *dwfl = pid_find_dwfl(pid);
> +  Dwfl *dwfl = dwflst_tracker_find_pid (tracker, pid, sysprof_init_dwfl, NULL);
>    struct __sample_arg *sample_arg;
>    bool cached = false;
> -  if (dwfl != NULL)
> +  if (dwfl != NULL && dwfl->process != NULL)
>      {
>        sample_arg = dwfl->process->callbacks_arg; /* XXX requires libdwflP.h */
>        cached = true;
>        goto reuse;
>      }
> -  dwfl = dwflst_tracker_dwfl_begin (tracker);
> -
> -  int err = dwfl_linux_proc_report (dwfl, pid);
> -  if (err < 0)
> -    {
> -      if (show_failures)
> -       fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
> -               (long long) pid, dwfl_errmsg (-1));
> -      return NULL;
> -    }
> -  err = dwfl_report_end (dwfl, NULL, NULL);
> -  if (err != 0)
> -    {
> -      if (show_failures)
> -       fprintf(stderr, "dwfl_report_end pid %lld: %s",
> -               (long long) pid, dwfl_errmsg (-1));
> -      return NULL;
> -    }
>
>    Elf *elf = NULL;
>    int elf_fd = -1;
> -  err = find_procfile (dwfl, &pid, &elf, &elf_fd);
> +  int err = find_procfile (dwfl, &pid, &elf, &elf_fd);
>    if (err < 0)
>      {
>        if (show_failures)
> @@ -1099,7 +1108,7 @@ sysprof_init_dwfl (struct sysprof_unwind_info *sui,
>
>    if (show_frames) {
>      bool is_abi32 = (sample_arg->abi == PERF_SAMPLE_REGS_ABI_32);
> -    fprintf(stderr, "sysprof_init_dwfl pid %lld%s: size=%ld%s pc=%lx sp=%lx+(%lx)\n",
> +    fprintf(stderr, "sysprof_find_dwfl pid %lld%s: size=%ld%s pc=%lx sp=%lx+(%lx)\n",
>             (long long) pid, cached ? " (cached)" : "",
>             sample_arg->size, is_abi32 ? " (32-bit)" : "",
>             sample_arg->pc, sample_arg->base_addr,
> @@ -1260,7 +1269,7 @@ sysprof_unwind_cb (SysprofCaptureFrame *frame, void *arg)
>    SysprofCaptureUserRegs *regs = (SysprofCaptureUserRegs *)tail_ptr;
>    if (show_frames)
>      fprintf(stderr, "\n"); /* extra newline for padding */
> -  Dwfl *dwfl = sysprof_init_dwfl (sui, ev, regs);
> +  Dwfl *dwfl = sysprof_find_dwfl (sui, ev, regs);
>    if (dwfl == NULL)
>      {
>        if (show_summary)
> @@ -1270,7 +1279,7 @@ sysprof_unwind_cb (SysprofCaptureFrame *frame, void *arg)
>           dwfl_ent->lost_samples++;
>         }
>        if (show_failures)
> -       fprintf(stderr, "sysprof_init_dwfl pid %lld (%s) (failed)\n",
> +       fprintf(stderr, "sysprof_find_dwfl pid %lld (%s) (failed)\n",
>                 (long long)frame->pid, comm);
>        return SYSPROF_CB_OK;
>      }
> --
> 2.47.0
>

Aaron
diff mbox series

Patch

diff --git a/src/stacktrace.c b/src/stacktrace.c
index c0c9929d..dd58ef3b 100644
--- a/src/stacktrace.c
+++ b/src/stacktrace.c
@@ -96,7 +96,7 @@ 
 #include ELFUTILS_HEADER(ebl)
 /* #include ELFUTILS_HEADER(dwfl) */
 #include "../libdwfl/libdwflP.h"
-/* XXX: Private header needed for find_procfile, sysprof_init_dwfl,
+/* XXX: Private header needed for find_procfile, sysprof_find_dwfl,
    sample_set_initial_registers. */
 #include ELFUTILS_HEADER(dwfl_stacktrace)
 
@@ -1014,7 +1014,34 @@  find_procfile (Dwfl *dwfl, pid_t *pid, Elf **elf, int *elf_fd)
 }
 
 Dwfl *
-sysprof_init_dwfl (struct sysprof_unwind_info *sui,
+sysprof_init_dwfl (Dwflst_Process_Tracker *cb_tracker,
+                   pid_t pid,
+                   void *arg __attribute__ ((unused)))
+{
+  Dwfl *dwfl = dwflst_tracker_dwfl_begin (cb_tracker);
+
+  int err = dwfl_linux_proc_report (dwfl, pid);
+  if (err < 0)
+    {
+      if (show_failures)
+	fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
+		(long long) pid, dwfl_errmsg (-1));
+      return NULL;
+    }
+  err = dwfl_report_end (dwfl, NULL, NULL);
+  if (err != 0)
+    {
+      if (show_failures)
+	fprintf(stderr, "dwfl_report_end pid %lld: %s",
+		(long long) pid, dwfl_errmsg (-1));
+      return NULL;
+    }
+
+  return dwfl;
+}
+
+Dwfl *
+sysprof_find_dwfl (struct sysprof_unwind_info *sui,
 		   SysprofCaptureStackUser *ev,
 		   SysprofCaptureUserRegs *regs)
 {
@@ -1027,42 +1054,24 @@  sysprof_init_dwfl (struct sysprof_unwind_info *sui,
   if (regs->n_regs < EXPECTED_REGS) /* XXX expecting everything except FLAGS */
     {
       if (show_failures)
-	fprintf(stderr, N_("sysprof_init_dwfl: n_regs=%d, expected %d\n"),
+	fprintf(stderr, N_("sysprof_find_dwfl: n_regs=%d, expected %d\n"),
 		regs->n_regs, EXPECTED_REGS);
       return NULL;
     }
 
-  Dwfl *dwfl = pid_find_dwfl(pid);
+  Dwfl *dwfl = dwflst_tracker_find_pid (tracker, pid, sysprof_init_dwfl, NULL);
   struct __sample_arg *sample_arg;
   bool cached = false;
-  if (dwfl != NULL)
+  if (dwfl != NULL && dwfl->process != NULL)
     {
       sample_arg = dwfl->process->callbacks_arg; /* XXX requires libdwflP.h */
       cached = true;
       goto reuse;
     }
-  dwfl = dwflst_tracker_dwfl_begin (tracker);
-
-  int err = dwfl_linux_proc_report (dwfl, pid);
-  if (err < 0)
-    {
-      if (show_failures)
-	fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
-		(long long) pid, dwfl_errmsg (-1));
-      return NULL;
-    }
-  err = dwfl_report_end (dwfl, NULL, NULL);
-  if (err != 0)
-    {
-      if (show_failures)
-	fprintf(stderr, "dwfl_report_end pid %lld: %s",
-		(long long) pid, dwfl_errmsg (-1));
-      return NULL;
-    }
 
   Elf *elf = NULL;
   int elf_fd = -1;
-  err = find_procfile (dwfl, &pid, &elf, &elf_fd);
+  int err = find_procfile (dwfl, &pid, &elf, &elf_fd);
   if (err < 0)
     {
       if (show_failures)
@@ -1099,7 +1108,7 @@  sysprof_init_dwfl (struct sysprof_unwind_info *sui,
 
   if (show_frames) {
     bool is_abi32 = (sample_arg->abi == PERF_SAMPLE_REGS_ABI_32);
-    fprintf(stderr, "sysprof_init_dwfl pid %lld%s: size=%ld%s pc=%lx sp=%lx+(%lx)\n",
+    fprintf(stderr, "sysprof_find_dwfl pid %lld%s: size=%ld%s pc=%lx sp=%lx+(%lx)\n",
 	    (long long) pid, cached ? " (cached)" : "",
 	    sample_arg->size, is_abi32 ? " (32-bit)" : "",
 	    sample_arg->pc, sample_arg->base_addr,
@@ -1260,7 +1269,7 @@  sysprof_unwind_cb (SysprofCaptureFrame *frame, void *arg)
   SysprofCaptureUserRegs *regs = (SysprofCaptureUserRegs *)tail_ptr;
   if (show_frames)
     fprintf(stderr, "\n"); /* extra newline for padding */
-  Dwfl *dwfl = sysprof_init_dwfl (sui, ev, regs);
+  Dwfl *dwfl = sysprof_find_dwfl (sui, ev, regs);
   if (dwfl == NULL)
     {
       if (show_summary)
@@ -1270,7 +1279,7 @@  sysprof_unwind_cb (SysprofCaptureFrame *frame, void *arg)
 	  dwfl_ent->lost_samples++;
 	}
       if (show_failures)
-	fprintf(stderr, "sysprof_init_dwfl pid %lld (%s) (failed)\n",
+	fprintf(stderr, "sysprof_find_dwfl pid %lld (%s) (failed)\n",
 		(long long)frame->pid, comm);
       return SYSPROF_CB_OK;
     }