Re: [PATCH v5 7/9] rv: Replace tss and sncid monitors with more complete sts

[Nam Cao]

On Mon, Jul 28, 2025 at 05:53:34PM +0200, Nam Cao wrote:
> On Mon, Jul 28, 2025 at 03:50:19PM +0200, Gabriele Monaco wrote:
> > The tss monitor currently guarantees task switches can happen only while
> > scheduling, whereas the sncid monitor enforces scheduling occurs with
> > interrupt disabled.
> > 
> > Replace the monitors with a more comprehensive specification which
> > implies both but also ensures that:
> > * each scheduler call disable interrupts to switch
> > * each task switch happens with interrupts disabled
> > 
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Signed-off-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>
> 
> I gave this a try on riscv64 and observed some errors:
> 
> [  620.696055] rv: monitor sts does not allow event sched_switch on state enable_to_exit
> [  621.047705] rv: monitor sts does not allow event sched_switch on state enable_to_exit
> [  642.440209] rv: monitor sts does not allow event sched_switch on state enable_to_exit
> 
> I tested with two user programs:
> 
>     int main() { asm ("unimp"); }
>     int main() { asm ("ebreak"); }
> 
> The two programs are repeatedly executed:
> 
>     #!/bin/bash
>     ./test1 &
>     ./test2 &
>     # ... repeat lots of time

Okay, I think I know why..

It seems the monitor is in scheduling state. Then it sees a pair of
irq_disable and irq_enable, and it mistakenly thinks that this is the
is_switch==false case in __schedule. So it thinks it is at the end of
__schedule(), and does not expect a switch_switch.

However, this is wrong. The irq_disable and irq_enable pair is not from
__schedule(), it is from softirq (see below).

In short, the monitor thinks it is at the end of __schedule(), but actually
it is still at the beginning.

That's just from my limited understanding of the model, so I may be wrong.
What do you think?

Nam

             test-256     [002] dns..    63.070743: da_event_sts: scheduling x irq_disable -> disable_to_switch
             test-256     [002] dns..    63.070748: <stack trace>
  => trace_dump_stack
  => da_event_sts
  => handle_irq_disable
  => trace_hardirqs_off.part.0
  => trace_hardirqs_off
  => note_gp_changes
  => rcu_core
  => rcu_core_si
  => handle_softirqs
  => __irq_exit_rcu
  => irq_exit_rcu
  => handle_riscv_irq
  => call_on_irq_stack
             test-256     [002] dns..    63.070755: da_event_sts: disable_to_switch x irq_enable -> enable_to_exit
             test-256     [002] dns..    63.070760: <stack trace>
  => trace_dump_stack
  => da_event_sts
  => handle_irq_enable
  => trace_hardirqs_on
  => note_gp_changes
  => rcu_core
  => rcu_core_si
  => handle_softirqs
  => __irq_exit_rcu
  => irq_exit_rcu
  => handle_riscv_irq
  => call_on_irq_stack
  => call_on_irq_stack