Messages in this thread | | | Subject | Re: [PATCH v3 2/2] timers: Don't search for expired timers while TIMER_SOFTIRQ is scheduled | From | Haris Okanovic <> | Date | Fri, 5 Jan 2018 13:37:45 -0600 |
| |
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@ni.com> > --- > [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); > } > >
| |