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

[Nam Cao <namcao@linutronix.de>]

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.

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.

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

Best regards,
Nam