Messages in this thread | | | From | Jiafei Pan <> | Subject | RE: [EXT] Re: [PATCH] softirq: add irq off checking for __raise_softirq_irqoff | Date | Fri, 14 Aug 2020 04:17:19 +0000 |
| |
> 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
| |