| Message ID | 69cbbd98cce2515c84127c8827d733dc87b04823.1635181938.git.bristot@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RTLA: An interface for osnoise/timerlat tracers | expand |
On Mon, 25 Oct 2021 19:40:31 +0200 Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > kernel/trace/trace_osnoise.c | 101 +++++++++++++++++++++++++++-------- > 1 file changed, 78 insertions(+), 23 deletions(-) > > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > index 3db506f49a90..8681ffc3817b 100644 > --- a/kernel/trace/trace_osnoise.c > +++ b/kernel/trace/trace_osnoise.c > @@ -64,6 +64,24 @@ static bool osnoise_has_registered_instances(void) > list); > } > > +/* > + * osnoise_instance_registered - check if a tr is already registered > + */ > +static int osnoise_instance_registered(struct trace_array *tr) > +{ > + struct osnoise_instance *inst; > + int found = 0; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(inst, &osnoise_instances, list) { > + if (inst->tr == tr) > + found = 1; > + } > + rcu_read_unlock(); > + > + return found; > +} > + > /* > * osnoise_register_instance - register a new trace instance > * > @@ -2048,6 +2066,16 @@ static int osnoise_workload_start(void) > { > int retval; > > + /* > + * Instances need to be registered after calling workload > + * start. Hence, if there is already an instance, the > + * workload was already registered. Otherwise, this > + * code is on the way to register the first instance, > + * and the workload will start. > + */ > + if (osnoise_has_registered_instances()) > + return 0; Looking at how this is checked before being called, it really should return -1, as it is an error if this is called with instances active. -- Steve > + > osn_var_reset_all(); > > retval = osnoise_hook_events(); > @@ -2075,6 +2103,13 @@ static int osnoise_workload_start(void) > */ > static void osnoise_workload_stop(void) > { > + /* > + * Instances need to be unregistered before calling > + * stop. Hence, if there is a registered instance, more > + * than one instance is running, and the workload will not > + * yet stop. Otherwise, this code is on the way to disable > + * the last instance, and the workload can stop. > + */ > if (osnoise_has_registered_instances()) > return; > > @@ -2096,7 +2131,11 @@ static void osnoise_tracer_start(struct trace_array *tr) > { > int retval; > > - if (osnoise_has_registered_instances()) > + /* > + * If the instance is already registered, there is no need to > + * register it again. > + */ > + if (osnoise_instance_registered(tr)) > return; > > retval = osnoise_workload_start();
On 10/26/21 04:08, Steven Rostedt wrote: > On Mon, 25 Oct 2021 19:40:31 +0200 > Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > >> kernel/trace/trace_osnoise.c | 101 +++++++++++++++++++++++++++-------- >> 1 file changed, 78 insertions(+), 23 deletions(-) >> >> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c >> index 3db506f49a90..8681ffc3817b 100644 >> --- a/kernel/trace/trace_osnoise.c >> +++ b/kernel/trace/trace_osnoise.c >> @@ -64,6 +64,24 @@ static bool osnoise_has_registered_instances(void) >> list); >> } >> >> +/* >> + * osnoise_instance_registered - check if a tr is already registered >> + */ >> +static int osnoise_instance_registered(struct trace_array *tr) >> +{ >> + struct osnoise_instance *inst; >> + int found = 0; >> + >> + rcu_read_lock(); >> + list_for_each_entry_rcu(inst, &osnoise_instances, list) { >> + if (inst->tr == tr) >> + found = 1; >> + } >> + rcu_read_unlock(); >> + >> + return found; >> +} >> + >> /* >> * osnoise_register_instance - register a new trace instance >> * >> @@ -2048,6 +2066,16 @@ static int osnoise_workload_start(void) >> { >> int retval; >> >> + /* >> + * Instances need to be registered after calling workload >> + * start. Hence, if there is already an instance, the >> + * workload was already registered. Otherwise, this >> + * code is on the way to register the first instance, >> + * and the workload will start. >> + */ >> + if (osnoise_has_registered_instances()) >> + return 0; > > Looking at how this is checked before being called, it really should > return -1, as it is an error if this is called with instances active. Hum.... maybe my explanation is not good enough. It is not a problem if it is called with active instances. It would be an error if the same instance was already registered at this point, but that was checked before. Here it is checking for other instances that should have enabled the workload. Does updating the comment with the one below helps? ---- %< ---- Instances need to be registered after calling workload start. Hence, if there is already a registered instance, this instance is not the first one to enable the workload, and a previous instance has already started the workload. Otherwise, this code is on the way to register the first instance, and the workload needs to start. ---- >% ---- ? > -- Steve > >> + >> osn_var_reset_all(); >> >> retval = osnoise_hook_events(); >> @@ -2075,6 +2103,13 @@ static int osnoise_workload_start(void) >> */ >> static void osnoise_workload_stop(void) >> { >> + /* >> + * Instances need to be unregistered before calling >> + * stop. Hence, if there is a registered instance, more >> + * than one instance is running, and the workload will not >> + * yet stop. Otherwise, this code is on the way to disable >> + * the last instance, and the workload can stop. >> + */ >> if (osnoise_has_registered_instances()) >> return; >> >> @@ -2096,7 +2131,11 @@ static void osnoise_tracer_start(struct trace_array *tr) >> { >> int retval; >> >> - if (osnoise_has_registered_instances()) >> + /* >> + * If the instance is already registered, there is no need to >> + * register it again. >> + */ >> + if (osnoise_instance_registered(tr)) >> return; >> >> retval = osnoise_workload_start();
On Tue, 26 Oct 2021 10:38:27 +0200 Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > >> * osnoise_register_instance - register a new trace instance > >> * > >> @@ -2048,6 +2066,16 @@ static int osnoise_workload_start(void) > >> { > >> int retval; > >> > >> + /* > >> + * Instances need to be registered after calling workload > >> + * start. Hence, if there is already an instance, the > >> + * workload was already registered. Otherwise, this > >> + * code is on the way to register the first instance, > >> + * and the workload will start. > >> + */ > >> + if (osnoise_has_registered_instances()) > >> + return 0; > > > > Looking at how this is checked before being called, it really should > > return -1, as it is an error if this is called with instances active. > > Hum.... maybe my explanation is not good enough. It is not a problem if it is > called with active instances. It would be an error if the same instance was > already registered at this point, but that was checked before. Here it is > checking for other instances that should have enabled the workload. > > Does updating the comment with the one below helps? No need. > >> @@ -2096,7 +2131,11 @@ static void osnoise_tracer_start(struct trace_array *tr) > >> { > >> int retval; > >> > >> - if (osnoise_has_registered_instances()) > >> + /* > >> + * If the instance is already registered, there is no need to > >> + * register it again. > >> + */ > >> + if (osnoise_instance_registered(tr)) My eyes missed that you removed the osnoise_has_registered_instances() for the osnoise_instance_registered(tr), and thought you were doing the same test twice. My mistake. I need to not review patches at the end of the day when I'm ready to go to sleep. -- Steve
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c index 3db506f49a90..8681ffc3817b 100644 --- a/kernel/trace/trace_osnoise.c +++ b/kernel/trace/trace_osnoise.c @@ -64,6 +64,24 @@ static bool osnoise_has_registered_instances(void) list); } +/* + * osnoise_instance_registered - check if a tr is already registered + */ +static int osnoise_instance_registered(struct trace_array *tr) +{ + struct osnoise_instance *inst; + int found = 0; + + rcu_read_lock(); + list_for_each_entry_rcu(inst, &osnoise_instances, list) { + if (inst->tr == tr) + found = 1; + } + rcu_read_unlock(); + + return found; +} + /* * osnoise_register_instance - register a new trace instance * @@ -2048,6 +2066,16 @@ static int osnoise_workload_start(void) { int retval; + /* + * Instances need to be registered after calling workload + * start. Hence, if there is already an instance, the + * workload was already registered. Otherwise, this + * code is on the way to register the first instance, + * and the workload will start. + */ + if (osnoise_has_registered_instances()) + return 0; + osn_var_reset_all(); retval = osnoise_hook_events(); @@ -2075,6 +2103,13 @@ static int osnoise_workload_start(void) */ static void osnoise_workload_stop(void) { + /* + * Instances need to be unregistered before calling + * stop. Hence, if there is a registered instance, more + * than one instance is running, and the workload will not + * yet stop. Otherwise, this code is on the way to disable + * the last instance, and the workload can stop. + */ if (osnoise_has_registered_instances()) return; @@ -2096,7 +2131,11 @@ static void osnoise_tracer_start(struct trace_array *tr) { int retval; - if (osnoise_has_registered_instances()) + /* + * If the instance is already registered, there is no need to + * register it again. + */ + if (osnoise_instance_registered(tr)) return; retval = osnoise_workload_start(); @@ -2108,18 +2147,17 @@ static void osnoise_tracer_start(struct trace_array *tr) static void osnoise_tracer_stop(struct trace_array *tr) { - if (!osnoise_has_registered_instances()) - return; - osnoise_unregister_instance(tr); osnoise_workload_stop(); } static int osnoise_tracer_init(struct trace_array *tr) { - - /* Only allow one instance to enable this */ - if (osnoise_has_registered_instances()) + /* + * Only allow osnoise tracer if timerlat tracer is not running + * already. + */ + if (osnoise_data.timerlat_tracer) return -EBUSY; tr->max_latency = 0; @@ -2148,45 +2186,55 @@ static void timerlat_tracer_start(struct trace_array *tr) { int retval; - if (osnoise_has_registered_instances()) + /* + * If the instance is already registered, there is no need to + * register it again. + */ + if (osnoise_instance_registered(tr)) return; - osnoise_data.timerlat_tracer = 1; - retval = osnoise_workload_start(); if (retval) - goto out_err; + pr_err(BANNER "Error starting timerlat tracer\n"); osnoise_register_instance(tr); return; -out_err: - pr_err(BANNER "Error starting timerlat tracer\n"); } static void timerlat_tracer_stop(struct trace_array *tr) { int cpu; - if (!osnoise_has_registered_instances()) - return; - - for_each_online_cpu(cpu) - per_cpu(per_cpu_osnoise_var, cpu).sampling = 0; + osnoise_unregister_instance(tr); - osnoise_tracer_stop(tr); + /* + * Instruct the threads to stop only if this is the last instance. + */ + if (!osnoise_has_registered_instances()) { + for_each_online_cpu(cpu) + per_cpu(per_cpu_osnoise_var, cpu).sampling = 0; + } - osnoise_data.timerlat_tracer = 0; + osnoise_workload_stop(); } static int timerlat_tracer_init(struct trace_array *tr) { - /* Only allow one instance to enable this */ - if (osnoise_has_registered_instances()) + /* + * Only allow timerlat tracer if osnoise tracer is not running already. + */ + if (osnoise_has_registered_instances() && !osnoise_data.timerlat_tracer) return -EBUSY; - tr->max_latency = 0; + /* + * If this is the first instance, set timerlat_tracer to block + * osnoise tracer start. + */ + if (!osnoise_has_registered_instances()) + osnoise_data.timerlat_tracer = 1; + tr->max_latency = 0; timerlat_tracer_start(tr); return 0; @@ -2195,6 +2243,13 @@ static int timerlat_tracer_init(struct trace_array *tr) static void timerlat_tracer_reset(struct trace_array *tr) { timerlat_tracer_stop(tr); + + /* + * If this is the last instance, reset timerlat_tracer allowing + * osnoise to be started. + */ + if (!osnoise_has_registered_instances()) + osnoise_data.timerlat_tracer = 0; } static struct tracer timerlat_tracer __read_mostly = {
Currently, the user can start only one instance of timerlat/osnoise tracers and the tracers cannot run in parallel. As starting point to add more flexibility, let's allow the same tracer to run on different trace instances. The workload will start when the first trace_array (instance) is registered and stop when the last instance is unregistered. So, while this patch allows the same tracer to run in multiple instances (e.g., two instances running osnoise), it still does not allow instances of timerlat and osnoise in parallel (e.g., one timerlat and osnoise). That is because the osnoise: events have different behavior depending on which tracer is enabled (osnoise or timerlat). Enabling the parallel usage of these two tracers is on my TODO list. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Tom Zanussi <zanussi@kernel.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Juri Lelli <juri.lelli@redhat.com> Cc: Clark Williams <williams@redhat.com> Cc: John Kacur <jkacur@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Daniel Bristot de Oliveira <bristot@kernel.org> Cc: linux-rt-users@vger.kernel.org Cc: linux-trace-devel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org> --- kernel/trace/trace_osnoise.c | 101 +++++++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 23 deletions(-)