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>
CC:     "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "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: AQHWa6gdrPnfNrtYpEW7p3l8OUGNa6k1YJYQgADMBQCAAN9u0A==
Date:   Fri, 14 Aug 2020 04:17:19 +0000
Message-ID: <AM0PR04MB47728EEAE7457ADC1F29F47D8A400@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: b9038b64-2334-4bc5-aed9-08d84008ef52
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Aug 2020 04:17:19.0665
 (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: HX8UUanWhVomwwOi5EdcPOWBiKhAUDCJ+uSZDdobgqB8zenlQxa4f9DgDgYsuVrlFWhUr0vrshtUOg1x6Ocleg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB6898
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Steven Rostedt <rostedt@goodmis.org>
> Sent: Thursday, August 13, 2020 10:57 PM
>=20
> On Thu, 13 Aug 2020 03:03:46 +0000
> Jiafei Pan <jiafei.pan@nxp.com> wrote:
>=20
> > Any comments? Thanks.
> >
> > @Steven Rostedt, I thinks irq off checking is necessary especially
>=20
> This is probably more for Thomas Gleixner.
>=20
> > for Preempt-RT kernel, because some context may be changed from irq
> > off to irq on when enable Preempt RT, I once met a issue that hrtimer
> > soft irq is lost when enabled Preempt RT, finally I found
> > napi_schedule_irqoff is called in hardware interrupt handler, there
> > maybe no issue for non RT kernel, but for Preempt RT, interrupt is
> > threaded, so irq is on in interrupt handler, the result 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 hrtimer soft irq is lost. So I think adding irq status checking
> > in __raise_softirq_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 items? Such as CONFIG_DEBUG_PREEMPT or other debug
> > items?
> >
>=20
>=20
> > 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;
> > rostedt@goodmis.org; romain.perier@gmail.com; will@kernel.org
> > Cc: linux-kernel@vger.kernel.org; linux-rt-users@vger.kernel.org;
> > Jiafei Pan <jiafei.pan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Vladimir
> > Oltean <vladimir.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=
 need
> 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 is=
sue,
> for example hrtimer soft irq will be losed and soft hrtimer will never be=
 expire
> and handled.
>=20
> Please wrap your change logs.
[Jiafei Pan] Thanks, will update it.
>=20
> >
> > Adding irqs disabled checking here to provide warning in irqs enabled
> context.
> >
> > 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..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());
>=20
> Perhaps: lockdep_assert_irqs_disabled() is more appropriate, and doesn't =
add
> extra overhead on production systems.
>=20
> -- Steve
[Jiafei Pan] Thanks, will update it.
>=20
>=20
> >       trace_softirq_raise(nr);
> >       or_softirq_pending(1UL << nr);
> >  }
> > --
> > 2.17.1