Re: [rtla 07/13] rtla: Introduce timerlat_restart() helper

[Wander Lairson Costa <wander@redhat.com>]

On Mon, Nov 24, 2025 at 9:46 PM Crystal Wood <crwood@redhat.com> wrote:
>
> On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
>
> > +enum restart_result
> > +timerlat_restart(const struct osnoise_tool *tool, struct timerlat_params *params)
> > +{
> > +     actions_perform(&params->common.threshold_actions);
> > +
> > +     if (!params->common.threshold_actions.continue_flag)
> > +             /* continue flag not set, break */
> > +             return RESTART_STOP;
> > +
> > +     /* continue action reached, re-enable tracing */
> > +     if (tool->record && trace_instance_start(&tool->record->trace))
> > +             goto err;
> > +     if (tool->aa && trace_instance_start(&tool->aa->trace))
> > +             goto err;
> > +     return RESTART_OK;
> > +
> > +err:
> > +     err_msg("Error restarting trace\n");
> > +     return RESTART_ERROR;
> > +}
>
> The non-BPF functions in common.c have the same logic and should also
> call this.  This isn't timerlat-specific.
>

I will replace them here, thanks.

>
> > diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
> > index 09a3da3f58630..f14fc56c5b4a5 100644
> > --- a/tools/tracing/rtla/src/timerlat_hist.c
> > +++ b/tools/tracing/rtla/src/timerlat_hist.c
> > @@ -1165,18 +1165,19 @@ static int timerlat_hist_bpf_main_loop(struct osnoise_tool *tool)
> >
> >               if (!stop_tracing) {
> >                       /* Threshold overflow, perform actions on threshold */
> > -                     actions_perform(&params->common.threshold_actions);
> > +                     enum restart_result result;
> >
> > -                     if (!params->common.threshold_actions.continue_flag)
> > -                             /* continue flag not set, break */
> > +                     result = timerlat_restart(tool, params);
> > +                     if (result == RESTART_STOP)
> >                               break;
> >
> > -                     /* continue action reached, re-enable tracing */
> > -                     if (tool->record)
> > -                             trace_instance_start(&tool->record->trace);
> > -                     if (tool->aa)
> > -                             trace_instance_start(&tool->aa->trace);
> > -                     timerlat_bpf_restart_tracing();
> > +                     if (result == RESTART_ERROR)
> > +                             return -1;
>
> Does it matter that we're not detaching on an error here?  Is this
> something that gets cleaned up automatically (and if so, why do we ever
> need to do it explicitly)?
>

On process exit, it does.

> > +
> > +                     if (timerlat_bpf_restart_tracing()) {
> > +                             err_msg("Error restarting BPF trace\n");
> > +                             return -1;
> > +                     }
>
> [insert rant about not being able to use exceptions in userspace code in
> the year 2025]
>

I actually find exceptions an anti-pattern. Modern languages like Zig,
Go and Rust came back to error returning.

> -Crystal
>