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:     Jiafei Pan <jiafei.pan@nxp.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "romain.perier@gmail.com" <romain.perier@gmail.com>,
        "will@kernel.org" <will@kernel.org>
CC:     "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: [PATCH] softirq: add irq off checking for __raise_softirq_irqoff
Thread-Topic: [PATCH] softirq: add irq off checking for __raise_softirq_irqoff
Thread-Index: AQHWa6gdrPnfNrtYpEW7p3l8OUGNa6k1YJYQ
Date:   Thu, 13 Aug 2020 03:03:46 +0000
Message-ID: <AM0PR04MB4772B1B6D3391BAEAD4411808A430@AM0PR04MB4772.eurprd04.prod.outlook.com>
References: <20200806040729.39186-1-Jiafei.Pan@nxp.com>
In-Reply-To: <20200806040729.39186-1-Jiafei.Pan@nxp.com>
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: dd12a398-837a-4840-5b85-08d83f357edc
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Aug 2020 03:03:46.5693
 (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: +k3qJv3LyQrWMc53kn7J2UsWmQgB0UKtp7a/RrIzYGq2kDntjKJd1ImNnTzgdkO/ZZjzY9XOF+Ug5qBsi7WuTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR0402MB3441
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Any comments? Thanks.

@Steven Rostedt, I thinks irq off checking is necessary especially for Pree=
mpt-RT kernel, because some context may be changed from irq off to irq on w=
hen enable Preempt RT, I once met a issue that hrtimer soft irq is lost whe=
n enabled Preempt RT, finally I found napi_schedule_irqoff is called in har=
dware interrupt handler, there maybe no issue for non RT kernel, but for Pr=
eempt RT, interrupt is threaded, so irq is on in interrupt handler, the res=
ult is __raise_softirq_irqoff is called in irq on context, so that per-CPU =
softirq masking is corrupted because of the process of updating of soft irq=
 masking is interrupted and not a atomic operation , and then caused hrtime=
r soft irq is lost. So I think adding irq status checking in __raise_softir=
q_irqoff can report such issue directly and help us to find the root cause =
of such issue.

I know that there may be performance impaction to add extra checking here, =
if it is the case, how about to include it in some debug configuration item=
s? Such as CONFIG_DEBUG_PREEMPT or other debug items?

Best Regards,
Jiafei.

-----Original Message-----
From: Jiafei Pan <Jiafei.Pan@nxp.com>=20
Sent: Thursday, August 6, 2020 12:07 PM
To: peterz@infradead.org; mingo@kernel.org; tglx@linutronix.de; rostedt@goo=
dmis.org; romain.perier@gmail.com; will@kernel.org
Cc: linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org; Jiafei Pa=
n <jiafei.pan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Vladimir Oltean <vladi=
mir.oltean@nxp.com>; Jiafei Pan <jiafei.pan@nxp.com>
Subject: [PATCH] softirq: add irq off checking for __raise_softirq_irqoff

__raise_softirq_irqoff will update per-CPU mask of pending softirqs, it nee=
d to be called in irq disabled context in order to keep it atomic operation=
, otherwise it will be interrupted by hardware interrupt, and per-CPU softi=
rqs pending mask will be corrupted, the result is there will be unexpected =
issue, for example hrtimer soft irq will be losed and soft hrtimer will nev=
er be expire and handled.

Adding irqs disabled checking here to provide warning in irqs enabled conte=
xt.

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 bf88d7f62433..11f61e=
54a3ae 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -481,6 +481,11 @@ void raise_softirq(unsigned int nr)
=20
 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());
 	trace_softirq_raise(nr);
 	or_softirq_pending(1UL << nr);
 }
--
2.17.1