On Wed, 30 Nov 2022 16:47:29 +0100
Daniel Bristot de Oliveira <bristot@xxxxxxxxxx> wrote:
> On 11/28/22 21:39, Steven Rostedt wrote:
> > On Fri, 25 Nov 2022 22:20:23 +0100
> > Daniel Bristot de Oliveira <bristot@xxxxxxxxxx> 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
