Messages in this thread | | | Date | Fri, 21 Jul 2023 18:06:07 +0200 | Subject | Re: [PATCH] tracing: Have all levels of checks prevent recursion | From | Alexander Lobakin <> |
| |
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 <aleksander.lobakin@intel.com> wrote:
[...]
>>> + 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. > >> 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. > >> >> 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 > > >> >>> + >>> + return level; >>> +} >>> + >> [0] >> https://lore.kernel.org/netdev/b3884ff9-d903-948d-797a-1830a39b1e71@intel.com >> >> Thanks, >> Olek >
Thanks, Olek
| |