Re: [RFC PATCH 7/9] rv: Retry when da monitor detects race conditions

[Gabriele Monaco <gmonaco@redhat.com>]

On Fri, 2025-04-11 at 06:52 +0200, Nam Cao wrote:
> On Fri, Apr 04, 2025 at 10:45:20AM +0200, Gabriele Monaco wrote:
> > DA monitor can be accessed from multiple cores simultaneously, this
> > is
> > likely, for instance when dealing with per-task monitors reacting
> > on
> > events that do not always occur on the CPU where the task is
> > running.
> > This can cause race conditions where two events change the next
> > state
> > and we see inconsistent values. E.g.:
> > 
> >   [62] event_srs: 27: sleepable x sched_wakeup -> running (final)
> >   [63] event_srs: 27: sleepable x sched_set_state_sleepable ->
> > sleepable
> >   [63] error_srs: 27: event sched_switch_suspend not expected in
> > the state running
> > 
> > In this case the monitor fails because the event on CPU 62 wins
> > against
> > the one on CPU 63, although the correct state should have been
> > sleepable, since the task get suspended.
> > 
> > Detect if the current state was modified by using try_cmpxchg while
> > storing the next value. If it was, try again reading the current
> > state.
> > After a maximum number of failed retries, react as if it was an
> > error
> > with invalid current state (we cannot determine it).
> > 
> > Monitors where this type of condition can occur must be able to
> > account
> > for racing events in any possible order, as we cannot know the
> > winner.
> 
> Is locking not simpler? I understand raw_spin_lock() doesn't work
> because
> it steps on some tracepoints. But how about adding something like
> raw_spin_lock_notrace()?

It is probably simpler, but I think it would require also to disable
interrupts (some events occur in interrupt context), I'm not sure the
introduced overhead is going to be worth it in the fast path, but
that's kinda what I wanted to learn in this RFC ;)

> 
> static inline bool raw_spin_lock_notrace(raw_spinlock_t *lock)
> {
> 	preempt_disable_notrace(); //probably not required,
> tracepoint handlers do this already
> 
> 	if (!do_raw_spin_trylock(lock))
> 		do_raw_spin_lock(lock);
> }
> 
> My LTL series theoretically also has this problem, but I have never
> got it
> during testing yet. We should use the same solution for both DA and
> LTL.

Yes totally, on the long run we might get some common utilities for
this kind of things that aren't too monitor specific. But for now I
wouldn't worry too much.

> 
> Also, can you please Cc me in your RV patches?
> 

Right.. will do!

Thanks for your feedback,
Gabriele