Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco <gmonaco@redhat.com>]

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:
> > On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:
> > > +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

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