Re: [Question] softlockup in run_timer_softirq

[liujian (CE)]

On 2023/5/2 11:06, John Stultz wrote:
> On Wed, Feb 15, 2023 at 12:34 AM liujian (CE) <liujian56@xxxxxxxxxx> wrote:
> > > On Fri, Feb 10, 2023 at 1:51 AM liujian (CE) <liujian56@xxxxxxxxxx> wrote:
> > > > During the syz test, we encountered many problems with various timer
> > > > handler functions softlockup.
> > > >
> > > > We analyze __run_timers() and find the following problem.
> > > >
> > > > In the while loop of __run_timers(), because there are too many timers
> > > > or improper timer handler functions, if the processing time of the
> > > > expired timers is always greater than the time wheel's next_expiry,
> > > > the function will loop infinitely.
> > > >
> > > > The following extreme test case can be used to reproduce the problem.
> > > > An extreme test case[1] is constructed to reproduce the problem.
> > >
> > > Thanks for reporting and sending out this data:
> > >
> > > First, any chance you might submit this as a in-kernel-stress test?
> > > Maybe utilizing the kernel/torture.c framework?
> > Okay,   I'll learn this framework and do this thing.
> > > (Though the test may need to occasionally take a break so the system can
> > > eventually catch up)
> > >
> > > > Is this a problem or an unreasonable use?
> > > >
> > > > Can we limit the running time of __run_timers() [2]?
> > > >
> > > > Does anyone have a good idea to solve this problem?
> > >
> > > So your patch reminds me of Peter's softirq_needs_break() logic:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=co
> > > re/softirq
> > >
> > > Maybe it could extend that series for the timer softirq as well?
> > Thank you. Yes.
> > Base on the patchset and the extended patch for timer [1], the soft lockup problem does not occur.
> >
> > By the way, I see this is a very old patchset?  Will this patchset push the main line? @John @Peter
> >
> >
> >   [1]
> > Author: Liu Jian <liujian56@xxxxxxxxxx>
> > Date:   Tue Feb 14 09:53:46 2023 +0800
> >
> >      softirq, timer: Use softirq_needs_break()
> >
> >      In the while loop of __run_timers(), because there are too many timers or
> >      improper timer handler functions, if the processing time of the expired
> >      timers is always greater than the time wheel's next_expiry, the function
> >      will loop infinitely.
> >
> >      To prevent this, use the timeout/break logic provided by SoftIRQs.If the
> >      running time exceeds the limit, break the loop and an additional
> >      TIMER_SOFTIRQ is triggered.
> >
> >      Signed-off-by: Liu Jian <liujian56@xxxxxxxxxx>
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index 63a8ce7177dd..70744a469a39 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -1992,7 +1992,7 @@ void timer_clear_idle(void)
> >    * __run_timers - run all expired timers (if any) on this CPU.
> >    * @base: the timer vector to be processed.
> >    */
> > -static inline void __run_timers(struct timer_base *base)
> > +static inline void __run_timers(struct timer_base *base, struct softirq_action *h)
> >   {
> >          struct hlist_head heads[LVL_DEPTH];
> >          int levels;
> > @@ -2020,6 +2020,12 @@ static inline void __run_timers(struct timer_base *base)
> >
> >                  while (levels--)
> >                          expire_timers(base, heads + levels);
> > +
> > +               if (softirq_needs_break(h)) {
> > +                       if (time_after_eq(jiffies, base->next_expiry))
> > +                               __raise_softirq_irqoff(TIMER_SOFTIRQ);
> > +                       break;
> > +               }
> >          }
> >          raw_spin_unlock_irq(&base->lock);
> >          timer_base_unlock_expiry(base);
> > @@ -2032,9 +2038,9 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
> >   {
> >          struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
> >
> > -       __run_timers(base);
> > +       __run_timers(base, h);
> >          if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
> > -               __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
> > +               __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]), h);
> >   }
> >
> >   /*
>
> So I wanted to revive this old thread, as Frank Woo mentioned his team
> has seen a similar issue as well.
>
> Liujian: I'm curious if you've made any further progress with your
> adapted patch ontop of PeterZ's softirq_needs_break patch series?
Hi John,
  Only the commit ("softirq, timer: Use softirq_needs_break()") is 
added to the patchset of Peter, and no other modification is made.
> Might it be worth re-submitting the whole series for consideration upstream?
I agree very much and expect, because we often encounter similar 
problems when doing fuzzy tests (especially when the test machine is poor).
> thanks
> -john