Re: [PATCH v3 08/13] rv: Ensure synchronous cleanup for HA monitors

[Gabriele Monaco <gmonaco@redhat.com>]

On Mon, 2026-06-01 at 09:52 +0200, Nam Cao wrote:
> > +/*
> > + * __ha_monitor_timer_callback - generic callback representation
> > + *
> > + * This callback runs in an RCU read-side critical section to
> > allow the
> > + * destruction sequence to easily synchronize_rcu() with all
> > pending timers
> > + * after asynchronously disabling them. The ha_mon_destroying
> > check ensures
> > + * any callback entering the RCU section after synchronize_rcu()
> > completes
> > + * will see the flag and bail out immediately.
> > + */
> >  static inline void __ha_monitor_timer_callback(struct ha_monitor
> > *ha_mon)
> >  {
> > -	enum states curr_state = READ_ONCE(ha_mon-
> > >da_mon.curr_state);
> >  	DECLARE_SEQ_BUF(env_string, ENV_BUFFER_SIZE);
> > -	u64 time_ns = ha_get_ns();
> > -
> > +	enum states curr_state;
> > +	u64 time_ns;
> > +
> > +	guard(rcu)();
> > +	if (unlikely(READ_ONCE(ha_mon_destroying)))
> 
> Instead of this global variable, can we instead use da_mon-
> >monitoring?
> 

We already do in da_monitor_handling_event(), this is guarding for the
very unlikely scenario of:

1. timer callback firing (just before guard(rcu))
2. sync rcu, return before waiting for that one
3. reset and free memory
4. guard(rcu) now and check the state in da_mon (freed)

I'm not too confident saying this cannot happen under PREEMPT_RT.

I know the easy alternative would be to synchronously stop timers on
destruction, but that gets complicated with per-task monitors, where we
iterate over task grabbing a read lock.

Am I just too paranoid?

Thanks,
Gabriele

> > +		return;
> > +	/* Ensure consistent curr_state if we race with
> > da_monitor_reset */
> > +	curr_state = smp_load_acquire(&ha_mon->da_mon.curr_state);
> > +	if (unlikely(!da_monitor_handling_event(&ha_mon->da_mon)))
> > +		return;
> > +
> > +	time_ns = ha_get_ns();
> >  	ha_get_env_string(&env_string, ha_mon, time_ns);
> >  	ha_react(curr_state, EVENT_NONE, env_string.buffer);
> >  	ha_trace_error_env(ha_mon,
> > model_get_state_name(curr_state),
> > -- 
> > 2.54.0