[V4,2/3] tracing/osnoise: Add preempt and/or irq disabled options
Commit Message
The osnoise workload runs with preemption and IRQs enabled in such
a way as to allow all sorts of noise to disturb osnoise's execution.
hwlat tracer has a similar workload but works with irq disabled,
allowing only NMIs and the hardware to generate noise.
While thinking about adding an options file to hwlat tracer to
allow the system to panic, and other features I was thinking
to add, like having a tracepoint at each noise detection, it
came to my mind that is easier to make osnoise and also do
hardware latency detection than making hwlat "feature compatible"
with osnoise.
Other points are:
- osnoise already has an independent cpu file.
- osnoise has a more intuitive interface, e.g., runtime/period vs.
window/width (and people often need help remembering what it is).
- osnoise: tracepoints
- osnoise stop options
- osnoise options file itself
Moreover, the user-space side (in rtla) is simplified by reusing the
existing osnoise code.
Finally, people have been asking me about using osnoise for hw latency
detection, and I have to explain that it was sufficient but not
necessary. These options make it sufficient and necessary.
Adding a Suggested-by Clark, as he often asked me about this
possibility.
Cc: Suggested-by: Clark Williams <williams@redhat.com>
Cc: Daniel Bristot de Oliveira <bristot@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
---
kernel/trace/trace_osnoise.c | 48 ++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 5 deletions(-)
Comments
On Wed, 30 Nov 2022 19:19:10 +0100
Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
Hi Daniel,
As I was adding this series, I noticed an issue that needs to be fixed.
> static int run_osnoise(void)
> {
> + bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> struct osnoise_variables *osn_var = this_cpu_osn_var();
> u64 start, sample, last_sample;
> u64 last_int_count, int_count;
> @@ -1315,11 +1323,18 @@ static int run_osnoise(void)
> s64 total, last_total = 0;
> struct osnoise_sample s;
> unsigned int threshold;
> + bool preempt_disable;
Let's use a different name for the above variable.
> u64 runtime, stop_in;
> u64 sum_noise = 0;
> int hw_count = 0;
> int ret = -1;
>
> + /*
> + * Disabling preemption is only required if IRQs are enabled,
> + * and the options is set on.
> + */
> + preempt_disable = !irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
> +
> /*
> * Considers the current thread as the workload.
> */
> @@ -1335,6 +1350,15 @@ static int run_osnoise(void)
> */
> threshold = tracing_thresh ? : 5000;
>
> + /*
> + * Apply PREEMPT and IRQ disabled options.
> + */
> + if (irq_disable)
> + local_irq_disable();
> +
> + if (preempt_disable)
> + preempt_disable();
> +
The only reason the above works is because preempt_disable() is a macro.
If it was a function, then it would likely fail to build (as you are
overriding the name with a bool variable).
-- Steve
On 12/9/22 21:35, Steven Rostedt wrote:
>> + if (preempt_disable)
>> + preempt_disable();
>> +
> The only reason the above works is because preempt_disable() is a macro.
> If it was a function, then it would likely fail to build (as you are
> overriding the name with a bool variable).
oops.
Sending a new version changing the variable name.
-- Daniel
@@ -55,10 +55,17 @@ enum osnoise_options_index {
OSN_DEFAULTS = 0,
OSN_WORKLOAD,
OSN_PANIC_ON_STOP,
+ OSN_PREEMPT_DISABLE,
+ OSN_IRQ_DISABLE,
OSN_MAX
};
-static const char * const osnoise_options_str[OSN_MAX] = { "DEFAULTS", "OSNOISE_WORKLOAD", "PANIC_ON_STOP" };
+static const char * const osnoise_options_str[OSN_MAX] = {
+ "DEFAULTS",
+ "OSNOISE_WORKLOAD",
+ "PANIC_ON_STOP",
+ "OSNOISE_PREEMPT_DISABLE",
+ "OSNOISE_IRQ_DISABLE" };
#define OSN_DEFAULT_OPTIONS 0x2
unsigned long osnoise_options = OSN_DEFAULT_OPTIONS;
@@ -1308,6 +1315,7 @@ static void notify_new_max_latency(u64 latency)
*/
static int run_osnoise(void)
{
+ bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
struct osnoise_variables *osn_var = this_cpu_osn_var();
u64 start, sample, last_sample;
u64 last_int_count, int_count;
@@ -1315,11 +1323,18 @@ static int run_osnoise(void)
s64 total, last_total = 0;
struct osnoise_sample s;
unsigned int threshold;
+ bool preempt_disable;
u64 runtime, stop_in;
u64 sum_noise = 0;
int hw_count = 0;
int ret = -1;
+ /*
+ * Disabling preemption is only required if IRQs are enabled,
+ * and the options is set on.
+ */
+ preempt_disable = !irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
+
/*
* Considers the current thread as the workload.
*/
@@ -1335,6 +1350,15 @@ static int run_osnoise(void)
*/
threshold = tracing_thresh ? : 5000;
+ /*
+ * Apply PREEMPT and IRQ disabled options.
+ */
+ if (irq_disable)
+ local_irq_disable();
+
+ if (preempt_disable)
+ preempt_disable();
+
/*
* Make sure NMIs see sampling first
*/
@@ -1422,16 +1446,21 @@ static int run_osnoise(void)
* cond_resched()
*/
if (IS_ENABLED(CONFIG_PREEMPT_RCU)) {
- local_irq_disable();
+ if (!irq_disable)
+ local_irq_disable();
+
rcu_momentary_dyntick_idle();
- local_irq_enable();
+
+ if (!irq_disable)
+ local_irq_enable();
}
/*
* For the non-preemptive kernel config: let threads runs, if
- * they so wish.
+ * they so wish, unless set not do to so.
*/
- cond_resched();
+ if (!irq_disable && !preempt_disable)
+ cond_resched();
last_sample = sample;
last_int_count = int_count;
@@ -1450,6 +1479,15 @@ static int run_osnoise(void)
*/
barrier();
+ /*
+ * Return to the preemptive state.
+ */
+ if (preempt_disable)
+ preempt_enable();
+
+ if (irq_disable)
+ local_irq_enable();
+
/*
* Save noise info.
*/