Re: [PATCH v8 20/22] rv: Add rtapp_sleep monitor

[Gabriele Monaco <gmonaco@redhat.com>]

On Mon, 2025-05-19 at 09:04 +0200, Nam Cao wrote:
> On Fri, May 16, 2025 at 04:31:03PM +0000, Gabriele Monaco wrote:
> > 2025-05-12T10:56:30Z Nam Cao <namcao@linutronix.de>:
> > > diff --git a/kernel/trace/rv/monitors/sleep/Kconfig
> > > b/kernel/trace/rv/monitors/sleep/Kconfig
> > > new file mode 100644
> > > index 000000000000..d00aa1aae069
> > > --- /dev/null
> > > +++ b/kernel/trace/rv/monitors/sleep/Kconfig
> > > @@ -0,0 +1,13 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +config RV_MON_SLEEP
> > > +   depends on RV
> > > +   select RV_LTL_MONITOR
> > > +   depends on HAVE_SYSCALL_TRACEPOINTS
> > > +   depends on RV_MON_RTAPP
> > > +   select TRACE_IRQFLAGS
> > 
> > I had a different approach towards those (the preemptirq
> > tracepoints)
> > under the assumption adding them introduces latency. Besides me
> > picking
> > the wrong config (I used IRQSOFF, I'll fix that) I considered the
> > monitor
> > should /depend/ on the tracepoint instead of select it.
> > 
> > This way it looks easier to me to avoid making a change that
> > introduces
> > latency slip in when distribution maintainers enable the monitor
> > (e.g.
> > TRACE_IRQFLAGS may be enabled on debug kernels and using depends
> > would
> > automatically prevent the monitor on non-debug kernels).
> > 
> > Now is this concern justified? Is it only a performance issue for
> > the
> > preempt tracepoint or not even there?  I'd like to keep consistency
> > but I
> > really can't decide on which approach is better.
> 
> Both approach is fine, I don't have a strong preference.
> 
> I doubt that the distribution people would carelessly enable anything
> new,
> and these monitors are disabled by default. So I wouldn't worry too
> much.
> 

Yeah that's true, I still see dependency makes their life mildly
easier, but as long as it's clear this type of monitor can affect
performance, both solutions work.

> I will do some measurements on the runtime impact of having these
> monitors
> built, so that there will be a recommendation whether to enable them
> in
> distribution kernel. But for now, just like any other debug configs,
> people
> should expect some performance hit.
> 

Fair enough. We did some tests internally showing noticeable latency
increases with /both/ preempt and irq tracepoints enabled but didn't
perform tests with the irq one alone.
Nevertheless, I'd say a note saying enabling compilation of the monitor
may affect performance even when the monitor is off would do the job
(or anything along the line as you see fit).
Currently RV should not really affect the system when compiled in but
disabled, so I'd make it clear when that happens.

> > Also curiosity on my side (I didn't try), you require
> > TRACE_IRQFLAGS to
> > use hardirq_context but how different is it from in_hardirq() in
> > your
> > case?
> 
> There is a wake_timersd() in __irq_exit_rcu(). This is a wakeup
> performed
> within interrupt handling, but in_hardirq() doesn't say that.
> 

Alright, got it.

Thanks,
Gabriele