Re: [PATCH] tracing: Have all levels of checks prevent recursion

From: Steven Rostedt <rostedt@goodmis.org>
Date: Fri, 21 Jul 2023 12:00:40 -0400

On Fri, 21 Jul 2023 17:34:41 +0200
Alexander Lobakin [off-list ref] wrote:

[...]

quoted

quoted

+	level += !!(pc & (NMI_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET));  

This doesn't take into account that we can switch the context manually
via local_bh_disable() / local_irq_save() etc. During the testing of the

You cannot manually switch interrupt context.

quoted

separate issue[0], I've found that the function returns 1 in both just
softirq and softirq under local_irq_save().
Is this intended? Shouldn't that be

That is intended behavior.

local_bh_disable() and local_irq_save() is not a context switch. It is just
preventing that context from happening. The interrupt_context_level() is to
tell us what context we are running in, not what context is disabled.

quoted

	level += !!(pc & (NMI_MASK));
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK)) || irqs_disabled();
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET)) ||
		 in_atomic();

?
Otherwise, the result it returns is not really "context level".

local_bh_disable() use to (and perhaps still does in some configurations)
confuse things. But read the comment in kernel/softirq.c

/*
 * SOFTIRQ_OFFSET usage:
 *
 * On !RT kernels 'count' is the preempt counter, on RT kernels this applies
 * to a per CPU counter and to task::softirqs_disabled_cnt.
 *
 * - count is changed by SOFTIRQ_OFFSET on entering or leaving softirq
 *   processing.
 *
 * - count is changed by SOFTIRQ_DISABLE_OFFSET (= 2 * SOFTIRQ_OFFSET)
 *   on local_bh_disable or local_bh_enable.
 *
 * This lets us distinguish between whether we are currently processing
 * softirq and whether we just have bh disabled.
 */

Just because you disable interrupts does not mean you are in interrupt
context.

Ah okay, thanks! IOW, if we want to check in some code that we're
certainly have interrupts enabled and are not in the interrupt context,
we must always do

if (!(in_hardirq() || irqs_disabled()))

, nothing more elegant / already existing / ...?

-- Steve

quoted

quoted

+
+	return level;
+}
+  

[0]
https://lore.kernel.org/netdev/b3884ff9-d903-948d-797a-1830a39b1e71@intel.com (local)

Thanks,
Olek

Thanks,
Olek