Re: [PATCH v3 2/2] timers: Don't search for expired timers while TIMER_SOFTIRQ is scheduled

[Haris Okanovic]

*bump* Has anyone looked into this?

On 01/05/2018 01:37 PM, Haris Okanovic wrote:
> It looks like an old version of this patch is included in v4.9*-rt* 
> kernels -- E.g. commit 032f93ca in v4.9.68-rt60. There's nothing 
> functionally wrong with the included version to the best of my 
> knowledge. However, I posted a newer V3 [1][2] based on Thomas' feedback 
> that's substantially cleaner and likely more efficient (haven't measured 
> yet). I think we should include the latter version instead, if only for 
> the cosmetic benefits. Thoughts?
>
> [1] https://patchwork.kernel.org/patch/9879825/Â; [PATCH v3,1/2]
> [2] https://patchwork.kernel.org/patch/9879827/Â; [PATCH v3,2/2]
>
> -- Haris
>
>
> On 08/03/2017 04:06 PM, Haris Okanovic wrote:
> > This change avoid needlessly searching for more timers in
> > run_local_timers() (hard interrupt context) when they can't fire.
> > For example, when ktimersoftd/run_timer_softirq() is scheduled but
> > preempted due to cpu contention. When it runs, run_timer_softirq() will
> > discover newly expired timers up to current jiffies in addition to
> > firing previously expired timers.
> >
> > However, this change also adds an edge case where non-hrtimer firing
> > is sometimes delayed by an additional tick. This is acceptable since we
> > don't make latency guarantees for non-hrtimers and would prefer to
> > minimize hard interrupt time instead.
> >
> > Signed-off-by: Haris Okanovic <haris.okanovic@xxxxxx>
> > ---
> > [PATCH v3]
> > Â - Split block_softirq into separate commit
> >
> > https://github.com/harisokanovic/linux/tree/dev/hokanovi/timer-peek-v5
> > ---
> > Â kernel/time/timer.c | 21 +++++++++++++++++++--
> > Â 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index 078027d8a866..f0ef9675abdf 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -208,6 +208,7 @@ struct timer_base {
> > ÂÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ migration_enabled;
> > ÂÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ nohz_active;
> > ÂÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ is_idle;
> > +ÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ block_softirq;
> > ÂÂÂÂÂ DECLARE_BITMAP(pending_map, WHEEL_SIZE);
> > ÂÂÂÂÂ struct hlist_headÂÂÂ vectors[WHEEL_SIZE];
> > ÂÂÂÂÂ struct hlist_headÂÂÂ expired_lists[LVL_DEPTH];
> > @@ -1376,9 +1377,11 @@ static int __collect_expired_timers(struct 
> > timer_base *base)
> > ÂÂÂÂÂ /*
> > ÂÂÂÂÂÂ * expire_timers() must be called at least once before we can
> > -ÂÂÂÂ * collect more timers.
> > +ÂÂÂÂ * collect more timers. We should never hit this case unless
> > +ÂÂÂÂ * TIMER_SOFTIRQ got raised without expired timers.
> > ÂÂÂÂÂÂ */
> > -ÂÂÂ if (base->expired_levels)
> > +ÂÂÂ if (WARN_ONCE(base->expired_levels,
> > +ÂÂÂÂÂÂÂÂÂÂÂ "Must expire collected timers before collecting more"))
> > ÂÂÂÂÂÂÂÂÂ return base->expired_levels;
> > ÂÂÂÂÂ clk = base->clk;
> > @@ -1702,6 +1705,9 @@ static __latent_entropy void 
> > run_timer_softirq(struct softirq_action *h)
> > ÂÂÂÂÂ __run_timers(base);
> > ÂÂÂÂÂ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active)
> > ÂÂÂÂÂÂÂÂÂ __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
> > +
> > +ÂÂÂ /* Allow new TIMER_SOFTIRQs to get scheduled by 
> > run_local_timers() */
> > +ÂÂÂ base->block_softirq = false;
> > Â }
> > Â /*
> > @@ -1712,6 +1718,14 @@ void run_local_timers(void)
> > ÂÂÂÂÂ struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
> > ÂÂÂÂÂ hrtimer_run_queues();
> > +
> > +ÂÂÂ /*
> > +ÂÂÂÂ * Skip if TIMER_SOFTIRQ is already running on this CPU, since it
> > +ÂÂÂÂ * will find and expire all timers up to current jiffies.
> > +ÂÂÂÂ */
> > +ÂÂÂ if (base->block_softirq)
> > +ÂÂÂÂÂÂÂ return;
> > +
> > ÂÂÂÂÂ /* Raise the softirq only if required. */
> > ÂÂÂÂÂ if (time_before(jiffies, base->clk) || !tick_find_expired(base)) {
> > ÂÂÂÂÂÂÂÂÂ if (!IS_ENABLED(CONFIG_NO_HZ_COMMON) || !base->nohz_active)
> > @@ -1720,7 +1734,10 @@ void run_local_timers(void)
> > ÂÂÂÂÂÂÂÂÂ base++;
> > ÂÂÂÂÂÂÂÂÂ if (time_before(jiffies, base->clk) || 
> > !tick_find_expired(base))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ return;
> > +ÂÂÂÂÂÂÂ base--;
> > ÂÂÂÂÂ }
> > +
> > +ÂÂÂ base->block_softirq = true;
> > ÂÂÂÂÂ raise_softirq(TIMER_SOFTIRQ);
> > Â }