Re: [PATCH v4 1/8] rv/da: introduce DA_MON_ALLOCATION_STRATEGY
From: Gabriele Monaco
Date: Fri Jul 17 2026 - 09:46:56 EST
On Wed, 2026-07-08 at 23:38 +0800, wen.yang@xxxxxxxxx wrote:
> From: Wen Yang <wen.yang@xxxxxxxxx>
>
> 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 <gmonaco@xxxxxxxxxx>
> Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
> ---
> 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