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;
MIME-Version: 1.0
References: <1594967873-29522-1-git-send-email-qianjun.kernel@gmail.com> <20200717220735.GA5965@pc636>
In-Reply-To: <20200717220735.GA5965@pc636>
From:   jun qian <qianjun.kernel@gmail.com>
Date:   Mon, 20 Jul 2020 19:33:10 +0800
Message-ID: <CAKc596Ju9LAbTH6YNnkpHQO3qc001mmjiP-0C=6kmTdOtMy_2Q@mail.gmail.com>
Subject: Re: [RFC PATCH 1/1] Softirq:avoid large sched delay from the pending softirqs
To:     Uladzislau Rezki <urezki@gmail.com>
Cc:     tglx@linutronix.de, peterz@infradead.org, will@kernel.org,
        luto@kernel.org, linux-kernel@vger.kernel.org, laoar.shao@gmail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jul 18, 2020 at 6:07 AM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> > From: jun qian <qianjun.kernel@gmail.com>
> >
> > When get the pending softirqs, it need to process all the pending
> > softirqs in the while loop. If the processing time of each pending
> > softirq is need more than 2 msec in this loop, or one of the softirq
> > will running a long time, according to the original code logic, it
> > will process all the pending softirqs without wakeuping ksoftirqd,
> > which will cause a relatively large scheduling delay on the
> > corresponding CPU, which we do not wish to see. The patch will check
> > the total time to process pending softirq, if the time exceeds 2 ms
> > we need to wakeup the ksofirqd to aviod large sched delay.
> >
> > Signed-off-by: jun qian <qianjun.kernel@gmail.com>
> > ---
> >  kernel/softirq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index c4201b7f..602d9fa 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -299,6 +299,9 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
> >               }
> >               h++;
> >               pending >>= softirq_bit;
> > +
> > +             if (time_after(jiffies, end) && need_resched())
> > +                     break;
> >       }
> >
> >       if (__this_cpu_read(ksoftirqd) == current)
> >
> I have a small concern about MAX_SOFTIRQ_TIME. The problem is that
> an "end" time is based on jiffies/tick update, so it depends on CONFIG_HZ
> value of your kernel.
>
> For example if we have CONFIG_HZ=100, msecs_to_jiffies(2) will return 1.
> For HZ=100 one jiffie is 10 milliseconds. So we can not rely on it,
> because of low resolution.
>
 good tip. Does this problem also exist in the current code, just like this:

        if (pending) {
                if (time_before(jiffies, end) && !need_resched() &&
/* low resolution problem */
                    --max_restart)
                        goto restart;

                wakeup_softirqd();
        }

> Maybe it make sense to fix it first in order to be at least aligned with
> "2 milliseconds time limit" documentation?
>
> <snip>
>  * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
>  * but break the loop if need_resched() is set or after 2 ms.
> <snip>
>
I can't find the snip from the linux/Documentation/, could you please
tell me where I can find this snip, thks

> ktime_get()/ktime_before()...?
>
if the low resolution problem also exists in the above code, i think
also need to fix it with using ktime_get()/ktime_before().

> --
> Vlad Rezki