On Wed, 2026-07-08 at 23:38 +0800, [email protected] wrote: > From: Wen Yang <[email protected]> > > Per-object DA storage allocation is currently limited to kmalloc on > demand. Add a compile-time selector so monitors can choose among three > strategies: > > DA_ALLOC_AUTO (default) - kmalloc per object on the monitor path > DA_ALLOC_POOL - pre-allocated fixed-size llist pool; > selected by defining DA_MON_POOL_SIZE > DA_ALLOC_MANUAL - caller pre-inserts storage; framework > only links the target field > > The pool strategy uses a lock-free llist (cmpxchg, no spinlock) so > pool release is safe from RCU callback context without acquiring a > lock. Moving allocation before the measurement window also prevents > kmalloc latency.
Measurement window here is tlob's, remember RV isn't itself a measurement tool (yet, perhaps). And isn't this also happening with other methods? We're trying to do allocation when the monitor starts (so before this measurement window). I'm a bit puzzled since you're mentioning it many times, when have we done /allocations/ from RCU callbacks? We surely do deallocations (kfree_rcu) but allocations are at most in RCU read-side critical sections and it's perfectly fine to take sleeping spinlocks there (that's a special kind of sleep under PREEMPT_RT). Besides I'm not quite sure spinlocks are that bad in RCU callbacks either (kfree surely takes them). I'm not sure what you mean here but I don't think deallocation was ever a problem, was it? > nomiss is updated to DA_ALLOC_MANUAL. > > Suggested-by: Gabriele Monaco <[email protected]> > Signed-off-by: Wen Yang <[email protected]> > --- > include/rv/da_monitor.h | 247 +++++++++++++++++++---- > include/rv/ha_monitor.h | 6 + > kernel/trace/rv/monitors/nomiss/nomiss.c | 6 +- > 3 files changed, 221 insertions(+), 38 deletions(-) > > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 34b8fba9ecd4..9c9acc123e3b 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h > @@ -14,7 +14,56 @@ > #ifndef _RV_DA_MONITOR_H > #define _RV_DA_MONITOR_H > > +/* > + * Allocation strategies for RV_MON_PER_OBJ monitors. > + * > + * Select the strategy with a single define before including this header: > + * > + * #define DA_MON_POOL_SIZE N - pool mode; N pre-allocated slots. > + * Implies DA_ALLOC_POOL > automatically. > + * #define DA_MON_ALLOCATION_STRATEGY \ > + * DA_ALLOC_MANUAL - manual mode (see below). > + * (neither) - auto mode (default). > + * > + * Do not define both DA_MON_POOL_SIZE and DA_MON_ALLOCATION_STRATEGY. > + * > + * DA_ALLOC_AUTO - lock-free kmalloc on the hot path; unbounded capacity. > + * DA_ALLOC_POOL - pre-allocated fixed-size pool; set by defining > DA_MON_POOL_SIZE. > + * DA_ALLOC_MANUAL - caller inserts storage before da_handle_start_event(); > + * the framework only links the target field. > + */ > +#define DA_ALLOC_AUTO 0 > +#define DA_ALLOC_POOL 1 > +#define DA_ALLOC_MANUAL 2 > + > +#ifdef DA_MON_POOL_SIZE > +#ifdef DA_MON_ALLOCATION_STRATEGY > +#error "Define only one of DA_MON_POOL_SIZE or DA_MON_ALLOCATION_STRATEGY" > +#endif > +#if DA_MON_POOL_SIZE == 0 > +#error "DA_MON_POOL_SIZE must be non-zero" > +#endif > +#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_POOL > +#endif Longer ifdefs should have comments to make them readable, like #endif /* DA_MON_POOL_SIZE */ > + > +#ifndef DA_MON_ALLOCATION_STRATEGY > +#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_AUTO > +#endif > + > +/* > + * Provide a zero default so da_monitor_init() can reference > + * DA_MON_POOL_SIZE in a plain C if() without an #if guard; the > + * compiler eliminates the dead branch. > + */ > +#ifndef DA_MON_POOL_SIZE > +#if DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL > +#error "DA_ALLOC_POOL requires DA_MON_POOL_SIZE to be defined and non-zero" > +#endif > +#define DA_MON_POOL_SIZE 0 > +#endif Same here, better to have a comment. > + > #include <rv/automata.h> > +#include <linux/llist.h> > #include <linux/rv.h> > #include <linux/stringify.h> > #include <linux/bug.h> > @@ -66,6 +115,16 @@ static struct rv_monitor rv_this; > #define da_monitor_sync_hook() > #endif > > +/* > + * Per-object teardown hook, called after da_monitor_reset_all() + > + * da_monitor_sync_hook() and before hash_del_rcu() for each entry. > + * All HA timer callbacks have completed at this point. > + * Define before including this header. Default: no-op. > + */ > +#ifndef da_extra_cleanup > +#define da_extra_cleanup(da_mon) > +#endif > + > /* > * Type for the target id, default to int but can be overridden. > * A long type can work as hash table key (PER_OBJ) but will be downgraded to > @@ -404,6 +463,12 @@ struct da_monitor_storage { > union rv_task_monitor rv; > struct hlist_node node; > struct rcu_head rcu; > + /* > + * Mutually exclusive with rcu: rcu is live during the RCU callback > + * flight; free_node when the slot is in da_pool_free_list. > + * Present in all monitors to avoid #if-gating the pool helpers. > + */ I really don't understand much more about it by this comment, perhaps drop it here and make the separate usages clearer later? By the way, if they are /really/ mutually exclusive and you want to save space, why not having them in an anonymous union? > + struct llist_node free_node; > }; > > #ifndef DA_MONITOR_HT_BITS > @@ -495,18 +560,6 @@ static inline da_id_type da_get_id(struct da_monitor > *da_mon) > return container_of(da_mon, struct da_monitor_storage, rv.da_mon)- > >id; > } > > -/* > - * da_create_or_get - create the per-object storage if not already there > - * > - * This needs a lookup so should be guarded by RCU, the condition is checked > - * directly in da_create_storage() > - */ > -static inline void da_create_or_get(da_id_type id, monitor_target target) > -{ > - guard(rcu)(); > - da_create_storage(id, target, da_get_monitor(id, target)); > -} > - > /* > * da_fill_empty_storage - store the target in a pre-allocated storage > * > @@ -537,15 +590,79 @@ static inline monitor_target > da_get_target_by_id(da_id_type id) > return mon_storage->target; > } > > +/* > + * Lock-free llist (cmpxchg) rather than kmem_cache/mempool: on > + * PREEMPT_RT spinlock_t becomes a sleeping lock, which is forbidden > + * in the rcuc kthread context where RCU callbacks run. This comment kind of implies we were using a kmem_cache, it's great for a changelog and helped me understand why you're doing this, but doesn't belong to the final version as is. > + * > + * Multiple producers (any context, any CPU) call llist_add; a single > + * consumer (llist_del_first, serialised by the monitor's start lock) Which monitor's start lock? There is no such a thing defined anywhere, maybe you wanted to say that monitors using this allocation scheme MUST lock during their start event. This by the way needs to be a global lock among all instances of the monitor (as you're indeed doing in tlob). With that in mind, I don't really see how this is better than the original mempool: you still need to lock. There's nothing wrong in freeing stuff from RCU callbacks, that's what they're for. > + * needs no additional synchronisation. > + * > + * Per-TU statics: each PER_OBJ monitor gets its own pool instance; > + * da_pool_storage and da_pool_free_list are NULL/empty and the pool > + * paths are dead code for non-pool monitors. > + */ > +static struct da_monitor_storage *da_pool_storage; > +static LLIST_HEAD(da_pool_free_list); ... > +++ b/kernel/trace/rv/monitors/nomiss/nomiss.c > @@ -17,8 +17,8 @@ > > #define RV_MON_TYPE RV_MON_PER_OBJ > #define HA_TIMER_TYPE HA_TIMER_WHEEL > -/* The start condition is on sched_switch, it's dangerous to allocate there > */ > -#define DA_SKIP_AUTO_ALLOC > +/* Allocate storage in sched_setscheduler; sched_switch is too hot to alloc. > */ > +#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL > typedef struct sched_dl_entity *monitor_target; > #include "nomiss.h" > #include <rv/ha_monitor.h> > @@ -214,7 +214,7 @@ static void handle_sys_enter(void *data, struct pt_regs > *regs, long id) > if (p->policy == SCHED_DEADLINE) > da_reset(EXPAND_ID_TASK(p)); > else if (new_policy == SCHED_DEADLINE) > - da_create_or_get(EXPAND_ID_TASK(p)); > + da_create_empty_storage(get_entity_id(&p->dl, task_cpu(p), > DL_TASK)); I'm starting to doubt this is the right thing to do. We do have the target (p) and that function doesn't check if the id already has a storage (which shouldn't happen but well, doesn't hurt checking). This simplification is probably just not worth it, and doesn't look related to the rest of the change. Thanks, Gabriele
