rv/tlob: Add task latency over budget RV monitor

From: Wen Yang <[email protected]>

da_monitor_start() set monitoring=1 before calling da_monitor_init_hook(), may racing with the sched_switch handler:

  da_monitor_start()               sched_switch handler -------------------------        --------------------------------- da_mon->monitoring = 1;
                                   if (da_monitoring(da_mon))  /* true  */ ha_start_timer_ns(...); /* hrtimer->base == NULL, crash */ da_monitor_init_hook(da_mon);
  /* hrtimer_setup() sets base */

Fix the ordering and pair with release/acquire semantics:

  da_monitor_init_hook(da_mon); smp_store_release(&da_mon->monitoring, 1);    /* da_monitor_start()  */ return smp_load_acquire(&da_mon->monitoring); /* da_monitoring()     */

On ARM64 a plain STR + LDR does not form a release-acquire pair, so the load can observe monitoring=1 while hrtimer->base is still NULL. The plain accesses are also data races under KCSAN.

Use WRITE_ONCE for the monitoring=0 store in da_monitor_reset() to cover the reset path.

Fixes 792575348ff7 ("rv/include: Add deterministic automata monitor

definition via C macros")

Signed-off-by Wen Yang <[email protected]>

---
 include/rv/da_monitor.h | 8 +++++---

 1 file changed, 5 insertions(+), 3 deletions(-)

include/rv/da_monitor.h +1 -1

index 39765ff6f098..00ded3d5ab3f 100644
@@ -82,7 +82,7 @@ static void react(enum states curr_state, enum events event)
  static inline void da_monitor_reset(struct da_monitor *da_mon)
  {
   da_monitor_reset_hook(da_mon);
- da_mon->monitoring = 0;
+ WRITE_ONCE(da_mon->monitoring, 0);
   da_mon->curr_state = model_get_initial_state();
  }

include/rv/da_monitor.h +1 -0

+ smp_store_release(&da_mon->monitoring, 1);
  }

 /*

include/rv/da_monitor.h +2 -1

@@ -104,7 +105,8 @@ static inline void da_monitor_start(struct da_monitor
 *da_mon)
   */
  static inline bool da_monitoring(struct da_monitor *da_mon)
  {
- return da_mon->monitoring;
+ /* Pairs with smp_store_release in da_monitor_start(). */
+ return smp_load_acquire(&da_mon->monitoring);
  }

 /*

From: Wen Yang <[email protected]>

The following two paths race:

  CPU 0 (disable_stall/__rv_disable_monitor)  CPU 1 (wwnr probe handler)

  ------------------------------------------  ----------------------------- disable_stall()
    da_monitor_destroy()
      da_monitor_reset_all()          <------ [task T: monitoring=0] da_monitor_start(&T->rv[n]) /* no timer_setup */ monitoring=1  <---- tracepoint_synchronize_unregister()
  // CPU 1 probe has already returned; sync returns

Later, enable_stall() acquires the same slot and calls da_monitor_init():

  da_monitor_reset_all()
    da_monitor_reset(&T->rv[slot])    // monitoring=1, timer.function==0 ha_monitor_reset_env()
        ha_cancel_timer()
          timer_delete(&ha_mon->timer)  // ODEBUG: timer never initialised

  ODEBUG: assert_init not available (active state 0) object type: timer_list

  Call trace: timer_delete <- da_monitor_reset_all <- enable_stall

Call tracepoint_synchronize_unregister() inside da_monitor_destroy() before da_monitor_reset_all().  The unregister_trace_xxx() calls in the monitor's disable() have already disconnected the tracepoints; the sync here drains any handler still in flight, so no new monitoring=1 can appear after da_monitor_reset_all() clears the slot.

Also fix the slot release ordering: release the slot only after reset_all() to avoid accessing rv[] with an out-of-bounds index.

Fixes f5587d1b6ec9 ("rv: Add Hybrid Automata monitor type")

Signed-off-by Wen Yang <[email protected]>

---

 include/rv/da_monitor.h | 18 ++++++++++++++++--

 1 file changed, 16 insertions(+), 2 deletions(-)

include/rv/da_monitor.h +0 -0

index 00ded3d5ab3f..d04bb3229c75 100644
@@ -304,6 +304,20 @@ static int da_monitor_init(void)

 /*
  * da_monitor_destroy - return the allocated slot
+ *
+ * Call tracepoint_synchronize_unregister() before reset_all() to close
+ * the race where an in-flight non-HA probe handler sets monitoring=1
+ * (without calling timer_setup()) after da_monitor_reset_all() has
+ * already cleared the slot but before the caller's own sync completes.
+ * Without this barrier, an HA_TIMER_WHEEL monitor that later acquires
+ * the same slot would call timer_delete() on a never-initialised
+ * timer_list, triggering ODEBUG warnings.
+ *
+ * Note: tracepoint_synchronize_unregister() is a system-wide barrier
+ * that waits for all CPUs to finish any in-flight tracepoint handlers.
+ * The caller's own __rv_disable_monitor() issues a second sync after
+ * returning from disable(); that redundant call is harmless on the
+ * infrequent admin (enable/disable) path.
  */
 static inline void da_monitor_destroy(void)
 {

include/rv/da_monitor.h +0 -0

@@ -311,10 +325,10 @@ static inline void da_monitor_destroy(void)

  WARN_ONCE(1, "Disabling a disabled monitor: " __stringify(MONITOR_NAME));
  return;
  }

include/rv/da_monitor.h +2 -0

+	tracepoint_synchronize_unregister();
+	da_monitor_reset_all();

  rv_put_task_monitor_slot(task_mon_slot);
  task_mon_slot = RV_PER_TASK_MONITOR_INIT;

include/rv/da_monitor.h +0 -2

-
-	da_monitor_reset_all();

 }

 #elif RV_MON_TYPE == RV_MON_PER_OBJ

Quoted 5 lines · On Tue, 2026-05-12 at 02:24 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

The following two paths race:

  CPU 0 (disable_stall/__rv_disable_monitor)  CPU 1 (wwnr probe handler)

^ did you mean stall?

Quoted 28 lines

  ------------------------------------------  ----------------------------- disable_stall()
    da_monitor_destroy()
      da_monitor_reset_all()          <------ [task T: monitoring=0] da_monitor_start(&T->rv[n]) /* no timer_setup */ monitoring=1  <---- tracepoint_synchronize_unregister()
  // CPU 1 probe has already returned; sync returns

Later, enable_stall() acquires the same slot and calls da_monitor_init():

  da_monitor_reset_all()
    da_monitor_reset(&T->rv[slot])    // monitoring=1, timer.function==0 ha_monitor_reset_env()
        ha_cancel_timer()
          timer_delete(&ha_mon->timer)  // ODEBUG: timer never initialised

  ODEBUG: assert_init not available (active state 0) object type: timer_list

  Call trace: timer_delete <- da_monitor_reset_all <- enable_stall

Call tracepoint_synchronize_unregister() inside da_monitor_destroy() before da_monitor_reset_all().  The unregister_trace_xxx() calls in the monitor's disable() have already disconnected the tracepoints; the sync here drains any handler still in flight, so no new monitoring=1 can appear after da_monitor_reset_all() clears the slot.

Quoted 6 lines

Also fix the slot release ordering: release the slot only after reset_all() to avoid accessing rv[] with an out-of-bounds index.

Fixes f5587d1b6ec9 ("rv: Add Hybrid Automata monitor type")

Signed-off-by Wen Yang <[email protected]>

---

Thanks for the fix, I have a similar one waiting for submission.

These are technically 2 separate fixes though: the ordering with unset task_mon_slot (independent on HA) and the synchronisation with pending tracepoints. They probably deserve separate patches and visibility, the first has always been around and we're technically overwriting who knows what.

The explanation above is a bit hard to follow though, are you talking about a handler for the same (stall) monitor running after the reset, effectively undoing it by setting the monitoring flag?

Then this is indeed an issue with ha_monitor_reset_env() which expects a clean environment.

So that's basically what you'd see now much more often because in fact we don't
reset the right slot (though, again, that's a different issue).

Calling tracepoint_synchronize_unregister() there too would surely fix, but it used to be kinda slow. But it's probably gotten faster since now tracepoints use
SRCU, so we can wait for a dedicated grace period.

I liked the idea to wait cumulatively in the end, but that's just making things
harder.. Let's do like this:

Prepare 2 separate patches as fixes, put the task slot one first (would ease backporting), mention this issue with the race condition only in the second. You can send them independently and I'll add them to the tree as urgent.

I'm soon going to send my set of fixes that will also include the task slot patch (not removing to ease my life with conflicts).

Thanks,
Gabriele

Quoted 42 lines

 include/rv/da_monitor.h | 18 ++++++++++++++++--

 1 file changed, 16 insertions(+), 2 deletions(-)

include/rv/da_monitor.h +0 -0

index 00ded3d5ab3f..d04bb3229c75 100644
@@ -304,6 +304,20 @@ static int da_monitor_init(void)

 /*
  * da_monitor_destroy - return the allocated slot
+ *
+ * Call tracepoint_synchronize_unregister() before reset_all() to close
+ * the race where an in-flight non-HA probe handler sets monitoring=1
+ * (without calling timer_setup()) after da_monitor_reset_all() has
+ * already cleared the slot but before the caller's own sync completes.
+ * Without this barrier, an HA_TIMER_WHEEL monitor that later acquires
+ * the same slot would call timer_delete() on a never-initialised
+ * timer_list, triggering ODEBUG warnings.
+ *
+ * Note: tracepoint_synchronize_unregister() is a system-wide barrier
+ * that waits for all CPUs to finish any in-flight tracepoint handlers.
+ * The caller's own __rv_disable_monitor() issues a second sync after
+ * returning from disable(); that redundant call is harmless on the
+ * infrequent admin (enable/disable) path.
  */
 static inline void da_monitor_destroy(void)
 {

include/rv/da_monitor.h +0 -0

@@ -311,10 +325,10 @@ static inline void da_monitor_destroy(void)

  WARN_ONCE(1, "Disabling a disabled monitor: " __stringify(MONITOR_NAME));
  return;
  }

include/rv/da_monitor.h +2 -0

+	tracepoint_synchronize_unregister();
+	da_monitor_reset_all();

  rv_put_task_monitor_slot(task_mon_slot);
  task_mon_slot = RV_PER_TASK_MONITOR_INIT;

include/rv/da_monitor.h +0 -2

-
-	da_monitor_reset_all();

 }

 #elif RV_MON_TYPE == RV_MON_PER_OBJ

Quoted 7 lines · Gabriele Monaco

Quoted 5 lines · On Tue, 2026-05-12 at 02:24 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

The following two paths race:

  CPU 0 (disable_stall/__rv_disable_monitor)  CPU 1 (wwnr probe handler)

^ did you mean stall?

Ok I got it now, so essentially you'd reproduce it like:

* start a DA per-task monitor (no timer)
* stop it, a handler is still running after reset, it sets monitoring back to 1
* start an HA per-task monitor

that would use the same slot that is now looking like:

{ monitoring = 1, timer.function = NULL }

because it was not initialised as HA but monitoring was reset in the race.

