Re: [PATCH v5 1/9] rv: Introduce DA_MON_ALLOCATION_STRATEGY

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] DA_ALLOC_POOL strategy causes lockdep 'Invalid wait context' warnings and crashes on PREEMPT_RT when used in tracepoints that hold raw spinlocks.
--

--- Patch [1]: [PATCH v5 1/9] rv: Introduce DA_MON_ALLOCATION_STRATEGY ---
commit e8961cb4205551f74e01ec6b7818734bafc846f3
Author: Wen Yang <[email protected]>

rv: Introduce DA_MON_ALLOCATION_STRATEGY

This patch introduces a compile-time selection for per-object deterministic
automata (DA) monitor allocation strategies. It adds DA_ALLOC_AUTO,
DA_ALLOC_POOL, and DA_ALLOC_MANUAL to handle contexts where dynamic memory
allocation is problematic.

> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
> index e3cf85c9ce556..48c534324cbb2 100644
> --- a/include/rv/da_monitor.h
> +++ b/include/rv/da_monitor.h
[ ... ]
> @@ -543,6 +596,59 @@ static inline monitor_target da_get_target_by_id(da_id_type id)
>  	return mon_storage->target;
>  }
>  
> +/*
> + * Pre-allocated mempool for DA_ALLOC_POOL monitors: DA_MON_POOL_SIZE
> + * slots, eager-allocated at init.  mempool_alloc_preallocated() pops a
> + * slot without touching the allocator (bounded start latency; NULL when
> + * exhausted).  mempool_free() is safe from RCU-callback context.
> + * Non-pool monitors get a zero-initialised mempool_t; pool paths compile
> + * away.
> + */
> +static mempool_t da_monitor_pool;
[ ... ]
> +/*
> + * da_create_pool_storage - pop a free pool slot and insert it into the hash.
> + *
> + * Returns the new da_monitor, or NULL if the pool is exhausted.  Finding
> + * an existing entry for the same id fires WARN_ON_ONCE (double-start bug).
> + *
> + * Caller must hold an RCU read-side CS and the monitor's serialisation lock.
> + */
> +static inline struct da_monitor *
> +da_create_pool_storage(da_id_type id, monitor_target target,
> +		       struct da_monitor *da_mon)
> +{
> +	struct da_monitor_storage *mon_storage, *existing;
> +
> +	if (da_mon)
> +		return da_mon;
> +
> +	mon_storage = mempool_alloc_preallocated(&da_monitor_pool);

[Severity: High]
Will this cause an "Invalid wait context" lockdep warning or a kernel panic
on PREEMPT_RT systems when executed from tracepoints holding raw spinlocks?

The patch describes DA_ALLOC_POOL as being useful for contexts problematic
for allocation, such as scheduling. If a monitor uses this pool strategy in
a tracepoint like sched_switch, it will be called while holding a
raw_spinlock_t (like rq->lock).

Looking at mempool_alloc_preallocated(), it unconditionally acquires the
pool lock, which is a spinlock_t. On PREEMPT_RT, spinlock_t maps to a
sleepable rt_mutex. Acquiring a sleeping lock while holding a raw_spinlock_t
is a strict wait-context locking violation.

Is there a way to manage the pre-allocated pool in da_create_pool_storage()
without relying on locks that sleep on PREEMPT_RT?

> +	if (!mon_storage)
> +		return NULL;
> +	memset(mon_storage, 0, sizeof(*mon_storage));
> +
> +	mon_storage->id = id;
> +	mon_storage->target = target;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1