Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221208075604.811710-1-junxiao.chang@intel.com> <BN9PR11MB5370BA8A506EB8519DC879C1ECEA9@BN9PR11MB5370.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB5370BA8A506EB8519DC879C1ECEA9@BN9PR11MB5370.namprd11.prod.outlook.com>
From:   Alison Chaiken <achaiken@aurora.tech>
Date:   Tue, 20 Dec 2022 09:10:45 -0800
Message-ID: <CAFzL-7t4AxR-8j6_JP-MQPOqw-tjq6Ds5MTgy4bgRUCK7Ga8bQ@mail.gmail.com>
Subject: Re: [PATCH] softirq: wake up ktimer thread in softirq context
To:     "Chang, Junxiao" <junxiao.chang@intel.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-rt-users@vger.kernel.org" <linux-rt-users@vger.kernel.org>,
        "bigeasy@linutronix.de" <bigeasy@linutronix.de>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "Peh, Hock Zhang" <hock.zhang.peh@intel.com>,
        Glenn Elliott <glenn@aurora.tech>,
        Rami Jaber <rjaber@aurora.tech>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 20, 2022 at 2:44 AM Chang, Junxiao <junxiao.chang@intel.com> wr=
ote:
> Any comment? This patch is for 6.1-rt, issue could be reproduced with 5.1=
9-rt kernel as well.

In

https://lore.kernel.org/linux-rt-users/CAFzL-7v-NSFKAsyhxReEES7bMomSTwYyrZs=
cnjbkydEP3CTXmQ@mail.gmail.com/

we reported an occasional problem with an x86 system entering a deep C
state even though timers were pending.   Perhaps your patch would
prevent this transition.

> This issue is easier to reproduced when there is heavy network workload w=
hich introduces a lot of softirq events. If hrtimer interrupt is triggered =
in softirq context, with current RT kernel, it will not wake up ktimers thr=
ead which handles hrtimer event because in function __irq_exit_rcu, "in_int=
errupt()" is true:
>
> static inline void __irq_exit_rcu(void)
> {
> ...
>         preempt_count_sub(HARDIRQ_OFFSET);
>         if (!in_interrupt()) {
>                 if (local_softirq_pending())
>                         invoke_softirq();
>
>                 if (IS_ENABLED(CONFIG_PREEMPT_RT) && local_pending_timers=
())
>                         wake_timersd();
>         }
> ...
> }
>

Isn't removing the check for IS_ENABLED(CONFIG_PREEMPT_RT) inadvisable?

> Then ktimers threads stays in sleep state, hrtimer function will not be c=
alled although hrtimer interrupt has been triggered. Ktimers thread might b=
e woken up in next timer interrupt which introduces long delay.
>
> Any comments are welcome.
>
> Regards,
> Junxiao

-- Alison Chaiken
Aurora Innovation


 -----Original Message-----
> From: Chang, Junxiao <junxiao.chang@intel.com>
> Sent: Thursday, December 8, 2022 3:56 PM
> To: linux-kernel@vger.kernel.org
> Cc: linux-rt-users@vger.kernel.org; bigeasy@linutronix.de; tglx@linutroni=
x.de; rostedt@goodmis.org; Chang, Junxiao <junxiao.chang@intel.com>; Peh, H=
ock Zhang <hock.zhang.peh@intel.com>
> Subject: [PATCH] softirq: wake up ktimer thread in softirq context
>
> Occiaionally timer interrupt might be triggered in softirq context, ktime=
r thread should be woken up with RT kernel, or else ktimer thread might sta=
y in sleep state although timer interrupt has been triggered.
>
> This change fixes a latency issue that timer handler is delayed for more =
than 4ms in network related test.
>
> Fixes: 2165d27554e8 ("softirq: Use a dedicated thread for timer wakeups."=
)
> Reported-by: Peh, Hock Zhang <hock.zhang.peh@intel.com>
> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
> ---
>  kernel/softirq.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c index ab1fe34326bab..34a=
e39e4a3d10 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -664,13 +664,10 @@ static inline void __irq_exit_rcu(void)  #endif
>         account_hardirq_exit(current);
>         preempt_count_sub(HARDIRQ_OFFSET);
> -       if (!in_interrupt()) {
> -               if (local_softirq_pending())
> -                       invoke_softirq();
> -
> -               if (IS_ENABLED(CONFIG_PREEMPT_RT) && local_pending_timers=
())
> -                       wake_timersd();
> -       }
> +       if (!in_interrupt() && local_softirq_pending())
> +               invoke_softirq();
> +       if (!(in_nmi() || in_hardirq()) && local_pending_timers())
> +               wake_timersd();
>
>         tick_irq_exit();
>  }
> --
> 2.25.1
>