[V3,2/3] tracing/osnoise: Add preempt/irq disable options

  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 | 40 +++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)
  

On Fri, 25 Nov 2022 22:20:23 +0100
Daniel Bristot de Oliveira <bristot@kernel.org> wrote:

> @@ -1308,6 +1315,8 @@ static void notify_new_max_latency(u64 latency)
>   */
>  static int run_osnoise(void)
>  {
> +	bool preempt_disable = test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
> +	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);

	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
	bool preempt_disable = IS_ENABLED(CONFIG_PREEMPT) &&
			!irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);

>  	struct osnoise_variables *osn_var = this_cpu_osn_var();
>  	u64 start, sample, last_sample;
>  	u64 last_int_count, int_count;
> @@ -1335,6 +1344,14 @@ static int run_osnoise(void)
>  	 */
>  	threshold = tracing_thresh ? : 5000;
>  
> +	/*
> +	 * IRQ disable also implies in preempt disable.
> +	 */
> +	if (irq_disable)
> +		local_irq_disable();

	if (preempt_disable)
> +		preempt_disable();
> +
>  	/*
>  	 * Make sure NMIs see sampling first
>  	 */
> @@ -1422,16 +1439,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 +1472,14 @@ static int run_osnoise(void)
>  	 */
>  	barrier();
>  
> +	/*
> +	 * Return to the preemptive state.
> +	 */

	if (preempt_disable)
> +		preempt_enable();
> +

> +	if (irq_disable)
> +		local_irq_enable();

-- Steve

>  	/*
>  	 * Save noise info.
>  	 */
  

On 11/28/22 21:39, Steven Rostedt wrote:
> On Fri, 25 Nov 2022 22:20:23 +0100
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> 
>> @@ -1308,6 +1315,8 @@ static void notify_new_max_latency(u64 latency)
>>   */
>>  static int run_osnoise(void)
>>  {
>> +	bool preempt_disable = test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
>> +	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> 	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> 	bool preempt_disable = IS_ENABLED(CONFIG_PREEMPT) &&
> 			!irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
> 


Ooops, you are right. I will fix this, and the doc as well, in the v4.

Thanks
-- Daniel
  

On 11/28/22 21:39, Steven Rostedt wrote:
> On Fri, 25 Nov 2022 22:20:23 +0100
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> 
>> @@ -1308,6 +1315,8 @@ static void notify_new_max_latency(u64 latency)
>>   */
>>  static int run_osnoise(void)
>>  {
>> +	bool preempt_disable = test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
>> +	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> 
> 	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> 	bool preempt_disable = IS_ENABLED(CONFIG_PREEMPT) &&
> 			!irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);

Ooops again, that is not exactly what I wanted, because...

>>  	struct osnoise_variables *osn_var = this_cpu_osn_var();
>>  	u64 start, sample, last_sample;
>>  	u64 last_int_count, int_count;
>> @@ -1335,6 +1344,14 @@ static int run_osnoise(void)
>>  	 */
>>  	threshold = tracing_thresh ? : 5000;
>>  
>> +	/*
>> +	 * IRQ disable also implies in preempt disable.
>> +	 */
>> +	if (irq_disable)
>> +		local_irq_disable();
> 
> 	if (preempt_disable)
>> +		preempt_disable();
>> +
>>  	/*
>>  	 * Make sure NMIs see sampling first
>>  	 */
>> @@ -1422,16 +1439,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.
>>  		 */

Then I end up cond_resched'ing here in the non-preemptive kernel.

>> -		cond_resched();
>> +		if (!irq_disable && !preempt_disable)
>> +			cond_resched();

But I also want to avoid this cond_resched if preempt_disable is set.

So, I will merge both things:

	- change the preempt_disable assignment to check !irq_disabled, like:

		/*
		 * 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);

	- change the preempt disabled if to
		if (IS_ENABLED(CONFIG_PREEMPT) && preempt_disabled)
			preempt_disable();

I tested with both preemption models (preemptive and not preemptive) and it works fine.

am I missing something?

-- Daniel

>>  		last_sample = sample;
>>  		last_int_count = int_count;
>> @@ -1450,6 +1472,14 @@ static int run_osnoise(void)
>>  	 */
>>  	barrier();
>>  
>> +	/*
>> +	 * Return to the preemptive state.
>> +	 */
> 
> 	if (preempt_disable)
>> +		preempt_enable();
>> +
> 
>> +	if (irq_disable)
>> +		local_irq_enable();
> 
> -- Steve
> 
>>  	/*
>>  	 * Save noise info.
>>  	 */
  

On Wed, 30 Nov 2022 16:47:29 +0100
Daniel Bristot de Oliveira <bristot@kernel.org> wrote:

> On 11/28/22 21:39, Steven Rostedt wrote:
> > On Fri, 25 Nov 2022 22:20:23 +0100
> > Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> >   
> >> @@ -1308,6 +1315,8 @@ static void notify_new_max_latency(u64 latency)
> >>   */
> >>  static int run_osnoise(void)
> >>  {
> >> +	bool preempt_disable = test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);
> >> +	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);  
> > 
> > 	bool irq_disable = test_bit(OSN_IRQ_DISABLE, &osnoise_options);
> > 	bool preempt_disable = IS_ENABLED(CONFIG_PREEMPT) &&
> > 			!irq_disable && test_bit(OSN_PREEMPT_DISABLE, &osnoise_options);  
> 
> Ooops again, that is not exactly what I wanted, because...

Then just remove the "IS_ENABLED()" part and it should work just fine.

> 
> >>  	struct osnoise_variables *osn_var = this_cpu_osn_var();
> >>  	u64 start, sample, last_sample;
> >>  	u64 last_int_count, int_count;
> >> @@ -1335,6 +1344,14 @@ static int run_osnoise(void)
> >>  	 */
> >>  	threshold = tracing_thresh ? : 5000;
> >>  
> >> +	/*
> >> +	 * IRQ disable also implies in preempt disable.
> >> +	 */
> >> +	if (irq_disable)
> >> +		local_irq_disable();  
> > 
> > 	if (preempt_disable)  
> >> +		preempt_disable();

The above is a nop when CONFIG_PREEMPT is false.

> >> +
> >>  	/*
> >>  	 * Make sure NMIs see sampling first
> >>  	 */
> >> @@ -1422,16 +1439,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.
> >>  		 */  
> 
> Then I end up cond_resched'ing here in the non-preemptive kernel.

Sorry, I missed the point that you want to *not* cond_resched() even in a
CONFIG_PREEMPT is false situation, if preempt_disable flag is set. That's
the reason I added the IS_ENABLED(CONFIG_PREEMPT) check at the top. I
originally didn't have that, but then thought this should always happen in
that case.

> 
> >> -		cond_resched();
> >> +		if (!irq_disable && !preempt_disable)
> >> +			cond_resched();  
> 
> But I also want to avoid this cond_resched if preempt_disable is set.

Right, so just remove the IS_ENABLED() part in the beginning.

> 
> So, I will merge both things:
> 
> 	- change the preempt_disable assignment to check !irq_disabled, like:
> 
> 		/*
> 		 * 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);

Yep (that's what I original had until I changed it)

> 
> 	- change the preempt disabled if to
> 		if (IS_ENABLED(CONFIG_PREEMPT) && preempt_disabled)
> 			preempt_disable();

No need, preempt_disable() is a nop when CONFIG_PREEMPT is false.

> 
> I tested with both preemption models (preemptive and not preemptive) and it works fine.
> 
> am I missing something?

Just that you don't need to add the IS_ENABLED() part at all.

-- Steve