Thinking about this again, it isn't just an issue with per-task monitors, all monitors reusing slots would suffer from it.
Besides, relying on monitoring can be fragile when using LTL monitors on the same task (those don't even have monitoring).

Perhaps the solution isn't that trivial, I'm going to give one more thought on it, but thanks again for bringing this up!

Gabriele

Quoted 72 lines

Quoted 28 lines

  ------------------------------------------  ----------------------------- disable_stall()
    da_monitor_destroy()
      da_monitor_reset_all()          <------ [task T: monitoring=0] da_monitor_start(&T->rv[n]) /* no timer_setup */ monitoring=1  <---- tracepoint_synchronize_unregister()
  // CPU 1 probe has already returned; sync returns

Later, enable_stall() acquires the same slot and calls da_monitor_init():

  da_monitor_reset_all()
    da_monitor_reset(&T->rv[slot])    // monitoring=1, timer.function==0 ha_monitor_reset_env()
        ha_cancel_timer()
          timer_delete(&ha_mon->timer)  // ODEBUG: timer never initialised

  ODEBUG: assert_init not available (active state 0) object type: timer_list

   Call trace: timer_delete <- da_monitor_reset_all <- enable_stall

Call tracepoint_synchronize_unregister() inside da_monitor_destroy() before da_monitor_reset_all().  The unregister_trace_xxx() calls in the monitor's disable() have already disconnected the tracepoints; the sync here drains any handler still in flight, so no new monitoring=1 can appear after da_monitor_reset_all() clears the slot.

Quoted 6 lines

Also fix the slot release ordering: release the slot only after reset_all() to avoid accessing rv[] with an out-of-bounds index.

Fixes f5587d1b6ec9 ("rv: Add Hybrid Automata monitor type")

Signed-off-by Wen Yang <[email protected]>

---

Thanks for the fix, I have a similar one waiting for submission.

These are technically 2 separate fixes though: the ordering with unset task_mon_slot (independent on HA) and the synchronisation with pending tracepoints. They probably deserve separate patches and visibility, the first has always been around and we're technically overwriting who knows what.

The explanation above is a bit hard to follow though, are you talking about a handler for the same (stall) monitor running after the reset, effectively undoing it by setting the monitoring flag?

Then this is indeed an issue with ha_monitor_reset_env() which expects a clean environment.

So that's basically what you'd see now much more often because in fact we don't
reset the right slot (though, again, that's a different issue).

Calling tracepoint_synchronize_unregister() there too would surely fix, but it used to be kinda slow. But it's probably gotten faster since now tracepoints use
SRCU, so we can wait for a dedicated grace period.

I liked the idea to wait cumulatively in the end, but that's just making things
harder.. Let's do like this:

Prepare 2 separate patches as fixes, put the task slot one first (would ease backporting), mention this issue with the race condition only in the second. You can send them independently and I'll add them to the tree as urgent.

I'm soon going to send my set of fixes that will also include the task slot patch (not removing to ease my life with conflicts).

Quoted 43 lines

Quoted 43 lines

  include/rv/da_monitor.h | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)

include/rv/da_monitor.h +16 -2

index 00ded3d5ab3f..d04bb3229c75 100644
@@ -304,6 +304,20 @@ static int da_monitor_init(void)
 
  /*
   * da_monitor_destroy - return the allocated slot
+ *
+ * Call tracepoint_synchronize_unregister() before reset_all() to close
+ * the race where an in-flight non-HA probe handler sets monitoring=1
+ * (without calling timer_setup()) after da_monitor_reset_all() has
+ * already cleared the slot but before the caller's own sync completes.
+ * Without this barrier, an HA_TIMER_WHEEL monitor that later acquires
+ * the same slot would call timer_delete() on a never-initialised
+ * timer_list, triggering ODEBUG warnings.
+ *
+ * Note: tracepoint_synchronize_unregister() is a system-wide barrier
+ * that waits for all CPUs to finish any in-flight tracepoint handlers.
+ * The caller's own __rv_disable_monitor() issues a second sync after
+ * returning from disable(); that redundant call is harmless on the
+ * infrequent admin (enable/disable) path.
   */
  static inline void da_monitor_destroy(void)
  {

@@ -311,10 +325,10 @@ static inline void da_monitor_destroy(void)
  		WARN_ONCE(1, "Disabling a disabled monitor: "
 __stringify(MONITOR_NAME));
  		return;
  	}
+	tracepoint_synchronize_unregister();
+	da_monitor_reset_all();
  	rv_put_task_monitor_slot(task_mon_slot);
  	task_mon_slot = RV_PER_TASK_MONITOR_INIT;
-
-	da_monitor_reset_all();
  }
 

 #elif RV_MON_TYPE == RV_MON_PER_OBJ

Thanks for both messages.  Two patches are ready; let me address your follow-up concerns before sending.

   1. "all monitors reusing slots would suffer from it"

      Only RV_MON_PER_TASK uses the rv_get/put_task_monitor_slot() pool.  RV_MON_GLOBAL and RV_MON_PER_CPU each have dedicated storage (a single static variable and a per-cpu variable) and never share slots across monitor types.  The race is exclusive to PER_TASK, so fixing that variant's da_monitor_destroy() is the correct scope.

   2. "LTL monitors don't even have monitoring"

      tracepoint_synchronize_unregister() does not rely on the monitoring flag at all.  It is a system-wide barrier — it calls synchronize_rcu_tasks_trace() followed by synchronize_srcu(&tracepoint_srcu) — draining every in-flight tracepoint handler on every CPU regardless of which monitor dispatched it.  LTL handlers are covered without any special treatment.

The slot-ordering issue (patch 1) affects all per-task DA monitors, not only HA ones — "independent on HA" — because RV_PER_TASK_MONITOR_INIT equals CONFIG_RV_PER_TASK_MONITORS (one past the end of rv[]), so da_monitor_reset_all() overwrites whatever follows rv[] in task_struct whenever any per-task monitor is disabled.

Also corrected "wwnr probe handler" to "stall probe handler" in patch 2 per your annotation.

Please let me know if the above reasoning addresses your concerns.

--
Best wishes,
Wen

Quoted 44 lines

Quoted 43 lines

Quoted 43 lines

  include/rv/da_monitor.h | 18 ++++++++++++++++--

  1 file changed, 16 insertions(+), 2 deletions(-)

include/rv/da_monitor.h +0 -0

index 00ded3d5ab3f..d04bb3229c75 100644
@@ -304,6 +304,20 @@ static int da_monitor_init(void)

  /*
   * da_monitor_destroy - return the allocated slot
+ *
+ * Call tracepoint_synchronize_unregister() before reset_all() to close
+ * the race where an in-flight non-HA probe handler sets monitoring=1
+ * (without calling timer_setup()) after da_monitor_reset_all() has
+ * already cleared the slot but before the caller's own sync completes.
+ * Without this barrier, an HA_TIMER_WHEEL monitor that later acquires
+ * the same slot would call timer_delete() on a never-initialised
+ * timer_list, triggering ODEBUG warnings.
+ *
+ * Note: tracepoint_synchronize_unregister() is a system-wide barrier
+ * that waits for all CPUs to finish any in-flight tracepoint handlers.
+ * The caller's own __rv_disable_monitor() issues a second sync after
+ * returning from disable(); that redundant call is harmless on the
+ * infrequent admin (enable/disable) path.
   */
  static inline void da_monitor_destroy(void)
  {

include/rv/da_monitor.h +0 -0

@@ -311,10 +325,10 @@ static inline void da_monitor_destroy(void)

   WARN_ONCE(1, "Disabling a disabled monitor: " __stringify(MONITOR_NAME));
   return;
   }

include/rv/da_monitor.h +2 -0

+	tracepoint_synchronize_unregister();
+	da_monitor_reset_all();

   rv_put_task_monitor_slot(task_mon_slot);
   task_mon_slot = RV_PER_TASK_MONITOR_INIT;

include/rv/da_monitor.h +0 -2

-
-	da_monitor_reset_all();

  }

  #elif RV_MON_TYPE == RV_MON_PER_OBJ

From: Wen Yang <[email protected]>

verificationtest-ktap used a CWD-relative path (../ftrace/ftracetest) and a relative argument (../verification) for --rv.  This works when the shell changes into the verification directory first, but breaks when the script is invoked directly - e.g. by the kselftest runner or vng - because the working directory is the kernel source root, not the script's own directory.

Fix this by computing the script's directory from $0 with cd/dirname/pwd and using absolute paths for both the ftracetest invocation and the --rv argument.  Also export the directory to PATH so that check_requires in the ftracetest framework can locate helper binaries.

Signed-off-by Wen Yang <[email protected]>

---
 tools/testing/selftests/verification/verificationtest-ktap | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)

tools/testing/selftests/verification/verificationtest-ktap +0 -0

index 18f7fe324e2f..456b8578a307 100755
@@ -5,4 +5,6 @@
  #
  # Copyright (C) Arm Ltd., 2023

-../ftrace/ftracetest -K -v --rv ../verification

+"$dir/../ftrace/ftracetest" -K -v --rv "$dir"

From: Wen Yang <[email protected]>

da_create_empty_storage() uses kmalloc_nolock(), which requires CONFIG_HAVE_ALIGNED_STRUCT_PAGE; on UML and some PREEMPT_RT configurations it always returns NULL.  Calling kmalloc from scheduler tracepoint handlers also adds unwanted latency and can fail under memory pressure.

Add da_monitor_init_prealloc(N) as an opt-in alternative to da_monitor_init().  It allocates N da_monitor_storage slots with GFP_KERNEL up-front and manages them on a LIFO free-stack protected by a spinlock, so da_create_or_get() never calls kmalloc on the hot path.

Monitors that do not call da_monitor_init_prealloc() are unaffected.

Signed-off-by Wen Yang <[email protected]>

---
 include/rv/da_monitor.h | 208 +++++++++++++++++++++++++++++++++++-----

 1 file changed, 186 insertions(+), 22 deletions(-)

include/rv/da_monitor.h +0 -0

index d04bb3229c75..7d6f62766251 100644
@@ -433,18 +433,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;
  }

include/rv/da_monitor.h +0 -12

-/*
- * 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 *

include/rv/da_monitor.h +0 -0

@@ -475,15 +463,121 @@ static inline monitor_target
 da_get_target_by_id(da_id_type id)
   return mon_storage->target;
  }

include/rv/da_monitor.h +64 -0

+/*
+ * Per-object pool state.
+ *
+ * Zero-initialised by default (storage == NULL ⟹ kmalloc mode).  A monitor
+ * opts into pool mode by calling da_monitor_init_prealloc(N) instead of
+ * da_monitor_init(), which sets storage to a non-NULL kcalloc'd array.
+ *
+ * Because every field is wrapped in this struct and the struct itself is a
+ * per-TU static, each monitor that includes this header gets a completely
+ * independent pool.  A kmalloc monitor (e.g. nomiss) and a pool monitor
+ * (e.g. tlob) therefore coexist without any interference.
+ *
+ * da_pool_return_cb runs from softirq on non-PREEMPT_RT, so irqsave is
+ * required to prevent deadlock with task-context callers.  On PREEMPT_RT
+ * it runs from an rcuc kthread where spinlock_t is a sleeping lock.
+ */
+struct da_per_obj_pool {
+ struct da_monitor_storage  *storage;  /* non-NULL ⟹ pool mode */
+ struct da_monitor_storage **free;     /* kmalloc'd pointer stack */
+ unsigned int                free_top;
+ spinlock_t                  lock;
+};
+
+static struct da_per_obj_pool da_pool = {
+ .lock = __SPIN_LOCK_UNLOCKED(da_pool.lock),
+};
+
+static void da_pool_return_cb(struct rcu_head *head)
+{
+ struct da_monitor_storage *ms =
+ container_of(head, struct da_monitor_storage, rcu);
+ unsigned long flags;
+
+ spin_lock_irqsave(&da_pool.lock, flags);
+ da_pool.free[da_pool.free_top++] = ms;
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+}
+
+/* Pops a slot from the pre-allocated pool; returns -ENOSPC if exhausted. */
+static inline int da_create_or_get_pool(da_id_type id, monitor_target target)
+{
+ struct da_monitor_storage *mon_storage;
+ unsigned long flags;
+
+ spin_lock_irqsave(&da_pool.lock, flags);
+ if (!da_pool.free_top) {
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+ return -ENOSPC;
+ }
+ mon_storage = da_pool.free[--da_pool.free_top];
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+
+ mon_storage->id = id;
+ mon_storage->target = target;
+ guard(rcu)();
+ hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
+ return 0;
+}
+
+/*
+ * Tries da_create_storage() first (lock-free via kmalloc_nolock); falls back
+ * to kmalloc(GFP_KERNEL).  Must be called from task context.
+ */
+static inline int da_create_or_get_kmalloc(da_id_type id, monitor_target

target)

include/rv/da_monitor.h +43 -0

+{
+ struct da_monitor_storage *mon_storage;
+
+ scoped_guard(rcu) {
+ if (da_create_storage(id, target, da_get_monitor(id, target)))
+ return 0;
+ }
+
+ /*
+ * da_create_storage() failed because kmalloc_nolock() returned NULL.
+ * Allocate with GFP_KERNEL outside the RCU read section: GFP_KERNEL
+ * may sleep for memory reclaim, which is illegal while the RCU read
+ * lock is held (preemption disabled on !PREEMPT_RT).
+ */
+ mon_storage = kmalloc_obj(*mon_storage, GFP_KERNEL | __GFP_ZERO);
+ if (!mon_storage)
+ return -ENOMEM;
+ mon_storage->id = id;
+ mon_storage->target = target;
+
+ /*
+ * Re-check for a concurrent insertion before linking: another
+ * caller may have succeeded while we slept in kmalloc().
+ * Discard our allocation and let the winner's entry stand.
+ */
+ scoped_guard(rcu) {
+ if (da_get_monitor(id, target)) {
+ kfree(mon_storage);
+ return 0;
+ }
+ hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
+ }
+ return 0;
+}
+
+/* Create the per-object storage if not already there. */
+static inline int da_create_or_get(da_id_type id, monitor_target target)
+{
+ if (da_pool.storage)
+ return da_create_or_get_pool(id, target);
+ return da_create_or_get_kmalloc(id, target);
+}
+

 /*
  * da_destroy_storage - destroy the per-object storage
  *
- * The caller is responsible to synchronise writers, either with locks or
- * implicitly. For instance, if da_destroy_storage is called at sched_exit and
- * da_create_storage can never occur after that, it's safe to call this without
- * locks.
- * This function includes an RCU read-side critical section to synchronise
- * against da_monitor_destroy().
+ * Pool mode: removes from hash and returns the slot via call_rcu().
+ * Kmalloc mode: removes from hash and frees via kfree_rcu().
+ *
+ * Includes an RCU read-side critical section to synchronise against
+ * da_monitor_destroy().
  */
 static inline void da_destroy_storage(da_id_type id)
 {

include/rv/da_monitor.h +0 -0

@@ -491,15 +585,17 @@ static inline void da_destroy_storage(da_id_type id)

  guard(rcu)();
  mon_storage = __da_get_mon_storage(id);
-
  if (!mon_storage)
  return;
  da_monitor_reset_hook(&mon_storage->rv.da_mon);
  hash_del_rcu(&mon_storage->node);
- kfree_rcu(mon_storage, rcu);
+ if (da_pool.storage)
+ call_rcu(&mon_storage->rcu, da_pool_return_cb);
+ else
+ kfree_rcu(mon_storage, rcu);
 }

