lkml.org 
[lkml]   [2020]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [EXT] Re: [PATCH] softirq: add irq off checking for __raise_softirq_irqoff
Date
> From: Steven Rostedt <rostedt@goodmis.org>
> Sent: Thursday, August 13, 2020 10:57 PM
>
> 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.
>
> > 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?
> >
>
>
> > 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 softirqs
> 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 never be expire
> and handled.
>
> Please wrap your change logs.
[Jiafei Pan] Thanks, will update it.
>
> >
> > 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());
>
> Perhaps: lockdep_assert_irqs_disabled() is more appropriate, and doesn't add
> extra overhead on production systems.
>
> -- Steve
[Jiafei Pan] Thanks, will update it.
>
>
> > trace_softirq_raise(nr);
> > or_softirq_pending(1UL << nr);
> > }
> > --
> > 2.17.1

\
 
 \ /
  Last update: 2020-08-14 06:18    [W:0.093 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site