Re: [PATCH 1/1] softirq: Use a dedicated thread for timer wakeups on PREEMPT_RT.

[Frederic Weisbecker <frederic@kernel.org>]

Le Tue, Oct 22, 2024 at 05:34:21PM +0200, Sebastian Andrzej Siewior a écrit :
> On 2024-10-22 15:28:56 [+0200], Frederic Weisbecker wrote:
> > > Once the ksoftirqd is marked as pending (or is running) it will collect
> > > all raised softirqs. This in turn means that a softirq which would have
> > > been processed at the end of the threaded interrupt, which runs at an
> > > elevated priority, is now moved to ksoftirqd which runs at SCHED_OTHER
> > > priority and competes with every regular task for CPU resources.
> > 
> > But for ksoftirqd to collect other non-timers softirqs, those vectors must
> > have been raised before from a threaded interrupt, right? So why those
> > vectors didn't get a chance to execute at the end of that threaded interrupt?
> 
> This statement is no longer accurate since
> 	d15121be74856 ("Revert "softirq: Let ksoftirqd do its job"")
> 
> So the "collect all" part is no longer.
> 
> > OTOH one problem I can imagine is a threaded interrupt preempting ksoftirqd
> > which must wait for ksoftirqd to complete due to the local_bh_disable()
> > in the beginning of irq_forced_thread_fn(). But then isn't there some
> > PI involved on the local lock?
> 
> Yes, there is PI involved on the local lock. So this will happen.
> 
> > Sorry I'm probably missing something...
> 
> Try again without the "ksoftirqd will collect it all" since this won't
> happen since the revert I mentioned.

I still don't get it, this makes:

"""
Once the ksoftirqd is marked as pending (or is running), a softirq which
would have been processed at the end of the threaded interrupt, which runs
at an elevated priority, is now moved to ksoftirqd which runs at SCHED_OTHER
priority and competes with every regular task for CPU resources.
"""

ksoftirqd raised for timers still doesn't prevent a threaded IRQ from running
softirqs, unless it preempts ksoftirqd and waits with PI. So is it what you're
trying to solve?

Or is the problem that timer softirqs are executed with SCHED_NORMAL?

> 
> > > This introduces long delays on heavy loaded systems and is not desired
> > > especially if the system is not overloaded by the softirqs.
> > > 
> > > Split the TIMER_SOFTIRQ and HRTIMER_SOFTIRQ processing into a dedicated
> > > timers thread and let it run at the lowest SCHED_FIFO priority.
> > > Wake-ups for RT tasks happen from hardirq context so only timer_list timers
> > > and hrtimers for "regular" tasks are processed here.
> > 
> > That last sentence confuses me. How are timers for non regular task processed
> > elsewhere? Ah you mean RT tasks rely on hard hrtimers?
> 
> Correct. A clock_nanosleep() for a RT task will result in wake_up() from
> hardirq. A clock_nanosleep() for a !RT task will result in wake_up()
> from ksoftirqd or now the suggested ktimersd.

Oh I see now, that's all in __hrtimer_init_sleeper().

> 
> Quick question: Do we want this in forced-threaded mode, too? The timer
> (timer_list timer) and all HRTIMER_MODE_SOFT are handled in ksoftirqd.

It's hard to tell for me as I don't know the !RT usecases for forced-threaded mode.
"If in doubt say N" ;-)

> > > +#ifdef CONFIG_PREEMPT_RT
> > > +static void timersd_setup(unsigned int cpu)
> > > +{
> > 
> > That also needs a comment.
> 
> Why we want the priority I guess.

Yes.

> 
> …
> > > +void raise_hrtimer_softirq(void)
> > > +{
> > > +	raise_ktimers_thread(HRTIMER_SOFTIRQ);
> > > +}
> > > +
> > > +void raise_timer_softirq(void)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	local_irq_save(flags);
> > > +	raise_ktimers_thread(TIMER_SOFTIRQ);
> > > +	wake_timersd();
> > 
> > This is supposed to be called from hardirq only, right?
> > Can't irq_exit_rcu() take care of it? Why is it different
> > from HRTIMER_SOFTIRQ ?
> 
> Good question. This shouldn't be any different compared to the hrtimer
> case. This is only raised in hardirq, so yes, the irq_save can go away
> and the wake call, too.

Cool. You can add lockdep_assert_in_irq() within raise_ktimers_thread() for
some well deserved relief :-)

Thanks.

> 
> > Thanks.
> 
> Sebastian