From: John Kacur <jkacur@redhat.com>
To: Tomas Glozar <tglozar@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
linux-trace-kernel@vger.kernel.org,
linux-kernel@vger.kernel.org,
Luis Goncalves <lgoncalv@redhat.com>
Subject: Re: [PATCH 4/6] rtla: Always set all tracer options
Date: Thu, 20 Mar 2025 16:31:42 -0400 (EDT) [thread overview]
Message-ID: <8e41f0ee-7534-eedb-0a67-184ac195baf4@redhat.com> (raw)
In-Reply-To: <20250320092500.101385-5-tglozar@redhat.com>
On Thu, 20 Mar 2025, Tomas Glozar wrote:
> rtla currently only sets tracer options that are explicitly set by the
> user, with the exception of OSNOISE_WORKLOAD.
>
> This leads to improper behavior in case rtla is run with those options
> not set to the default value. rtla does reset them to the original
> value upon exiting, but that does not protect it from starting with
> non-default values set either by an improperly exited rtla or by another
> user of the tracers.
>
> For example, after running this command:
>
> $ echo 1 > /sys/kernel/tracing/osnoise/stop_tracing_us
>
> all runs of rtla will stop at the 1us threshold, even if not requested
> by the user:
>
> $ rtla osnoise hist
> Index CPU-000 CPU-001
> 1 8 5
> 2 5 9
> 3 1 2
> 4 6 1
> 5 2 1
> 6 0 1
> 8 1 1
> 12 0 1
> 14 1 0
> 15 1 0
> over: 0 0
> count: 25 21
> min: 1 1
> avg: 3.68 3.05
> max: 15 12
> rtla osnoise hit stop tracing
>
> Fix the problem by setting the default value for all tracer options if
> the user has not provided their own value.
>
> For most of the options, it's enough to just drop the if clause checking
> for the value being set. For cpus, "all" is used as the default value,
> and for osnoise default period and runtime, default values of
> the osnoise_data variable in trace_osnoise.c are used.
>
> Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode")
> Fixes: 829a6c0b5698 ("rtla/osnoise: Add the hist mode")
> Fixes: a828cd18bc4a ("rtla: Add timerlat tool and timelart top mode")
> Fixes: 1eeb6328e8b3 ("rtla/timerlat: Add timerlat hist mode")
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
> tools/tracing/rtla/src/osnoise.c | 56 ++++++++++++++---------------
> tools/tracing/rtla/src/timerlat.c | 59 +++++++++++++++----------------
> 2 files changed, 56 insertions(+), 59 deletions(-)
>
> diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
> index a71618d876e9..2dc3e4539e99 100644
> --- a/tools/tracing/rtla/src/osnoise.c
> +++ b/tools/tracing/rtla/src/osnoise.c
> @@ -17,6 +17,9 @@
>
> #include "osnoise.h"
>
> +#define DEFAULT_SAMPLE_PERIOD 1000000 /* 1s */
> +#define DEFAULT_SAMPLE_RUNTIME 1000000 /* 1s */
> +
> /*
> * osnoise_get_cpus - return the original "osnoise/cpus" content
> *
> @@ -1127,46 +1130,43 @@ osnoise_apply_config(struct osnoise_tool *tool, struct osnoise_params *params)
> if (!params->sleep_time)
> params->sleep_time = 1;
>
> - if (params->cpus) {
> - retval = osnoise_set_cpus(tool->context, params->cpus);
> - if (retval) {
> - err_msg("Failed to apply CPUs config\n");
> - goto out_err;
> - }
> + retval = osnoise_set_cpus(tool->context, params->cpus ? params->cpus : "all");
> + if (retval) {
> + err_msg("Failed to apply CPUs config\n");
> + goto out_err;
> }
>
> if (params->runtime || params->period) {
> retval = osnoise_set_runtime_period(tool->context,
> params->runtime,
> params->period);
> - if (retval) {
> - err_msg("Failed to set runtime and/or period\n");
> - goto out_err;
> - }
> + } else {
> + retval = osnoise_set_runtime_period(tool->context,
> + DEFAULT_SAMPLE_PERIOD,
> + DEFAULT_SAMPLE_RUNTIME);
> }
>
> - if (params->stop_us) {
> - retval = osnoise_set_stop_us(tool->context, params->stop_us);
> - if (retval) {
> - err_msg("Failed to set stop us\n");
> - goto out_err;
> - }
> + if (retval) {
> + err_msg("Failed to set runtime and/or period\n");
> + goto out_err;
> }
>
> - if (params->stop_total_us) {
> - retval = osnoise_set_stop_total_us(tool->context, params->stop_total_us);
> - if (retval) {
> - err_msg("Failed to set stop total us\n");
> - goto out_err;
> - }
> + retval = osnoise_set_stop_us(tool->context, params->stop_us);
> + if (retval) {
> + err_msg("Failed to set stop us\n");
> + goto out_err;
> }
>
> - if (params->threshold) {
> - retval = osnoise_set_tracing_thresh(tool->context, params->threshold);
> - if (retval) {
> - err_msg("Failed to set tracing_thresh\n");
> - goto out_err;
> - }
> + retval = osnoise_set_stop_total_us(tool->context, params->stop_total_us);
> + if (retval) {
> + err_msg("Failed to set stop total us\n");
> + goto out_err;
> + }
> +
> + retval = osnoise_set_tracing_thresh(tool->context, params->threshold);
> + if (retval) {
> + err_msg("Failed to set tracing_thresh\n");
> + goto out_err;
> }
>
> if (params->hk_cpus) {
> diff --git a/tools/tracing/rtla/src/timerlat.c b/tools/tracing/rtla/src/timerlat.c
> index 448fb1f7d3a6..c29e2ba2d7d8 100644
> --- a/tools/tracing/rtla/src/timerlat.c
> +++ b/tools/tracing/rtla/src/timerlat.c
> @@ -16,6 +16,8 @@
>
> #include "timerlat.h"
>
> +#define DEFAULT_TIMERLAT_PERIOD 1000 /* 1ms */
> +
> /*
> * timerlat_apply_config - apply common configs to the initialized tool
> */
> @@ -27,49 +29,44 @@ timerlat_apply_config(struct osnoise_tool *tool, struct timerlat_params *params)
> if (!params->sleep_time)
> params->sleep_time = 1;
>
> - if (params->cpus) {
> - retval = osnoise_set_cpus(tool->context, params->cpus);
> - if (retval) {
> - err_msg("Failed to apply CPUs config\n");
> - goto out_err;
> - }
> - } else {
> + retval = osnoise_set_cpus(tool->context, params->cpus ? params->cpus : "all");
> + if (retval) {
> + err_msg("Failed to apply CPUs config\n");
> + goto out_err;
> + }
> +
> + if (!params->cpus) {
> for (i = 0; i < sysconf(_SC_NPROCESSORS_CONF); i++)
> CPU_SET(i, ¶ms->monitored_cpus);
> }
>
> - if (params->stop_us) {
> - retval = osnoise_set_stop_us(tool->context, params->stop_us);
> - if (retval) {
> - err_msg("Failed to set stop us\n");
> - goto out_err;
> - }
> + retval = osnoise_set_stop_us(tool->context, params->stop_us);
> + if (retval) {
> + err_msg("Failed to set stop us\n");
> + goto out_err;
> }
>
> - if (params->stop_total_us) {
> - retval = osnoise_set_stop_total_us(tool->context, params->stop_total_us);
> - if (retval) {
> - err_msg("Failed to set stop total us\n");
> - goto out_err;
> - }
> + retval = osnoise_set_stop_total_us(tool->context, params->stop_total_us);
> + if (retval) {
> + err_msg("Failed to set stop total us\n");
> + goto out_err;
> }
>
>
> - if (params->timerlat_period_us) {
> - retval = osnoise_set_timerlat_period_us(tool->context, params->timerlat_period_us);
> - if (retval) {
> - err_msg("Failed to set timerlat period\n");
> - goto out_err;
> - }
> + retval = osnoise_set_timerlat_period_us(tool->context,
> + params->timerlat_period_us ?
> + params->timerlat_period_us :
> + DEFAULT_TIMERLAT_PERIOD);
> + if (retval) {
> + err_msg("Failed to set timerlat period\n");
> + goto out_err;
> }
>
>
> - if (params->print_stack) {
> - retval = osnoise_set_print_stack(tool->context, params->print_stack);
> - if (retval) {
> - err_msg("Failed to set print stack\n");
> - goto out_err;
> - }
> + retval = osnoise_set_print_stack(tool->context, params->print_stack);
> + if (retval) {
> + err_msg("Failed to set print stack\n");
> + goto out_err;
> }
>
> if (params->hk_cpus) {
> --
> 2.48.1
>
>
>
Reviewed-by: John Kacur <jkacur@redhat.com>
next prev parent reply other threads:[~2025-03-20 20:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-20 9:24 [PATCH 0/6] " Tomas Glozar
2025-03-20 9:24 ` [PATCH 1/6] rtla/osnoise: Unify params struct Tomas Glozar
2025-03-20 19:00 ` John Kacur
2025-03-24 16:06 ` Steven Rostedt
2025-03-20 9:24 ` [PATCH 2/6] rtla: Unify apply_config between top and hist Tomas Glozar
2025-03-20 19:27 ` John Kacur
2025-03-20 9:24 ` [PATCH 3/6] rtla/osnoise: Set OSNOISE_WORKLOAD to true Tomas Glozar
2025-03-20 19:40 ` John Kacur
2025-03-20 9:24 ` [PATCH 4/6] rtla: Always set all tracer options Tomas Glozar
2025-03-20 20:31 ` John Kacur [this message]
2025-03-20 9:24 ` [PATCH 5/6] rtla/tests: Reset osnoise options before check Tomas Glozar
2025-03-20 20:36 ` John Kacur
2025-03-20 9:25 ` [PATCH 6/6] rtla/tests: Test setting default options Tomas Glozar
2025-03-20 20:42 ` John Kacur
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e41f0ee-7534-eedb-0a67-184ac195baf4@redhat.com \
--to=jkacur@redhat.com \
--cc=lgoncalv@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglozar@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®