Re: [PATCH] rv: Add signal reactor

On Mon, 2025-09-22 at 18:29 +0200, Nam Cao wrote:

On Fri, Sep 19, 2025 at 02:26:12PM +0200, Gabriele Monaco wrote:

quoted

On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:

quoted

+static void rv_reaction_signal(int signal, const char *fmt, va_list args)
+{
+	struct rv_signal_work *work;
+	char message[256];
+
+	work = mempool_alloc_preallocated(rv_signal_task_work_pool);
+	if (!work) {
+		pr_warn_ratelimited("Unable to signal through task_work,
sending directly\n");
+		vsnprintf(message, sizeof(message), fmt, args);
+		rv_signal_force_sig(signal, message);
+		return;
+	}

Why do you use the task_work at all instead of signalling directly?
If that's something not safe from a (any) tracepoint because it can sleep

If I remember correctly, sending signals requires a spinlock and therefore
may sleep on PREEMPT_RT.

Yeah that's what I quickly glanced at. Which seems to be the case also for
mempool_alloc_preallocated by the way, so I'm not sure that's safer than
signalling directly on PREEMPT_RT.

Thomas, did you test your reactor on PREEMPT_RT? I'd expect a few fat warnings
when this is called from sched tracepoints. Unless you're lucky and never get
contention. Lockdep (CONFIG_PROVE_LOCKING) may help here.

Thanks,
Gabriele

quoted

you should definitely not call it if allocation fails.

Yep.

We probably can get away with not reacting at all if allocation fails, by
crafting our tests such that only one reaction happens at a time, and
allocation won't fail.

Nam