include/rv/da_monitor.h +1 -1

-static void da_monitor_reset_all(void)
+static __maybe_unused void da_monitor_reset_all(void)

 {
  struct da_monitor_storage *mon_storage;
  int bkt;

include/rv/da_monitor.h +0 -0

@@ -510,13 +606,65 @@ static void da_monitor_reset_all(void)
   rcu_read_unlock();
  }

+/*
+ * da_monitor_init_prealloc - initialise with a pre-allocated storage pool
+ *
+ * Allocates @prealloc_count storage slots up-front so that da_create_or_get()
+ * and da_destroy_storage() never call kmalloc/kfree.  Must be called instead
+ * of da_monitor_init() for monitors that require pool mode.
+ */

include/rv/da_monitor.h +26 -0

+static inline int da_monitor_init_prealloc(unsigned int prealloc_count)
+{
+ hash_init(da_monitor_ht);
+
+ da_pool.storage = kcalloc(prealloc_count, sizeof(*da_pool.storage),
+   GFP_KERNEL);
+ if (!da_pool.storage)
+ return -ENOMEM;
+
+ da_pool.free = kmalloc_array(prealloc_count, sizeof(*da_pool.free),
+      GFP_KERNEL);
+ if (!da_pool.free) {
+ kfree(da_pool.storage);
+ da_pool.storage = NULL;
+ return -ENOMEM;
+ }
+
+ da_pool.free_top = 0;
+ for (unsigned int i = 0; i < prealloc_count; i++)
+ da_pool.free[da_pool.free_top++] = &da_pool.storage[i];
+ return 0;
+}
+
+/*
+ * da_monitor_init - initialise in kmalloc mode (no pre-allocation)
+ */

 static inline int da_monitor_init(void)
 {
  hash_init(da_monitor_ht);
  return 0;
 }

include/rv/da_monitor.h +20 -1

-static inline void da_monitor_destroy(void)
+static inline void da_monitor_destroy_pool(void)
+{
+ WARN_ON_ONCE(!hash_empty(da_monitor_ht));
+ /*
+ * Wait for all in-flight da_pool_return_cb() callbacks to
+ * complete before freeing da_pool.free.  synchronize_rcu() is
+ * not sufficient: it only waits for callbacks registered before
+ * it was called, but call_rcu() from concurrent da_destroy_storage()
+ * calls may have been enqueued later.  rcu_barrier() drains every
+ * pending callback.
+ */
+ rcu_barrier();
+ kfree(da_pool.storage);
+ da_pool.storage = NULL;
+ kfree(da_pool.free);
+ da_pool.free = NULL;
+ da_pool.free_top = 0;
+}
+
+static inline void da_monitor_destroy_kmalloc(void)

 {
  struct da_monitor_storage *mon_storage;
  struct hlist_node *tmp;

include/rv/da_monitor.h +0 -0

@@ -534,6 +682,22 @@ static inline void da_monitor_destroy(void)
   }
  }

include/rv/da_monitor.h +16 -0

+/*
+ * da_monitor_destroy - tear down the per-object monitor
+ *
+ * Pool mode: the hash must already be empty (caller must have drained all
+ * tasks first); calls rcu_barrier() to drain all pending da_pool_return_cb()
+ * callbacks before freeing pool arrays.
+ * Kmalloc mode: drains any remaining entries after synchronize_rcu().
+ */
+static inline void da_monitor_destroy(void)
+{
+ if (da_pool.storage)
+ da_monitor_destroy_pool();
+ else
+ da_monitor_destroy_kmalloc();
+}
+

 /*
  * Allow the per-object monitors to run allocation manually, necessary if the
  * start condition is in a context problematic for allocation (e.g. scheduling).

From: Wen Yang <[email protected]>

da_create_empty_storage() uses kmalloc_nolock(), which requires CONFIG_HAVE_ALIGNED_STRUCT_PAGE; on UML and some PREEMPT_RT configurations it always returns NULL.  Calling kmalloc from scheduler tracepoint handlers also adds unwanted latency and can fail under memory pressure.

Add da_monitor_init_prealloc(N) as an opt-in alternative to da_monitor_init().  It allocates N da_monitor_storage slots with GFP_KERNEL up-front and manages them on a LIFO free-stack protected by a spinlock, so da_create_or_get() never calls kmalloc on the hot path.

Monitors that do not call da_monitor_init_prealloc() are unaffected.

Signed-off-by Wen Yang <[email protected]>

---
 include/rv/da_monitor.h | 208 +++++++++++++++++++++++++++++++++++-----

 1 file changed, 186 insertions(+), 22 deletions(-)

include/rv/da_monitor.h +0 -0

index d04bb3229c75..7d6f62766251 100644
@@ -433,18 +433,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;
  }

include/rv/da_monitor.h +0 -12

-/*
- * 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 *

include/rv/da_monitor.h +0 -0

@@ -475,15 +463,121 @@ static inline monitor_target
 da_get_target_by_id(da_id_type id)
   return mon_storage->target;
  }

include/rv/da_monitor.h +64 -0

+/*
+ * Per-object pool state.
+ *
+ * Zero-initialised by default (storage == NULL ⟹ kmalloc mode).  A monitor
+ * opts into pool mode by calling da_monitor_init_prealloc(N) instead of
+ * da_monitor_init(), which sets storage to a non-NULL kcalloc'd array.
+ *
+ * Because every field is wrapped in this struct and the struct itself is a
+ * per-TU static, each monitor that includes this header gets a completely
+ * independent pool.  A kmalloc monitor (e.g. nomiss) and a pool monitor
+ * (e.g. tlob) therefore coexist without any interference.
+ *
+ * da_pool_return_cb runs from softirq on non-PREEMPT_RT, so irqsave is
+ * required to prevent deadlock with task-context callers.  On PREEMPT_RT
+ * it runs from an rcuc kthread where spinlock_t is a sleeping lock.
+ */
+struct da_per_obj_pool {
+ struct da_monitor_storage  *storage;  /* non-NULL ⟹ pool mode */
+ struct da_monitor_storage **free;     /* kmalloc'd pointer stack */
+ unsigned int                free_top;
+ spinlock_t                  lock;
+};
+
+static struct da_per_obj_pool da_pool = {
+ .lock = __SPIN_LOCK_UNLOCKED(da_pool.lock),
+};
+
+static void da_pool_return_cb(struct rcu_head *head)
+{
+ struct da_monitor_storage *ms =
+ container_of(head, struct da_monitor_storage, rcu);
+ unsigned long flags;
+
+ spin_lock_irqsave(&da_pool.lock, flags);
+ da_pool.free[da_pool.free_top++] = ms;
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+}
+
+/* Pops a slot from the pre-allocated pool; returns -ENOSPC if exhausted. */
+static inline int da_create_or_get_pool(da_id_type id, monitor_target target)
+{
+ struct da_monitor_storage *mon_storage;
+ unsigned long flags;
+
+ spin_lock_irqsave(&da_pool.lock, flags);
+ if (!da_pool.free_top) {
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+ return -ENOSPC;
+ }
+ mon_storage = da_pool.free[--da_pool.free_top];
+ spin_unlock_irqrestore(&da_pool.lock, flags);
+
+ mon_storage->id = id;
+ mon_storage->target = target;
+ guard(rcu)();
+ hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
+ return 0;
+}
+
+/*
+ * Tries da_create_storage() first (lock-free via kmalloc_nolock); falls back
+ * to kmalloc(GFP_KERNEL).  Must be called from task context.
+ */
+static inline int da_create_or_get_kmalloc(da_id_type id, monitor_target

target)

include/rv/da_monitor.h +43 -0

+{
+ struct da_monitor_storage *mon_storage;
+
+ scoped_guard(rcu) {
+ if (da_create_storage(id, target, da_get_monitor(id, target)))
+ return 0;
+ }
+
+ /*
+ * da_create_storage() failed because kmalloc_nolock() returned NULL.
+ * Allocate with GFP_KERNEL outside the RCU read section: GFP_KERNEL
+ * may sleep for memory reclaim, which is illegal while the RCU read
+ * lock is held (preemption disabled on !PREEMPT_RT).
+ */
+ mon_storage = kmalloc_obj(*mon_storage, GFP_KERNEL | __GFP_ZERO);
+ if (!mon_storage)
+ return -ENOMEM;
+ mon_storage->id = id;
+ mon_storage->target = target;
+
+ /*
+ * Re-check for a concurrent insertion before linking: another
+ * caller may have succeeded while we slept in kmalloc().
+ * Discard our allocation and let the winner's entry stand.
+ */
+ scoped_guard(rcu) {
+ if (da_get_monitor(id, target)) {
+ kfree(mon_storage);
+ return 0;
+ }
+ hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
+ }
+ return 0;
+}
+
+/* Create the per-object storage if not already there. */
+static inline int da_create_or_get(da_id_type id, monitor_target target)
+{
+ if (da_pool.storage)
+ return da_create_or_get_pool(id, target);
+ return da_create_or_get_kmalloc(id, target);
+}
+

 /*
  * da_destroy_storage - destroy the per-object storage
  *
- * The caller is responsible to synchronise writers, either with locks or
- * implicitly. For instance, if da_destroy_storage is called at sched_exit and
- * da_create_storage can never occur after that, it's safe to call this without
- * locks.
- * This function includes an RCU read-side critical section to synchronise
- * against da_monitor_destroy().
+ * Pool mode: removes from hash and returns the slot via call_rcu().
+ * Kmalloc mode: removes from hash and frees via kfree_rcu().
+ *
+ * Includes an RCU read-side critical section to synchronise against
+ * da_monitor_destroy().
  */
 static inline void da_destroy_storage(da_id_type id)
 {

include/rv/da_monitor.h +0 -0

@@ -491,15 +585,17 @@ static inline void da_destroy_storage(da_id_type id)

  guard(rcu)();
  mon_storage = __da_get_mon_storage(id);
-
  if (!mon_storage)
  return;
  da_monitor_reset_hook(&mon_storage->rv.da_mon);
  hash_del_rcu(&mon_storage->node);
- kfree_rcu(mon_storage, rcu);
+ if (da_pool.storage)
+ call_rcu(&mon_storage->rcu, da_pool_return_cb);
+ else
+ kfree_rcu(mon_storage, rcu);
 }

include/rv/da_monitor.h +1 -1

-static void da_monitor_reset_all(void)
+static __maybe_unused void da_monitor_reset_all(void)

 {
  struct da_monitor_storage *mon_storage;
  int bkt;

include/rv/da_monitor.h +0 -0

@@ -510,13 +606,65 @@ static void da_monitor_reset_all(void)
   rcu_read_unlock();
  }

+/*
+ * da_monitor_init_prealloc - initialise with a pre-allocated storage pool
+ *
+ * Allocates @prealloc_count storage slots up-front so that da_create_or_get()
+ * and da_destroy_storage() never call kmalloc/kfree.  Must be called instead
+ * of da_monitor_init() for monitors that require pool mode.
+ */

include/rv/da_monitor.h +26 -0

+static inline int da_monitor_init_prealloc(unsigned int prealloc_count)
+{
+ hash_init(da_monitor_ht);
+
+ da_pool.storage = kcalloc(prealloc_count, sizeof(*da_pool.storage),
+   GFP_KERNEL);
+ if (!da_pool.storage)
+ return -ENOMEM;
+
+ da_pool.free = kmalloc_array(prealloc_count, sizeof(*da_pool.free),
+      GFP_KERNEL);
+ if (!da_pool.free) {
+ kfree(da_pool.storage);
+ da_pool.storage = NULL;
+ return -ENOMEM;
+ }
+
+ da_pool.free_top = 0;
+ for (unsigned int i = 0; i < prealloc_count; i++)
+ da_pool.free[da_pool.free_top++] = &da_pool.storage[i];
+ return 0;
+}
+
+/*
+ * da_monitor_init - initialise in kmalloc mode (no pre-allocation)
+ */

 static inline int da_monitor_init(void)
 {
  hash_init(da_monitor_ht);
  return 0;
 }

