Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jiafei Pan <jiafei.pan@nxp.com>
To:     Steven Rostedt <rostedt@goodmis.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>
CC:     "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "romain.perier@gmail.com" <romain.perier@gmail.com>,
        "will@kernel.org" <will@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-rt-users@vger.kernel.org" <linux-rt-users@vger.kernel.org>,
        Leo Li <leoyang.li@nxp.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Jiafei Pan <jiafei.pan@nxp.com>
Subject: RE: [EXT] Re: [PATCH] softirq: add irq off checking for
 __raise_softirq_irqoff
Thread-Topic: [EXT] Re: [PATCH] softirq: add irq off checking for
 __raise_softirq_irqoff
Thread-Index: AQHWa6gdrPnfNrtYpEW7p3l8OUGNa6k1YJYQgADMBQCAAL3KYA==
Date:   Fri, 14 Aug 2020 02:21:25 +0000
Message-ID: <AM0PR04MB4772BDE0787FD18E8E6A48C28A400@AM0PR04MB4772.eurprd04.prod.outlook.com>
References: <20200806040729.39186-1-Jiafei.Pan@nxp.com>
        <AM0PR04MB4772B1B6D3391BAEAD4411808A430@AM0PR04MB4772.eurprd04.prod.outlook.com>
 <20200813105644.1eb6f2cd@oasis.local.home>
In-Reply-To: <20200813105644.1eb6f2cd@oasis.local.home>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: be2c1992-9268-44a9-0985-08d83ff8beb2
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Aug 2020 02:21:25.6303
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 20tLIU5P4Rrk/lcJkTEJjmmA6eLCyAMOsO+Bf2ADcT2SufA6xLdS8TODjZhl/PglrAR091PUYRuOTlVhn00rtA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0401MB2402
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On Thu, 13 Aug 2020 03:03:46 +0000
Jiafei Pan <jiafei.pan@nxp.com> wrote:

> Any comments? Thanks.
>
> @Steven Rostedt, I thinks irq off checking is necessary especially

> This is probably more for Thomas Gleixner.
Thanks Steven.
@Thomas Gleixner, would you please review the patch? thanks.
Jiafei.
> for Preempt-RT kernel, because some context may be changed from irq=20
> off to irq on when enable Preempt RT, I once met a issue that hrtimer=20
> soft irq is lost when enabled Preempt RT, finally I found=20
> napi_schedule_irqoff is called in hardware interrupt handler, there=20
> maybe no issue for non RT kernel, but for Preempt RT, interrupt is=20
> threaded, so irq is on in interrupt handler, the result is=20
> __raise_softirq_irqoff is called in irq on context, so that per-CPU=20
> softirq masking is corrupted because of the process of updating of=20
> soft irq masking is interrupted and not a atomic operation , and then=20
> caused hrtimer soft irq is lost. So I think adding irq status checking=20
> in __raise_softirq_irqoff can report such issue directly and help us=20
> to find the root cause of such issue.
>
> I know that there may be performance impaction to add extra checking=20
> here, if it is the case, how about to include it in some debug=20
> configuration items? Such as CONFIG_DEBUG_PREEMPT or other debug=20
> items?
>


> Best Regards,
> Jiafei.
>
> -----Original Message-----
> From: Jiafei Pan <Jiafei.Pan@nxp.com>
> Sent: Thursday, August 6, 2020 12:07 PM
> To: peterz@infradead.org; mingo@kernel.org; tglx@linutronix.de;=20
> rostedt@goodmis.org; romain.perier@gmail.com; will@kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org;=20
> Jiafei Pan <jiafei.pan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Vladimir=20
> Oltean <vladimir.oltean@nxp.com>; Jiafei Pan <jiafei.pan@nxp.com>
> Subject: [PATCH] softirq: add irq off checking for=20
> __raise_softirq_irqoff
>
> __raise_softirq_irqoff will update per-CPU mask of pending softirqs, it n=
eed to be called in irq disabled context in order to keep it atomic operati=
on, otherwise it will be interrupted by hardware interrupt, and per-CPU sof=
tirqs pending mask will be corrupted, the result is there will be unexpecte=
d issue, for example hrtimer soft irq will be losed and soft hrtimer will n=
ever be expire and handled.

Please wrap your change logs.

>
> Adding irqs disabled checking here to provide warning in irqs enabled con=
text.
>
> Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> ---
>  kernel/softirq.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c index=20
> bf88d7f62433..11f61e54a3ae 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -481,6 +481,11 @@ void raise_softirq(unsigned int nr)
>
>  void __raise_softirq_irqoff(unsigned int nr)  {
> +     /* This function can only be called in irq disabled context,
> +      * otherwise or_softirq_pending will be interrupted by hardware
> +      * interrupt, so that there will be unexpected issue.
> +      */
> +     WARN_ON_ONCE(!irqs_disabled());

Perhaps: lockdep_assert_irqs_disabled() is more appropriate, and doesn't ad=
d extra overhead on production systems.

-- Steve


>       trace_softirq_raise(nr);
>       or_softirq_pending(1UL << nr);
>  }
> --
> 2.17.1