Re: [RFC PATCH 1/9] tracing/irq: Add generic IRQ context boundary events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] softirq: tick_irq_exit() called before trace_hardirq_context_exit() handoff
--

commit 2beb0d9dac0a7b76bfe5b73b550fff3be238bbab
Author: Karl Mehltretter <kmehltretter@gmail.com>

tracing/irq: Add generic IRQ context boundary events

This commit adds tracepoints at the generic hardirq and serving-softirq
context boundaries. It also updates softirq pending snapshots and wakes
worker threads when callbacks attached to boundary events raise new work.

> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index be49e6469f3ad..2e3b6fd3f6d5d 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c

[ ... ]

> @@ -722,6 +770,7 @@ void irq_enter_rcu(void)
>  		tick_irq_enter();
>  
>  	account_hardirq_enter(current);
> +	trace_hardirq_context_entry(hardirq_count() >> HARDIRQ_SHIFT);
>  }
>  
>  /**
> @@ -796,6 +845,36 @@ static inline void __irq_exit_rcu(void)
>  		wake_timersd();
>  
>  	tick_irq_exit();
> +	if (trace_hardirq_context_exit_enabled()) {
> +		bool ksoftirqd_wake_requested = false;
> +		bool timersd_wake_requested = false;
> +
> +		trace_hardirq_context_exit(hardirq_count() >> HARDIRQ_SHIFT);
> +
> +		/* Hand work raised by the final boundary event to its worker. */
> +		if (hardirq_count() == HARDIRQ_OFFSET) {
> +			if (irq_count() == HARDIRQ_OFFSET &&
> +			    unlikely(local_softirq_pending()) &&
> +			    should_wake_ksoftirqd()) {
> +				wakeup_softirqd();
> +				ksoftirqd_wake_requested = true;
> +			}
> +
> +			if (IS_ENABLED(CONFIG_IRQ_FORCED_THREADING) &&
> +			    force_irqthreads() && local_timers_pending_force_th()) {
> +				wake_timersd();
> +				timersd_wake_requested = true;
> +			}
> +
> +			/* The timersd wakeup can itself raise a softirq. */
> +			if (timersd_wake_requested &&
> +			    !ksoftirqd_wake_requested &&
> +			    irq_count() == HARDIRQ_OFFSET &&
> +			    unlikely(local_softirq_pending()) &&
> +			    should_wake_ksoftirqd())
> +				wakeup_softirqd();
> +		}
> +	}
>  	preempt_count_sub(HARDIRQ_OFFSET);
>  }

[Severity: Medium]
Should tick_irq_exit() be called after the trace_hardirq_context_exit()
block rather than before it?

In irq_enter_rcu(), tick_irq_enter() runs before
trace_hardirq_context_entry() so that the nohz clock wakeup and idle
cputime accounting state are established before the entry event fires.

In __irq_exit_rcu(), however, tick_irq_exit() runs before
trace_hardirq_context_exit() and the subsequent wakeup_softirqd() and
wake_timersd() calls:

kernel/softirq.c:tick_irq_exit() {
	if ((sched_core_idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
		if (hardirq_count() == HARDIRQ_OFFSET)
			tick_nohz_irq_exit();
	}
}

kernel/time/tick-sched.c:tick_nohz_irq_exit() {
	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);

	if (tick_sched_flag_test(ts, TS_FLAG_INIDLE)) {
		tick_nohz_clock_sleep(ts);
		ts->idle_entrytime = ktime_get();
		if (tick_sched_flag_test(ts, TS_FLAG_STOPPED))
			kcpustat_irq_exit(ts->idle_entrytime);
	} else {
		tick_nohz_full_update_tick(ts);
	}
}

On an idle cpu where need_resched() is false before the tracepoint,
tick_irq_exit() calls tick_nohz_irq_exit(), which places sched_clock and
kcpustat back into idle sleep via tick_nohz_clock_sleep() and
kcpustat_irq_exit(). If the hardirq_context_exit callback then raises work
and wakes ksoftirqd or ktimerd, try_to_wake_up() reads sched_clock_cpu()
and sets need_resched after the idle clock and cputime state have already
gone to sleep.

Similarly, on a nohz_full cpu, tick_nohz_irq_exit() calls
tick_nohz_full_update_tick() to evaluate and potentially stop the tick
before trace_hardirq_context_exit() runs. Any timer, softirq, rcu, or
irq_work state raised by the exit callback is missed by that evaluation,
or forces an extra self-ipi when waking ksoftirqd after the tick was just
stopped.

Would moving tick_irq_exit() to just after the
trace_hardirq_context_exit_enabled() block (while hardirq_count() is still
equal to the outermost offset) avoid this regression and keep entry and
exit symmetric?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022050712.75035-1-kmehltretter@gmail.com?part=1