include/rv/da_monitor.h +20 -1

-static inline void da_monitor_destroy(void)
+static inline void da_monitor_destroy_pool(void)
+{
+ WARN_ON_ONCE(!hash_empty(da_monitor_ht));
+ /*
+ * Wait for all in-flight da_pool_return_cb() callbacks to
+ * complete before freeing da_pool.free.  synchronize_rcu() is
+ * not sufficient: it only waits for callbacks registered before
+ * it was called, but call_rcu() from concurrent da_destroy_storage()
+ * calls may have been enqueued later.  rcu_barrier() drains every
+ * pending callback.
+ */
+ rcu_barrier();
+ kfree(da_pool.storage);
+ da_pool.storage = NULL;
+ kfree(da_pool.free);
+ da_pool.free = NULL;
+ da_pool.free_top = 0;
+}
+
+static inline void da_monitor_destroy_kmalloc(void)

 {
  struct da_monitor_storage *mon_storage;
  struct hlist_node *tmp;

include/rv/da_monitor.h +0 -0

@@ -534,6 +682,22 @@ static inline void da_monitor_destroy(void)
   }
  }

include/rv/da_monitor.h +16 -0

+/*
+ * da_monitor_destroy - tear down the per-object monitor
+ *
+ * Pool mode: the hash must already be empty (caller must have drained all
+ * tasks first); calls rcu_barrier() to drain all pending da_pool_return_cb()
+ * callbacks before freeing pool arrays.
+ * Kmalloc mode: drains any remaining entries after synchronize_rcu().
+ */
+static inline void da_monitor_destroy(void)
+{
+ if (da_pool.storage)
+ da_monitor_destroy_pool();
+ else
+ da_monitor_destroy_kmalloc();
+}
+

 /*
  * Allow the per-object monitors to run allocation manually, necessary if the
  * start condition is in a context problematic for allocation (e.g. scheduling).

That's definitely a good addition, kmalloc_nolock was not that good already so I tried some way have a preallocation, though I realise it isn't really flexible.

Since you're using spinlocks, isn't that going to sleep on PREEMPT_RT?

Isn't this similar to what you'd do with a kmem_cache. That was my original idea although that uses spinlocks too.

I quickly tried an implementation like yours using mempool_create_slab_pool(prealloc_count) and mempool_alloc_preallocated() and it still explodes with my monitors, but perhaps now that tracepoints no longer disable preemption it could play well with some monitors.

The selftests with tlob seem to work just the same with this kmem_cache (up to the unrelated RCU stall). To be fair since you only allocate from the uprobe handler, you'd probably be just fine with kmalloc_nolock, but let's continue with the preallocation logic.

The API is starting to get complex (well, not that it wasn't already). We have essentially 3 ways to allocate:
* fully automatic with kmalloc_nolock
* semi-automatic with pool preallocation
* manual with direct storage preallocation

We can have a macro DA_MON_ALLOCATION_STRATEGY = {DA_ALLOC_AUTO, DA_ALLOC_POOL, DA_ALLOC_MANUAL} where DA_MON_POOL also requires DA_MON_POOL_SIZE to be define (force that with an #error).

And isn't it clearer to do:
dir=$(realpath "$(dirname "$0")")

Then if you really really need to call it directly, do you need to override PATH?

Since you're using spinlocks, isn't that going to sleep on PREEMPT_RT?

We can have a macro DA_MON_ALLOCATION_STRATEGY = {DA_ALLOC_AUTO, DA_ALLOC_POOL, DA_ALLOC_MANUAL} where DA_MON_POOL also requires DA_MON_POOL_SIZE to be defined (force that with an #error).

Anyway, this way you probably wouldn't need to define a different init function and let everything handled more transparently.

Also you don't need to call da_create_or_get() explicitly, da_handle_start_event() should do it for you.

Why don't you make it a separate event (e.g. "start_tlob") [...] then you also wouldn't need to call reset() and start_timer() manually.

Could you have everything that isn't strictly tlob-related in another patch.

I'm not particularly fond of ioctls, they aren't that flexible and in this way I don't really see an added value.
[...] cannot the same thing be achieved using uprobes alone, e.g. by registering a function address or the current instruction pointer? [...] wouldn't a sysfs/tracefs file achieve a similar purpose without much of the boilerplate code?

Perhaps keep the handler simpler by moving this reporting to a helper function and use guard(rcu)() there.

You probably don't need these. da_handle_event should skip tasks without a monitor.

That should be a scoped_guard(rcu), definitely use guards if you have return paths, the compiler is going to clean up (unlock) for you.

All this function does should be done by da_monitor_destroy. We could add a way to pass some additional deallocation for all the other cleanup you're doing on each storage. Something like a da_extra_cleanup() you can define as whatever you need and gets called in all per-obj destruction paths.

Why don't you just override EVENT_NONE_LBL (and if you prefer call it MONITOR_TIMER_EVENT_NAME) without the need for another function?

Do you need to add this here? Since you have a patch adding KUnit tests to tlob, cannot you put everything kunit-related there?
I couldn't build it as module.

Since you are not documenting the detail_env_tlob tracepoint, is it something really required? I would at the very least document its usage.

This is standard tracepoints usage, there's nothing about tlob we should document here.
Same here, standard RV [for enable/desc tracefs files].
And this is duplicating what mentioned above about uprobes, isn't it?

Is kernel code going to use this API? RV monitors are meant to be enabled by userspace. What's the use-case here?

That's probably a bit too detailed for this page. If you really want this information somewhere couldn't it stay in the code?

What caught my eyes are tests enrolling tracepoints handlers. If you go there you're no longer doing unit testing, what's the advantage of testing the entire monitor here over doing that in selftests?

I run it on a VM and have it hanging at step 9 [...] rcu_preempt stall. Did you see that? Am I doing something wrong?

This should be tested together with the other monitors (enable/disable), we could at most expand those with the check_requires.
Let's focus on tlob-only features in this patch.

So this is what I meant. It's quick and dirty but seems to work as far as I could test it.

I didn't change too much around to avoid confusing more, but it probably needs a refactor for the functions positions and names. Some AI can do that later after we agree on how it should look.

The main idea is (using current function names):

da_handle_start_[run_]_event() calls da_prepare_storage(), this makes sure the storage is there and usable based on the strategy:
1. da_create_storage() plain allocation with kmalloc_nolock
2. da_create_or_get_pool() get a slot from the pool
3. da_fill_empty_storage() only set the target in a storage manually allocated before

The reason why you'd need 3. is that since da_handle_start_event() is called from a tracepoint, you may in no way be able to allocate from there, then you use manually somewhere else with da_create_empty_storage() if you don't have the target and da_create_or_get() if you do (this one is misleading, we should probably simplify further).

The newly created 2. might be useful if you aren't on preempt-rt and cannot sleep but also don't want a manual allocation (beware of lock dependencies, it doesn't always work).

Now, I left your da_create_or_get_kmalloc() unwired because I don't really see the use case (you use kmalloc_nolock because you cannot lock, so if it fails you don't try a kmalloc). But if we really want to offer a possibility to allocate with GFP_KERNEL, we can make 1. more configurable.

Does this make sense to you?

Thanks,
Gabriele

---
  include/rv/da_monitor.h                  | 160 ++++++++++-------------
  kernel/trace/rv/monitors/nomiss/nomiss.c |   2 +-
  kernel/trace/rv/monitors/tlob/tlob.c     |  15 +--
  3 files changed, 74 insertions(+), 103 deletions(-)

include/rv/da_monitor.h +70 -90

index 74aa95d9a284..3b4a36245531 100644
@@ -21,6 +21,7 @@
  #include <linux/sched.h>
  #include <linux/slab.h>
  #include <linux/hashtable.h>
+#include <linux/mempool.h>
 
  /*
   * Per-cpu variables require a unique name although static in some

@@ -67,6 +68,35 @@ static struct rv_monitor rv_this;
  #define da_id_type int
  #endif
 
+#define DA_ALLOC_AUTO 0
+#define DA_ALLOC_POOL 1
+#define DA_ALLOC_MANUAL 2
+
+/*
+ * Allow the per-object monitors to run allocation manually, necessary if the
+ * start condition is in a context problematic for allocation (e.g. scheduling).
+ * In such case, if the storage was pre-allocated without a target, set it now.
+ */
+#ifndef DA_MON_ALLOCATION_STRATEGY
+#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_AUTO
+#endif
+#ifndef DA_MON_POOL_SIZE
+#define DA_MON_POOL_SIZE 0
+#endif
+#if DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_MANUAL
+#define da_prepare_storage da_fill_empty_storage
+
+#elif DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL
+#define da_prepare_storage da_create_or_get_pool
+#if DA_MON_POOL_SIZE == 0
+#error "DA_ALLOC_POOL requires DA_MON_POOL_SIZE to be non-zero"
+#endif
+
+#else
+#define da_prepare_storage da_create_storage
+#endif /* DA_MON_ALLOCATION_STRATEGY */
+
+
  static void react(enum states curr_state, enum events event)
  {
  	rv_react(&rv_this,

@@ -448,62 +478,38 @@ static inline monitor_target da_get_target_by_id(da_id_type id)
  }
 
  /*
- * Per-object pool state.
- *
- * Zero-initialised by default (storage == NULL ⟹ kmalloc mode).  A monitor
- * opts into pool mode by calling da_monitor_init_prealloc(N) instead of
- * da_monitor_init(), which sets storage to a non-NULL kcalloc'd array.
- *
- * Because every field is wrapped in this struct and the struct itself is a
- * per-TU static, each monitor that includes this header gets a completely
- * independent pool.  A kmalloc monitor (e.g. nomiss) and a pool monitor
- * (e.g. tlob) therefore coexist without any interference.
- *
- * da_pool_return_cb runs from softirq on non-PREEMPT_RT, so irqsave is
- * required to prevent deadlock with task-context callers.  On PREEMPT_RT
- * it runs from an rcuc kthread where spinlock_t is a sleeping lock.
- */
-struct da_per_obj_pool {
-	struct da_monitor_storage  *storage;  /* non-NULL ⟹ pool mode */
-	struct da_monitor_storage **free;     /* kmalloc'd pointer stack */
-	unsigned int                free_top;
-	spinlock_t                  lock;
-};
-
-static struct da_per_obj_pool da_pool = {
-	.lock = __SPIN_LOCK_UNLOCKED(da_pool.lock),
-};
+ * Per-object pool state using kmem_cache and mempool.
+ */
+static struct kmem_cache *da_mon_cache;
+static mempool_t *da_mon_pool;
 
  static void da_pool_return_cb(struct rcu_head *head)
  {
  	struct da_monitor_storage *ms =
  		container_of(head, struct da_monitor_storage, rcu);
-	unsigned long flags;
-
-	spin_lock_irqsave(&da_pool.lock, flags);
-	da_pool.free[da_pool.free_top++] = ms;
-	spin_unlock_irqrestore(&da_pool.lock, flags);
+	mempool_free(ms, da_mon_pool);
  }
 
-/* Pops a slot from the pre-allocated pool; returns -ENOSPC if exhausted. */
-static inline int da_create_or_get_pool(da_id_type id, monitor_target target)
+/* Pops a slot from the pre-allocated pool; returns NULL if exhausted. */
+static inline struct da_monitor *da_create_or_get_pool(da_id_type id,
+						       monitor_target target,
+						       struct da_monitor *da_mon)
  {
  	struct da_monitor_storage *mon_storage;
-	unsigned long flags;
 
-	spin_lock_irqsave(&da_pool.lock, flags);
-	if (!da_pool.free_top) {
-		spin_unlock_irqrestore(&da_pool.lock, flags);
-		return -ENOSPC;
-	}
-	mon_storage = da_pool.free[--da_pool.free_top];
-	spin_unlock_irqrestore(&da_pool.lock, flags);
+	if (da_mon)
+		return da_mon;
 
+	mon_storage = mempool_alloc_preallocated(da_mon_pool);
+	if (!mon_storage)
+		return NULL;
+
+	memset(mon_storage, 0, sizeof(*mon_storage));
  	mon_storage->id = id;
  	mon_storage->target = target;
  	guard(rcu)();
  	hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
-	return 0;
+	return &mon_storage->rv.da_mon;
  }
 
  /*

@@ -547,11 +553,12 @@ static inline int da_create_or_get_kmalloc(da_id_type id, monitor_target target)
  }
 
  /* Create the per-object storage if not already there. */
-static inline int da_create_or_get(da_id_type id, monitor_target target)
+// NOTE: this is only needed for manual allocation!
+// we can refactor to have it only defined there, leaving it for now
+static inline void da_create_or_get(da_id_type id, monitor_target target)
  {
-	if (da_pool.storage)
-		return da_create_or_get_pool(id, target);
-	return da_create_or_get_kmalloc(id, target);
+	guard(rcu)();
+	da_create_storage(id, target, da_get_monitor(id, target));
  }
 
  /*

@@ -573,7 +580,7 @@ static inline void da_destroy_storage(da_id_type id)
  		return;
  	da_monitor_reset_hook(&mon_storage->rv.da_mon);
  	hash_del_rcu(&mon_storage->node);
-	if (da_pool.storage)
+	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
  		call_rcu(&mon_storage->rcu, da_pool_return_cb);
  	else
  		kfree_rcu(mon_storage, rcu);

@@ -591,41 +598,26 @@ static __maybe_unused void da_monitor_reset_all(void)
  }
 
  /*
- * da_monitor_init_prealloc - initialise with a pre-allocated storage pool
- *
- * Allocates @prealloc_count storage slots up-front so that da_create_or_get()
- * and da_destroy_storage() never call kmalloc/kfree.  Must be called instead
- * of da_monitor_init() for monitors that require pool mode.
+ * da_monitor_init - initialise in kmalloc mode (no pre-allocation)
   */
-static inline int da_monitor_init_prealloc(unsigned int prealloc_count)
+static inline int da_monitor_init(void)
  {
  	hash_init(da_monitor_ht);
+	if (DA_MON_ALLOCATION_STRATEGY != DA_ALLOC_POOL)
+		return 0;
 
-	da_pool.storage = kcalloc(prealloc_count, sizeof(*da_pool.storage),
-				  GFP_KERNEL);
-	if (!da_pool.storage)
+	da_mon_cache = kmem_cache_create(__stringify(DA_MON_NAME) "_cache",
+					 sizeof(struct da_monitor_storage),
+					 0, 0, NULL);
+	if (!da_mon_cache)
  		return -ENOMEM;
 
-	da_pool.free = kmalloc_array(prealloc_count, sizeof(*da_pool.free),
-				     GFP_KERNEL);
-	if (!da_pool.free) {
-		kfree(da_pool.storage);
-		da_pool.storage = NULL;
+	da_mon_pool = mempool_create_slab_pool(DA_MON_POOL_SIZE, da_mon_cache);
+	if (!da_mon_pool) {
+		kmem_cache_destroy(da_mon_cache);
+		da_mon_cache = NULL;
  		return -ENOMEM;
  	}
-
-	da_pool.free_top = 0;
-	for (unsigned int i = 0; i < prealloc_count; i++)
-		da_pool.free[da_pool.free_top++] = &da_pool.storage[i];
-	return 0;
-}
-
-/*
- * da_monitor_init - initialise in kmalloc mode (no pre-allocation)
- */
-static inline int da_monitor_init(void)
-{
-	hash_init(da_monitor_ht);
  	return 0;
  }
 

@@ -641,11 +633,10 @@ static inline void da_monitor_destroy_pool(void)
  	 * pending callback.
  	 */
  	rcu_barrier();
-	kfree(da_pool.storage);
-	da_pool.storage = NULL;
-	kfree(da_pool.free);
-	da_pool.free = NULL;
-	da_pool.free_top = 0;
+	mempool_destroy(da_mon_pool);
+	da_mon_pool = NULL;
+	kmem_cache_destroy(da_mon_cache);
+	da_mon_cache = NULL;
  }
 
  static inline void da_monitor_destroy_kmalloc(void)

@@ -676,23 +667,12 @@ static inline void da_monitor_destroy_kmalloc(void)
   */
  static inline void da_monitor_destroy(void)
  {
-	if (da_pool.storage)
+	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
  		da_monitor_destroy_pool();
  	else
  		da_monitor_destroy_kmalloc();
  }
 
-/*
- * Allow the per-object monitors to run allocation manually, necessary if the
- * start condition is in a context problematic for allocation (e.g. scheduling).
- * In such case, if the storage was pre-allocated without a target, set it now.
- */
-#ifdef DA_SKIP_AUTO_ALLOC
-#define da_prepare_storage da_fill_empty_storage
-#else
-#define da_prepare_storage da_create_storage
-#endif /* DA_SKIP_AUTO_ALLOC */
-
  #endif /* RV_MON_TYPE */
 
  #if RV_MON_TYPE == RV_MON_GLOBAL || RV_MON_TYPE == RV_MON_PER_CPU

kernel/trace/rv/monitors/nomiss/nomiss.c +1 -1

index 31f90f3638d8..f089cc0e2f10 100644
@@ -18,7 +18,7 @@
  #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
+#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL
  typedef struct sched_dl_entity *monitor_target;
  #include "nomiss.h"
  #include <rv/ha_monitor.h>

kernel/trace/rv/monitors/tlob/tlob.c +3 -12

index 90e7035a0b55..486a6bac5cf9 100644
@@ -88,8 +88,8 @@ struct tlob_task_state {
 
  #define RV_MON_TYPE RV_MON_PER_OBJ
  #define HA_TIMER_TYPE HA_TIMER_HRTIMER
-/* Pool mode: da_handle_start_event uses da_fill_empty_storage, not kmalloc. */
-#define DA_SKIP_AUTO_ALLOC
+#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_POOL
+#define DA_MON_POOL_SIZE TLOB_MAX_MONITORED
 
  /* Type for da_monitor_storage.target; must be defined before the includes. */
  typedef struct tlob_task_state *monitor_target;

@@ -428,7 +428,6 @@ int tlob_start_task(struct task_struct *task, u64 threshold_us)
  	struct da_monitor *da_mon;
  	struct ha_monitor *ha_mon;
  	u64 now_ns;
-	int ret;
 
  	if (!da_monitor_enabled())
  		return -ENODEV;

@@ -457,14 +456,6 @@ int tlob_start_task(struct task_struct *task, u64 threshold_us)
  	ws->last_ts = ktime_get();
  	raw_spin_lock_init(&ws->entry_lock);
 
-	/* Claim a pool slot (no kmalloc; DA_SKIP_AUTO_ALLOC + prealloc). */
-	ret = da_create_or_get(task->pid, ws);
-	if (ret) {
-		put_task_struct(task);
-		kmem_cache_free(tlob_state_cache, ws);
-		return ret;
-	}
-
  	atomic_inc(&tlob_num_monitored);
 
  	/* Hold RCU across handle + timer setup to keep da_mon valid. */

@@ -955,7 +946,7 @@ static int __tlob_init_monitor(void)
 
  	atomic_set(&tlob_num_monitored, 0);
 
-	retval = da_monitor_init_prealloc(TLOB_MAX_MONITORED);
+	retval = da_monitor_init();
  	if (retval) {
  		kmem_cache_destroy(tlob_state_cache);
  		tlob_state_cache = NULL;

Yes.  The ftracetest check_requires logic calls command -v <binary> to satisfy requires: <name>:program directives.  Without the script's directory in PATH those checks evaluate to exit_unsupported and test cases are skipped rather than run.  The make path avoids this only because make sets OUTDIR and the runner appends it to PATH internally.

-- Patch 04: pre-allocated storage pool

 > Since you're using spinlocks, isn't that going to sleep on PREEMPT_RT?

User-mode uprobe handlers run with preempt_count == 0, fully preemptible on both PREEMPT_RT and non-PREEMPT_RT.  The strongest evidence is in tlob
itself: tlob_start_task() takes a mutex and calls kmem_cache_zalloc(..., GFP_KERNEL) from the uprobe entry handler; both are illegal in atomic context and would trigger lockdep splats immediately.

On PREEMPT_RT, spinlock_t becoming a sleeping lock in the uprobe handler
iscfine: both call sites (da_create_or_get_pool() from the handler and da_pool_return_cb() from the rcuc kthread) are in sleepable task context.

 > We can have a macro DA_MON_ALLOCATION_STRATEGY = {DA_ALLOC_AUTO, > DA_ALLOC_POOL, DA_ALLOC_MANUAL} where DA_MON_POOL also requires > DA_MON_POOL_SIZE to be defined (force that with an #error). >
 > Anyway, this way you probably wouldn't need to define a different init > function and let everything handled more transparently.
 >
 > Also you don't need to call da_create_or_get() explicitly, > da_handle_start_event() should do it for you.

Agreed on all counts.  We plan to implement this in v3 as follows. The three strategies would be a compile-time selection in da_monitor.h:

   DA_ALLOC_AUTO   (default) - lock-free kmalloc_nolock on the hot path; unbounded capacity.

   DA_ALLOC_POOL             - pre-allocated fixed-size pool; DA_MON_POOL_SIZE
                               required, enforced with #error if missing.

   DA_ALLOC_MANUAL           - caller pre-inserts storage via da_create_empty_storage() before the first da_handle_start_event(); the framework only links the target field.

da_monitor_init_prealloc() would be removed; da_monitor_init() would select pool or kmalloc initialisation internally based on the strategy.

da_handle_start_event() and da_handle_start_run_event() would both call da_prepare_storage() at compile time:

   DA_ALLOC_AUTO   -> da_create_storage()        (kmalloc_nolock) DA_ALLOC_POOL   -> da_create_or_get_pool() DA_ALLOC_MANUAL -> da_fill_empty_storage()    (link target into pre- allocated slot; no allocation on the hot path)

No explicit da_create_or_get() call would be needed in any monitor.

da_create_or_get_kmalloc() would be removed: as you noted, a caller that uses kmalloc_nolock does so because locking is forbidden; a GFP_KERNEL fallback is equally forbidden if the lockless attempt fails, so the function has no viable use case.

tlob would define:
   #define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_POOL #define DA_MON_POOL_SIZE            TLOB_MAX_MONITORED

nomiss would define:
   #define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL

and call da_create_empty_storage() from handle_sys_enter() (the sched_setscheduler syscall path), which runs in safe task context; da_fill_empty_storage() would then link the sched_dl_entity target on the first da_handle_start_run_event() call in handle_sched_switch().

-- Patch 05: generic uprobe infrastructure

Carried unchanged into v3 (as part of the 08-b split described below).

-- Patch 06: rvgen __init arrow reset

Thanks, carried unchanged into v3.

 > Why don't you make it a separate event (e.g. "start_tlob") [...] then > you also wouldn't need to call reset() and start_timer() manually.

Good suggestion.  We plan to use a dedicated start_tlob event instead, with a self-loop in tlob.dot:

   "running" -> "running" [ label = "start;reset(clk_elapsed)" ]

da_handle_start_run_event(task->pid, ws, start_tlob) would put the monitor into running and deliver start_tlob, which resets clk_elapsed and arms the budget hrtimer via the generated ha_setup_invariants() — no manual reset() or start_timer() calls needed.

One guard would be added in tlob's ha_setup_invariants() to make the self-loop work correctly:

   if (next_state == curr_state && event != start_tlob) return;

Without this, the start_tlob self-loop would be treated the same as any repeated switch_in (already running) and ha_setup_invariants() would return early, leaving the timer unarmed.  Does this look right to you?

-- Patch 08: tlob monitor

--- Patch structure ---

 > Could you have everything that isn't strictly tlob-related in another > patch.

Agreed.  With the ioctl interface deferred (see below), v3 would keep patch 08 as the tlob monitor only:

   05-b: rv: extend uprobe API with three-phase detach helpers (rv_uprobe.c, rv_uprobe.h, rv_uprobe_detach refactoring) — extension of patch 05, independent of tlob

   08:   rv/tlob: add the tlob monitor itself
         (tlob.c, tlob.h, tlob_trace.h, Kconfig/Makefile, Documentation, rv_trace.h include; ha_monitor.h EVENT_NONE_LBL override bundled here as it is only needed by tlob)

The chardev infrastructure (rv_chardev.c, rv.h additions) and the UAPI header (include/uapi/linux/rv.h) would move to a follow-up series together with the ioctl self-instrumentation feature.

--- ioctl interface design ---

 > I'm not particularly fond of ioctls, they aren't that flexible and in > this way I don't really see an added value.
 > [...] cannot the same thing be achieved using uprobes alone, e.g. by > registering a function address or the current instruction pointer? > [...] wouldn't a sysfs/tracefs file achieve a similar purpose without > much of the boilerplate code?

Fair point.  We plan to ship v3 with the tracefs/uprobe interface only and defer the ioctl (/dev/rv chardev) to a follow-up series once there is a concrete in-tree user that requires it.

The unique value of the ioctl is that TLOB_IOCTL_TRACE_STOP returns a synchronous per-call result (-EOVERFLOW or 0) to the calling thread, which neither uprobes nor tracefs writes can provide.  We want to keep that option open for later, but agree it should not block the initial tlob submission.

Does this approach work for you?

What is your preference?

--- Handler simplification ---

 > Perhaps keep the handler simpler by moving this reporting to a helper > function and use guard(rcu)() there.

Done.  The accumulation logic is extracted into three inline helpers, each using scoped_guard(rcu) and returning bool (true if the task is monitored):

   tlob_acc_running(prev)   - accumulate running_ns on sched-out tlob_acc_waiting(next)   - accumulate waiting_ns on sched-in tlob_acc_sleeping(task)  - accumulate sleeping_ns on wakeup

handle_sched_switch() and handle_sched_wakeup() become one-liners:

   static void handle_sched_switch(...)
   {
       bool prev_preempted = (prev_state == 0);

       if (tlob_acc_running(prev)) da_handle_event(prev->pid, NULL,
                           prev_preempted ? preempt_tlob : sleep_tlob); if (tlob_acc_waiting(next)) da_handle_event(next->pid, NULL, switch_in_tlob); }

 > You probably don't need these. da_handle_event should skip tasks without > a monitor.

Agreed; the do_prev/do_next flags are gone.  The helpers return false for unmonitored tasks, and da_handle_event() skips them too — both paths are no-ops for tasks with no pool entry.

--- scoped_guard(rcu) ---

 > That should be a scoped_guard(rcu), definitely use guards if you have > return paths, the compiler is going to clean up (unlock) for you.

Applied to all RCU-protected sections in tlob_start_task() and tlob_stop_task().  tlob_start_task() now uses guard(mutex) for the serialised duplicate-check (replacing the explicit mutex_lock/unlock), and tlob_stop_task() uses scoped_guard(rcu) for the atomic CAS section:

   scoped_guard(rcu) {
       ws = da_get_target_by_id(task->pid);
       if (!ws)
           return -ESRCH;
       ...
       if (atomic_cmpxchg_release(&ws->stopping, 0, 1) != 0) return -EAGAIN;
   }

--- tlob_stop_all removal ---

 > All this function does should be done by da_monitor_destroy. We could > add a way to pass some additional deallocation for all the other cleanup > you're doing on each storage.  Something like a da_extra_cleanup() you > can define as whatever you need and gets called in all per-obj > destruction paths.

Agreed.  tlob_stop_all() (~50 lines) has been removed entirely.

A da_extra_cleanup() hook macro is introduced in da_monitor.h: the default is a no-op; a monitor may override it before including the header.  tlob defines:

   static inline void tlob_extra_cleanup(struct da_monitor *da_mon) {
       struct ha_monitor *ha_mon = to_ha_monitor(da_mon); struct tlob_task_state *ws = da_get_target(ha_mon);

       if (!ws)
           return;
       if (atomic_cmpxchg_release(&ws->stopping, 0, 1) != 0) return;

       ha_cancel_timer_sync(ha_mon);

       atomic_dec(&tlob_num_monitored);
       put_task_struct(ws->task);
       call_rcu(&ws->rcu, tlob_free_rcu);
   }
   #define da_extra_cleanup tlob_extra_cleanup

da_monitor_destroy() iterates remaining entries via da_extra_cleanup + hash_del_rcu + call_rcu, then waits for all callbacks via rcu_barrier(). tlob's disable path is now simply:

   static void __tlob_destroy_monitor(void)
   {
       da_monitor_destroy();
   }

--- EVENT_NONE_LBL ---

 > Why don't you just override EVENT_NONE_LBL (and if you prefer call it > MONITOR_TIMER_EVENT_NAME) without the need for another function?

Done.  model_get_timer_event_name() has been removed from automata.h. In ha_monitor.h, EVENT_NONE_LBL is now overridable:

   #ifndef EVENT_NONE_LBL
   #define EVENT_NONE_LBL "none"
   #endif

tlob.c defines it before including the model header:

   #define EVENT_NONE_LBL "budget_exceeded"

The two call sites in ha_monitor.h that previously called model_get_timer_event_name() now use EVENT_NONE_LBL directly.

--- KUnit config / tristate ---

 > Do you need to add this here? Since you have a patch adding KUnit tests > to tlob, cannot you put everything kunit-related there?
 > I couldn't build it as module.

Agreed on moving the Kconfig entry to patch 09.

The module build issue is fixed by exporting the symbols needed by the test via EXPORT_SYMBOL_IF_KUNIT (EXPORTED_FOR_KUNIT_TESTING namespace); tlob_kunit.c imports the namespace with MODULE_IMPORT_NS.  We plan to keep tristate rather than changing to bool.  Does that work for you?

--- detail_env_tlob tracepoint ---

 > Since you are not documenting the detail_env_tlob tracepoint, is it > something really required? I would at the very least document its usage.

Fair point.  detail_env_tlob emits (running_ns, waiting_ns, sleeping_ns) so the user can see which phase consumed the budget: high sleeping_ns indicates I/O latency, high waiting_ns indicates scheduler pressure, high running_ns indicates a compute overrun.  Without this breakdown the user only knows the total elapsed time exceeded the threshold, not why.

--- Documentation ---

 > This is standard tracepoints usage, there's nothing about tlob we should > document here.
 > Same here, standard RV [for enable/desc tracefs files].
 > And this is duplicating what mentioned above about uprobes, isn't it?

Agreed.  The following have been removed:

   - "Violation events" section: generic trace-cmd examples and cat-trace instructions (standard tracepoints usage).
   - tracefs files: "enable (rw)" and "desc (ro)" entries (standard RV).
   - tracefs files: "monitor (rw)" description condensed to one line with a cross-reference to the uprobes section above.

In their place, a new "Violation tracepoints" subsection documents both tlob-specific tracepoints with fields and a worked example:

   error_env_tlob: id, state, event ("budget_exceeded"), env ("clk_elapsed")

   detail_env_tlob: id, threshold_us, running_ns, waiting_ns, sleeping_ns Use sleeping_ns to diagnose I/O latency, waiting_ns for scheduler pressure, running_ns for compute overruns.

   Example:
     trace-cmd record -e error_env_tlob -e detail_env_tlob &

     # ... run workload ...
     trace-cmd report

 > Is kernel code going to use this API? RV monitors are meant to be > enabled by userspace. What's the use-case here?

Agreed.  The uprobe interface is driven from userspace; tlob_start_task() and tlob_stop_task() are the internal implementation functions it calls, not a public API for external kernel modules.  The hypothetical kernel-module use case would be removed from the documentation; the kernel-doc block is retained for code maintainers.

 > That's probably a bit too detailed for this page. If you really want > this information somewhere couldn't it stay in the code?

Agreed; moved to comments in handle_sched_switch() and handle_sched_wakeup().  The "Limitations" subsection is retained.

-- Patch 09: KUnit tests

 > What caught my eyes are tests enrolling tracepoints handlers. If you > go there you're no longer doing unit testing, what's the advantage of > testing the entire monitor here over doing that in selftests?

Agreed.  The three suites that register tracepoint handlers or create kthreads (tlob_sched_integration, tlob_trace_output, tlob_violation_react) have been removed from KUnit and will be added to selftests in v3.

Two pure unit test suites remain in KUnit:

   tlob_task_api:
     Tests tlob_start_task / tlob_stop_task return values (-ENODEV, -EALREADY, -ESRCH, -EOVERFLOW, -ENOSPC, -ERANGE) via direct calls (these functions are the internal implementation used by both the uprobe and, in future, the ioctl interface).
     No tracepoints, no scheduling.

   tlob_uprobe_format:
     Tests the uprobe line parser (tlob_parse_uprobe_line, tlob_parse_remove_line) against valid and invalid input strings. Pure string parsing; no scheduling, no tracepoints.

This also resolves the tristate-vs-bool issue: with only pure unit tests there is no dependency on sched_setscheduler_nocheck, so bool is correct.

--  Patch 10: selftests

--- PREEMPT_RT RCU stall ---

 > I run it on a VM and have it hanging at step 9 [...] rcu_preempt stall. > Did you see that? Am I doing something wrong?

Thanks for reporting.  The patch changed ha_monitor.h from HRTIMER_MODE_REL_HARD (the existing upstream value) to REL_SOFT; the stall appeared on PREEMPT_RT after that change.  We have not fully confirmed whether REL_SOFT is the root cause — REL_SOFT defers the callback to the ktimers kthread, which could starve rcu_preempt under certain PREEMPT_RT configurations, but other factors may be involved.

We plan to revert to HRTIMER_MODE_REL_HARD at both sites in ha_monitor.h as the conservative choice:

   ha_setup_timer():     HRTIMER_MODE_REL_SOFT -> HRTIMER_MODE_REL_HARD ha_start_timer_ns():  HRTIMER_MODE_REL_SOFT -> HRTIMER_MODE_REL_HARD

Do you have more insight into the stall, or does REL_HARD resolve it on your setup?

--- Selftest structure ---

 > This should be tested together with the other monitors (enable/disable), > we could at most expand those with the check_requires.
 > Let's focus on tlob-only features in this patch.

Agreed.  In v3 we plan to drop tracefs.tc (covered by the generic rv_monitor_enable_disable.tc) and keep only the six uprobe-specific test cases under test.d/tlob/

ioctl.tc is deferred with the ioctl interface to the follow-up series. The KUnit integration tests (sched_switch accounting, budget-expiry tracepoint) would be moved to selftests as additional test cases.

Hi Wen,

Quoted 7 lines · Wen Yang

Yes.  The ftracetest check_requires logic calls command -v <binary> to satisfy requires: <name>:program directives.  Without the script's directory in PATH those checks evaluate to exit_unsupported and test cases are skipped rather than run.  The make path avoids this only because make sets OUTDIR and the runner appends it to PATH internally.

So you're overriding PATH so the selftest's binaries can be found from the test, right?

Wouldn't it be simpler to just put the absolute paths in the tests and don't touch PATH.

If the selftests are run via makefile, it ensures the required binaries are built and available, so there's no need to go through the `requires:
<name>:program` infrastructure (that's more about what's installed on the system.

Or if you don't want anything hardcoded, you could pass the $OUTDIR from the environment and use that in scripts, whatever looks cleaner.

Does it make sense to you?

Quoted 11 lines

-- Patch 04: pre-allocated storage pool

 > Since you're using spinlocks, isn't that going to sleep on PREEMPT_RT?

User-mode uprobe handlers run with preempt_count == 0, fully preemptible on both PREEMPT_RT and non-PREEMPT_RT.  The strongest evidence is in tlob
itself: tlob_start_task() takes a mutex and calls kmem_cache_zalloc(..., GFP_KERNEL) from the uprobe entry handler; both are illegal in atomic context and would trigger lockdep splats immediately.

Quoted 4 lines

On PREEMPT_RT, spinlock_t becoming a sleeping lock in the uprobe handler
iscfine: both call sites (da_create_or_get_pool() from the handler and da_pool_return_cb() from the rcuc kthread) are in sleepable task context.

Yeah exactly, the uprobe is fine with anything (even the automatic kmalloc_nolock), but sure preallocation at least guarantees the slots are there.

Quoted 47 lines

 > We can have a macro DA_MON_ALLOCATION_STRATEGY = {DA_ALLOC_AUTO, > DA_ALLOC_POOL, DA_ALLOC_MANUAL} where DA_MON_POOL also requires > DA_MON_POOL_SIZE to be defined (force that with an #error). >
 > Anyway, this way you probably wouldn't need to define a different init > function and let everything handled more transparently.
 >
 > Also you don't need to call da_create_or_get() explicitly, > da_handle_start_event() should do it for you.

Agreed on all counts.  We plan to implement this in v3 as follows. The three strategies would be a compile-time selection in da_monitor.h:

   DA_ALLOC_AUTO   (default) - lock-free kmalloc_nolock on the hot path; unbounded capacity.

   DA_ALLOC_POOL             - pre-allocated fixed-size pool;
DA_MON_POOL_SIZE
                               required, enforced with #error if missing.

   DA_ALLOC_MANUAL           - caller pre-inserts storage via da_create_empty_storage() before the first da_handle_start_event(); the framework only links the target field.

da_monitor_init_prealloc() would be removed; da_monitor_init() would select pool or kmalloc initialisation internally based on the strategy.

da_handle_start_event() and da_handle_start_run_event() would both call da_prepare_storage() at compile time:

   DA_ALLOC_AUTO   -> da_create_storage()        (kmalloc_nolock) DA_ALLOC_POOL   -> da_create_or_get_pool() DA_ALLOC_MANUAL -> da_fill_empty_storage()    (link target into pre- allocated slot; no allocation on the hot path)

No explicit da_create_or_get() call would be needed in any monitor.

da_create_or_get_kmalloc() would be removed: as you noted, a caller that uses kmalloc_nolock does so because locking is forbidden; a GFP_KERNEL fallback is equally forbidden if the lockless attempt fails, so the function has no viable use case.

tlob would define:
   #define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_POOL #define DA_MON_POOL_SIZE            TLOB_MAX_MONITORED

Quoted 7 lines

nomiss would define:
   #define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL

and call da_create_empty_storage() from handle_sys_enter() (the sched_setscheduler syscall path), which runs in safe task context; da_fill_empty_storage() would then link the sched_dl_entity target on the first da_handle_start_run_event() call in handle_sched_switch().

Yeah good point, there's no need to make it a special path even if we have the target ready, da_handle_start_run_event() can do it just fine.

Quoted 11 lines

-- Patch 05: generic uprobe infrastructure

Carried unchanged into v3 (as part of the 08-b split described below).

-- Patch 06: rvgen __init arrow reset

Thanks, carried unchanged into v3.

Well, if you don't need reset() on the __init arrow we can drop this, right? Also it doesn't seem fully wired with the rest and requires a separate event to do handle_monitor_start(), which can be only just another handler for tlob, nothing general.

Quoted 17 lines

 > Why don't you make it a separate event (e.g. "start_tlob") [...] then > you also wouldn't need to call reset() and start_timer() manually.

Good suggestion.  We plan to use a dedicated start_tlob event instead, with a self-loop in tlob.dot:

   "running" -> "running" [ label = "start;reset(clk_elapsed)" ]

da_handle_start_run_event(task->pid, ws, start_tlob) would put the monitor into running and deliver start_tlob, which resets clk_elapsed and arms the budget hrtimer via the generated ha_setup_invariants() — no manual reset() or start_timer() calls needed.

One guard would be added in tlob's ha_setup_invariants() to make the self-loop work correctly:

Quoted 7 lines

   if (next_state == curr_state && event != start_tlob) return;

Without this, the start_tlob self-loop would be treated the same as any repeated switch_in (already running) and ha_setup_invariants() would return early, leaving the timer unarmed.  Does this look right to you?

If you just add a separate event rvgen should take care of everything, you should be able to take ha_verify_constraint() and friends as-is from the generated code.

But yeah, that's what it would end up doing.

Quoted 43 lines

-- Patch 08: tlob monitor

--- Patch structure ---

 > Could you have everything that isn't strictly tlob-related in another > patch.

Agreed.  With the ioctl interface deferred (see below), v3 would keep patch 08 as the tlob monitor only:

   05-b: rv: extend uprobe API with three-phase detach helpers (rv_uprobe.c, rv_uprobe.h, rv_uprobe_detach refactoring) — extension of patch 05, independent of tlob

   08:   rv/tlob: add the tlob monitor itself
         (tlob.c, tlob.h, tlob_trace.h, Kconfig/Makefile, Documentation, rv_trace.h include; ha_monitor.h EVENT_NONE_LBL override bundled here as it is only needed by tlob)

The chardev infrastructure (rv_chardev.c, rv.h additions) and the UAPI header (include/uapi/linux/rv.h) would move to a follow-up series together with the ioctl self-instrumentation feature.

--- ioctl interface design ---

 > I'm not particularly fond of ioctls, they aren't that flexible and in > this way I don't really see an added value.
 > [...] cannot the same thing be achieved using uprobes alone, e.g. by > registering a function address or the current instruction pointer? > [...] wouldn't a sysfs/tracefs file achieve a similar purpose without > much of the boilerplate code?

Fair point.  We plan to ship v3 with the tracefs/uprobe interface only and defer the ioctl (/dev/rv chardev) to a follow-up series once there is a concrete in-tree user that requires it.

The unique value of the ioctl is that TLOB_IOCTL_TRACE_STOP returns a synchronous per-call result (-EOVERFLOW or 0) to the calling thread, which neither uprobes nor tracefs writes can provide.  We want to keep that option open for later, but agree it should not block the initial tlob submission.

Quoted 3 lines

Does this approach work for you?

What is your preference?

Yeah looks good to me.
Ioctls are cumbersome to set up also for the user, perhaps another sysfs file in the monitor directory would keep the control entirely in tlob.c and give you roughly the same value with easier setup.

Heck we might even think of an RV reactor that does that: e.g. creates a file where reads sleep until the first reaction (-EOVERFLOW) and returns 0 in other scenarios. I'm gonna have a thought on that, but anyway I don't see why a sysfs file cannot do this.

Let's defer it for now.

Quoted 19 lines

--- Handler simplification ---

 > Perhaps keep the handler simpler by moving this reporting to a helper > function and use guard(rcu)() there.

Done.  The accumulation logic is extracted into three inline helpers, each using scoped_guard(rcu) and returning bool (true if the task is monitored):

   tlob_acc_running(prev)   - accumulate running_ns on sched-out tlob_acc_waiting(next)   - accumulate waiting_ns on sched-in tlob_acc_sleeping(task)  - accumulate sleeping_ns on wakeup

handle_sched_switch() and handle_sched_wakeup() become one-liners:

   static void handle_sched_switch(...)
   {
       bool prev_preempted = (prev_state == 0);

Quoted 6 lines

       if (tlob_acc_running(prev)) da_handle_event(prev->pid, NULL,
                           prev_preempted ? preempt_tlob : sleep_tlob); if (tlob_acc_waiting(next)) da_handle_event(next->pid, NULL, switch_in_tlob); }

Yeah sounds good.

Quoted 18 lines

 > You probably don't need these. da_handle_event should skip tasks without > a monitor.

Agreed; the do_prev/do_next flags are gone.  The helpers return false for unmonitored tasks, and da_handle_event() skips them too — both paths are no-ops for tasks with no pool entry.

--- scoped_guard(rcu) ---

 > That should be a scoped_guard(rcu), definitely use guards if you have > return paths, the compiler is going to clean up (unlock) for you.

Applied to all RCU-protected sections in tlob_start_task() and tlob_stop_task().  tlob_start_task() now uses guard(mutex) for the serialised duplicate-check (replacing the explicit mutex_lock/unlock), and tlob_stop_task() uses scoped_guard(rcu) for the atomic CAS section:

Quoted 8 lines

   scoped_guard(rcu) {
       ws = da_get_target_by_id(task->pid);
       if (!ws)
           return -ESRCH;
       ...
       if (atomic_cmpxchg_release(&ws->stopping, 0, 1) != 0) return -EAGAIN;
   }

Perfect.

Quoted 20 lines

--- tlob_stop_all removal ---

 > All this function does should be done by da_monitor_destroy. We could > add a way to pass some additional deallocation for all the other cleanup > you're doing on each storage.  Something like a da_extra_cleanup() you > can define as whatever you need and gets called in all per-obj > destruction paths.

Agreed.  tlob_stop_all() (~50 lines) has been removed entirely.

A da_extra_cleanup() hook macro is introduced in da_monitor.h: the default is a no-op; a monitor may override it before including the header.  tlob defines:

   static inline void tlob_extra_cleanup(struct da_monitor *da_mon) {
       struct ha_monitor *ha_mon = to_ha_monitor(da_mon); struct tlob_task_state *ws = da_get_target(ha_mon);

Quoted 6 lines

       if (!ws)
           return;
       if (atomic_cmpxchg_release(&ws->stopping, 0, 1) != 0) return;

       ha_cancel_timer_sync(ha_mon);

After my patch making timer callbacks RCU read-side critical section, you won't need that, just let the usual reset asynchronously stop the timer and put everything that needs it stopped in your RCU callback.

Of course make sure the timer was stopped before this extra cleanup, so put the macro accordingly.

I don't think da_extra_cleanup in general should be expected to sleep and call_rcu should do the heavy lifting (it may run from any tracepoint).

Anyway we can see it later after that's merged.

Quoted 6 lines

       atomic_dec(&tlob_num_monitored);
       put_task_struct(ws->task);
       call_rcu(&ws->rcu, tlob_free_rcu);
   }
   #define da_extra_cleanup tlob_extra_cleanup

Quoted 8 lines

da_monitor_destroy() iterates remaining entries via da_extra_cleanup + hash_del_rcu + call_rcu, then waits for all callbacks via rcu_barrier(). tlob's disable path is now simply:

   static void __tlob_destroy_monitor(void)
   {
       da_monitor_destroy();
   }

Looks good, let's see the full picture.

Quoted 25 lines

--- EVENT_NONE_LBL ---

 > Why don't you just override EVENT_NONE_LBL (and if you prefer call it > MONITOR_TIMER_EVENT_NAME) without the need for another function?

Done.  model_get_timer_event_name() has been removed from automata.h. In ha_monitor.h, EVENT_NONE_LBL is now overridable:

   #ifndef EVENT_NONE_LBL
   #define EVENT_NONE_LBL "none"
   #endif

tlob.c defines it before including the model header:

   #define EVENT_NONE_LBL "budget_exceeded"

The two call sites in ha_monitor.h that previously called model_get_timer_event_name() now use EVENT_NONE_LBL directly.

--- KUnit config / tristate ---

 > Do you need to add this here? Since you have a patch adding KUnit tests > to tlob, cannot you put everything kunit-related there?
 > I couldn't build it as module.

Quoted 6 lines

Agreed on moving the Kconfig entry to patch 09.

The module build issue is fixed by exporting the symbols needed by the test via EXPORT_SYMBOL_IF_KUNIT (EXPORTED_FOR_KUNIT_TESTING namespace); tlob_kunit.c imports the namespace with MODULE_IMPORT_NS.  We plan to keep tristate rather than changing to bool.  Does that work for you?

Yeah it's good as long as it works as module too.

I might have a look at making my patch module-ready, for now it just can't work but I wonder if we can do something nicer to allow it (like in your case a bunch of exports, a separate file and a standalone testcase, perhaps all wrapped in some helper).

Quoted 12 lines

--- detail_env_tlob tracepoint ---

 > Since you are not documenting the detail_env_tlob tracepoint, is it > something really required? I would at the very least document its usage.

Fair point.  detail_env_tlob emits (running_ns, waiting_ns, sleeping_ns) so the user can see which phase consumed the budget: high sleeping_ns indicates I/O latency, high waiting_ns indicates scheduler pressure, high running_ns indicates a compute overrun.  Without this breakdown the user only knows the total elapsed time exceeded the threshold, not why.

Alright, then this can go into the docs.

Quoted 21 lines

--- Documentation ---

 > This is standard tracepoints usage, there's nothing about tlob we should > document here.
 > Same here, standard RV [for enable/desc tracefs files].
 > And this is duplicating what mentioned above about uprobes, isn't it?

Agreed.  The following have been removed:

   - "Violation events" section: generic trace-cmd examples and cat-trace instructions (standard tracepoints usage).
   - tracefs files: "enable (rw)" and "desc (ro)" entries (standard RV).
   - tracefs files: "monitor (rw)" description condensed to one line with a cross-reference to the uprobes section above.

In their place, a new "Violation tracepoints" subsection documents both tlob-specific tracepoints with fields and a worked example:

   error_env_tlob: id, state, event ("budget_exceeded"), env ("clk_elapsed")

Quoted 8 lines

   detail_env_tlob: id, threshold_us, running_ns, waiting_ns, sleeping_ns Use sleeping_ns to diagnose I/O latency, waiting_ns for scheduler pressure, running_ns for compute overruns.

   Example:
     trace-cmd record -e error_env_tlob -e detail_env_tlob &

      # ... run workload ...
      trace-cmd report

Yeah sounds good, also pointing out to enable the monitor. We might think of a general way to do this kind of thing in tools/rv, although detail_env_tlob is non-standard.

Quoted 34 lines

 > Is kernel code going to use this API? RV monitors are meant to be > enabled by userspace. What's the use-case here?

Agreed.  The uprobe interface is driven from userspace; tlob_start_task() and tlob_stop_task() are the internal implementation functions it calls, not a public API for external kernel modules.  The hypothetical kernel-module use case would be removed from the documentation; the kernel-doc block is retained for code maintainers.

 > That's probably a bit too detailed for this page. If you really want > this information somewhere couldn't it stay in the code?

Agreed; moved to comments in handle_sched_switch() and handle_sched_wakeup().  The "Limitations" subsection is retained.

-- Patch 09: KUnit tests

 > What caught my eyes are tests enrolling tracepoints handlers. If you > go there you're no longer doing unit testing, what's the advantage of > testing the entire monitor here over doing that in selftests?

Agreed.  The three suites that register tracepoint handlers or create kthreads (tlob_sched_integration, tlob_trace_output, tlob_violation_react) have been removed from KUnit and will be added to selftests in v3.

Two pure unit test suites remain in KUnit:

   tlob_task_api:
     Tests tlob_start_task / tlob_stop_task return values (-ENODEV, -EALREADY, -ESRCH, -EOVERFLOW, -ENOSPC, -ERANGE) via direct calls (these functions are the internal implementation used by both the uprobe and, in future, the ioctl interface).
     No tracepoints, no scheduling.

Quoted 8 lines

   tlob_uprobe_format:
     Tests the uprobe line parser (tlob_parse_uprobe_line, tlob_parse_remove_line) against valid and invalid input strings. Pure string parsing; no scheduling, no tracepoints.

This also resolves the tristate-vs-bool issue: with only pure unit tests there is no dependency on sched_setscheduler_nocheck, so bool is correct.

Yeah looks good.

Quoted 15 lines

--  Patch 10: selftests

--- PREEMPT_RT RCU stall ---

 > I run it on a VM and have it hanging at step 9 [...] rcu_preempt stall. > Did you see that? Am I doing something wrong?

Thanks for reporting.  The patch changed ha_monitor.h from HRTIMER_MODE_REL_HARD (the existing upstream value) to REL_SOFT; the stall appeared on PREEMPT_RT after that change.  We have not fully confirmed whether REL_SOFT is the root cause — REL_SOFT defers the callback to the ktimers kthread, which could starve rcu_preempt under certain PREEMPT_RT configurations, but other factors may be involved.

Quoted 8 lines

We plan to revert to HRTIMER_MODE_REL_HARD at both sites in ha_monitor.h as the conservative choice:

   ha_setup_timer():     HRTIMER_MODE_REL_SOFT -> HRTIMER_MODE_REL_HARD ha_start_timer_ns():  HRTIMER_MODE_REL_SOFT -> HRTIMER_MODE_REL_HARD

Do you have more insight into the stall, or does REL_HARD resolve it on your setup?

Right, good point, any specific reason why you wanted REL_SOFT?

I indeed always test under PREEMPT_RT but I still see the same splat also after reverting REL_HARD..
Could you reproduce it on your setup?

My config is nothing special: what vng gives you adding PREEMPT_RT/RCU_PREEMPT and lockdep (PROVE_LOCKING/PROVE_RCU).

Quoted 7 lines

--- Selftest structure ---

 > This should be tested together with the other monitors (enable/disable), > we could at most expand those with the check_requires.
 > Let's focus on tlob-only features in this patch.

Quoted 8 lines

Agreed.  In v3 we plan to drop tracefs.tc (covered by the generic rv_monitor_enable_disable.tc) and keep only the six uprobe-specific test cases under test.d/tlob/

ioctl.tc is deferred with the ioctl interface to the follow-up series. The KUnit integration tests (sched_switch accounting, budget-expiry tracepoint) would be moved to selftests as additional test cases.

Thanks,
Gabriele

Hi Gabriele,

No specific reason for REL_SOFT — not intentional, reverting to REL_HARD.

Reproduced the stall on the same config (PREEMPT_RT + PROVE_LOCKING/PROVE_RCU).

Root cause is a cleanup ordering bug in uprobe_detail_waiting.tc, unrelated to REL_SOFT/REL_HARD:

   # original cleanup — wrong order 

   echo "-${UPROBE_TARGET}:${start_offset}" > "$TLOB_MONITOR"  # (A)

   kill "$hog_pid"                                              # (B)

                                                              (A) triggers synchronize_srcu() in the kernel. But tlob_target is stuck mid-uprobe_notify_resume holding an SRCU read lock, preempted by the FIFO-99 hog -> so the reader never finishes and (B) is never reached. rcuc/0 (a kthread on PREEMPT_RT) is also starved by the hog -> RCU stall.

   Fix: kill the hog first:

   kill "$hog_pid"; wait "$hog_pid"

   echo "-${UPROBE_TARGET}:${start_offset}" > "$TLOB_MONITOR"

On the PREEMPT_RT it is more reliably triggered there because rcuc/0 runs as a preemptible kthread rather than in softirq, making it easier for the hog to monopolise the CPU long enough to hit the stall.

Thank you for the thorough review and valuable suggestions. We are working through all of them and running the full test suite. We expect to post v3 within the next two days.

From: Wen Yang <[email protected]>

rvgen rejects a state invariant when its env is never reset on any state-transition edge.  This prevents expressing monitors where a clock tracks the full monitoring window — reset once at object creation, active in all states.

Allow reset() annotations on the __init_STATE -> STATE arrow. automata.py adds listed envs to the new env_init_started set (and to env_stored so the HA framework allocates per-object storage).  dot2k.py uses env_init_started for three purposes:

- Generate a handle_monitor_start() skeleton that resets the env and arms the timer after the caller sets up DA storage and initial state.

- Guard ha_inv_to_guard calls with !ha_monitor_env_invalid() for these
  envs: a concurrent DA event between da_handle_start_event() and ha_reset_env() would otherwise store U64_MAX - BUDGET as the guard anchor, silently disabling enforcement.

- Always generate ha_verify_guards() for monitors with invariants, providing a stable extension point for future per-event guards.

Models without __init resets (e.g. stall.dot) are unaffected.

Signed-off-by Wen Yang <[email protected]>

---
 tools/verification/rvgen/rvgen/automata.py |  26 ++++++ tools/verification/rvgen/rvgen/dot2k.py    | 100 +++++++++++++++++++--

 2 files changed, 119 insertions(+), 7 deletions(-)

tools/verification/rvgen/rvgen/automata.py +0 -0

index b9f8149f7118..178a1a4ffd8a 100644
@@ -69,15 +69,41 @@ class Automata:

         self.states, self.initial_state, self.final_states = self.__get_state_variables()
         self.env_types = {}
         self.env_stored = set()
+        self.env_init_started = set()
         self.constraint_vars = set()
         self.self_loop_reset_events = set()
         self.events, self.envs = self.__get_event_variables()
+        self.__parse_init_resets()
         self.function, self.constraints = self.__create_matrix() self.events_start, self.events_start_run = self.__store_init_events() self.env_stored = sorted(self.env_stored)
+        self.env_init_started = sorted(self.env_init_started) self.constraint_vars = sorted(self.constraint_vars) self.self_loop_reset_events = sorted(self.self_loop_reset_events)
+    def __parse_init_resets(self) -> None:
+        """Parse reset() annotations on the __init_STATE -> STATE arrow. +
+        Adds each listed env to env_stored (HA framework allocates per-object +        storage) and env_init_started (ha2k generates handle_monitor_start()).
+        """
+        init_prefix = f'"{self.init_marker}'
+        for line in map(str.lstrip, self.__dot_lines):
+            if not line.startswith(init_prefix):
+                continue
+            split_line = line.split()
+            if len(split_line) < 3 or split_line[1] != "->":
+                continue
+            if "label" not in line:
+                continue
+            label = "".join(split_line[split_line.index("label") + 2:- 1]).replace('"', '')
+            for part in label.split(";"):
+                reset_m = self.constraint_reset.search(part.strip()) +                if reset_m:
+                    env = reset_m["env"]
+                    self.env_stored.add(env)
+                    self.env_init_started.add(env)
+
     def __get_model_name(self) -> str:
         basename = ntpath.basename(self.__dot_path)
         if not basename.endswith(".dot") and not basename.endswith(".gv"):

tools/verification/rvgen/rvgen/dot2k.py +0 -0

index e6f476b903b0..e8066260c0af 100644
@@ -366,7 +366,18 @@ f"""static inline void ha_convert_inv_guard(struct
 ha_monitor *ha_mon,

             conf_g = [e for s, e in conflict_guards if s == state] if not conf_i and not conf_g:
                 continue
-            buff.append(f"\t{_else}if (curr_state == {self.states[state]}{self.enum_suffix})")
+
+            state_name = f"{self.states[state]}{self.enum_suffix}" +            env_full = self.__get_constraint_env(constr)
+            env_bare = env_full[:-len(self.enum_suffix)]
+            if env_bare in self.env_init_started:
+                # env_store is ENV_INVALID_VALUE until handle_monitor_start();
+                # skip ha_inv_to_guard during the init race window. +                cont = "\t\t " if _else else "\t    "
+                buff.append(f"\t{_else}if (curr_state == {state_name} &&") +                buff.append(f"{cont}!ha_monitor_env_invalid(ha_mon, {env_full}))")
+            else:
+                buff.append(f"\t{_else}if (curr_state == {state_name})")
             buff.append(f"\t\t{self.__start_to_conv(constr)};") _else = "else "

tools/verification/rvgen/rvgen/dot2k.py +0 -0

@@ -376,16 +387,22 @@ f"""static inline void ha_convert_inv_guard(struct
 ha_monitor *ha_mon,

     def __fill_verify_guards_func(self) -> list[str]:
         buff = []
-        if not self.guards:
+        # Always generate for monitors with invariants: stable extension +        # point for future guard conditions.
+        if not self.guards and not self.invariants:
             return []

         buff.append(
 f"""static inline bool ha_verify_guards(struct ha_monitor *ha_mon, \t\t\t\t    enum {self.enum_states_def} curr_state, enum {self.enum_events_def} event,
 \t\t\t\t    enum {self.enum_states_def} next_state, u64 time_ns)

tools/verification/rvgen/rvgen/dot2k.py +7 -3

-{{
-\tbool res = true;
-""")
+{{""")
+
+        if not self.guards:
+            buff.append("\treturn true;\n}\n")
+            return buff
+
+        buff.append("\tbool res = true;\n")

         _else = ""
         for edge, constr in sorted(self.guards.items()):

tools/verification/rvgen/rvgen/dot2k.py +0 -0

@@ -522,7 +539,7 @@ f"""static bool ha_verify_constraint(struct ha_monitor
 *ha_mon,

             buff.append("\tha_convert_inv_guard(ha_mon, curr_state, event, " "next_state, time_ns);\n")

-        if self.guards:
+        if self.guards or self.invariants:
             buff.append("\tif (!ha_verify_guards(ha_mon, curr_state, event, " "next_state, time_ns))\n\t\treturn false;\n")

tools/verification/rvgen/rvgen/dot2k.py +0 -0

@@ -599,8 +616,77 @@ f"""static bool ha_verify_constraint(struct ha_monitor
 *ha_mon,
                  buff.append("}\n")

         return buff

+    def __fill_init_start_helper(self) -> list[str]:
+        """Generate handle_monitor_start() for envs reset on the __init arrow.

tools/verification/rvgen/rvgen/dot2k.py +30 -0

+
+        env_store is invalid inside da_handle_start_event(); this helper must
+        be called after DA storage is allocated and initial state is set.
+        """
+        if not self.env_init_started:
+            return []
+
+        # Collect the ha_start_timer call for each init-started env from the
+        # first state invariant that references it.
+        timer_calls: dict[str, str] = {}
+        for env in self.env_init_started:
+            env_full = f"{env}{self.enum_suffix}"
+            for constr in self.invariants.values():
+                if env_full in constr:
+                    timer_calls[env] = constr
+                    break
+
+        buff = []
+        buff.append(
+"""/*
+ * handle_monitor_start - reset per-object clock(s) and arm the timer.
+ *
+ * env_store is invalid inside da_handle_start_event(); call this helper
+ * after allocating DA storage and setting the initial DA state.
+ *
+ * XXX: replace the placeholders with the actual logic for your monitor.
+ */""")
+
+        if self.monitor_type == "per_obj":
+            buff.append("static int handle_monitor_start(int id,

monitor_target t)")
+            buff.append("{")
+            buff.append("\tstruct ha_monitor *ha_mon;")
+            buff.append("\tu64 time_ns = ktime_get_ns();\n")
+            buff.append("\t/* XXX: allocate DA storage, e.g. da_create_or_get(id, t) */")
+            buff.append("\t/* XXX: set initial DA state, e.g. da_handle_start_event(id, t, <event>) */")
+            buff.append("\tha_mon = /* XXX: retrieve ha_monitor for (id, t) */;")
+        elif self.monitor_type == "per_task":
+            buff.append("static int handle_monitor_start(struct task_struct *p)")
+            buff.append("{")
+            buff.append("\tstruct ha_monitor *ha_mon;")
+            buff.append("\tu64 time_ns = ktime_get_ns();\n")
+            buff.append("\t/* XXX: allocate DA storage, e.g. da_create_or_get(p->pid, p) */")
+            buff.append("\t/* XXX: set initial DA state, e.g. da_handle_start_event(p->pid, p, <event>) */")
+            buff.append("\tha_mon = /* XXX: retrieve ha_monitor for p */;") +        else:
+            buff.append("static int handle_monitor_start(void)") +            buff.append("{")
+            buff.append("\tstruct ha_monitor *ha_mon;")
+            buff.append("\tu64 time_ns = ktime_get_ns();\n")
+            buff.append("\tha_mon = /* XXX: retrieve global ha_monitor */;")

tools/verification/rvgen/rvgen/dot2k.py +6 -0

+
+        buff.append("\tif (!ha_mon)")
+        buff.append("\t\treturn -ENOENT;")
+
+        for env in self.env_init_started:
+            buff.append(f"\tha_reset_env(ha_mon, {env}{self.enum_suffix},

time_ns);")
+            if env in timer_calls:
+                buff.append(f"\t{timer_calls[env]};") +            else:
+                buff.append(f"\t/* XXX: arm timer for {env} */")

tools/verification/rvgen/rvgen/dot2k.py +5 -0

+
+        buff.append("\treturn 0;")
+        buff.append("}\n")
+        return buff
+

     def _fill_hybrid_definitions(self) -> list[str]:
-        return self.__fill_hybrid_get_reset_functions() + self.__fill_constr_func()
+        return (self.__fill_hybrid_get_reset_functions() + +                self.__fill_init_start_helper() + +                self.__fill_constr_func())

     def _fill_timer_type(self) -> list:
         if self.invariants: