Re: [RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

On Thu, Jul 23, 2020 at 2:05 AM Thomas Gleixner  wrote:
>
> qianjun.ker...@gmail.com writes:
> > +
> > + end = ktime_get();
> > + delta = ktime_to_us(end - start);
>
> What's the point of this conversion? That's a division for no value
> because you can simply define the maximum time in nanoseconds with the
> same effect, i.e.
>
> ktime_t end = ktime_get() + MAX_SOFTIRQ_TIME_NS;
>
> if (need_resched() && ktime_get() > end)
> break;
>
> So you can spare all that start, delta and conversion dance and keep the
> code simple.
>
> Also notice that need_resched() wants to be evaluated first because
> there is no point to do the more expensive time read if need_resched()
> is false.
good suggestion，Thanks

I will make changes in the next version
>
> Thanks,
>
> tglx

Re: [RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[Thomas Gleixner]

qianjun.ker...@gmail.com writes:
> +
> + end = ktime_get();
> + delta = ktime_to_us(end - start);

What's the point of this conversion? That's a division for no value
because you can simply define the maximum time in nanoseconds with the
same effect, i.e.

ktime_t end = ktime_get() + MAX_SOFTIRQ_TIME_NS;

if (need_resched() && ktime_get() > end)
break;

So you can spare all that start, delta and conversion dance and keep the
code simple.

Also notice that need_resched() wants to be evaluated first because
there is no point to do the more expensive time read if need_resched()
is false.

Thanks,

tglx

Re: [RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[jun qian]

Hi Peter & Uladzislau

Are there any issues that have not been considered in this patch? Can
you give me some suggestions on this issue. If the situation I
described is indeed a problem，how about this modification. Thanks a
lot.

On Mon, Jul 20, 2020 at 9:09 PM  wrote:
>
> From: jun qian 
>
> 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 
> ---
>  kernel/softirq.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index c4201b7f..f8e5be9 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -210,7 +210,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int 
> cnt)
>   * we want to handle softirqs as soon as possible, but they
>   * should not be able to lock up the box.
>   */
> -#define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
> +#define MAX_SOFTIRQ_TIME 2000  /* In microseconds */
>  #define MAX_SOFTIRQ_RESTART 10
>
>  #ifdef CONFIG_TRACE_IRQFLAGS
> @@ -248,7 +248,8 @@ static inline void lockdep_softirq_end(bool in_hardirq) { 
> }
>
>  asmlinkage __visible void __softirq_entry __do_softirq(void)
>  {
> -   unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> +   ktime_t end, start;
> +   s64 delta;
> unsigned long old_flags = current->flags;
> int max_restart = MAX_SOFTIRQ_RESTART;
> struct softirq_action *h;
> @@ -256,6 +257,8 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
> __u32 pending;
> int softirq_bit;
>
> +   start = ktime_get();
> +
> /*
>  * Mask out PF_MEMALLOC as the current task context is borrowed for 
> the
>  * softirq. A softirq handled, such as network RX, might set 
> PF_MEMALLOC
> @@ -299,6 +302,15 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
> }
> h++;
> pending >>= softirq_bit;
> +
> +   end = ktime_get();
> +   delta = ktime_to_us(end - start);
> +   /*
> +* the softirq's action has been running for too much time
> +* so it may need to wakeup the ksoftirqd
> +*/
> +   if (delta > MAX_SOFTIRQ_TIME && need_resched())
> +   break;
> }
>
> if (__this_cpu_read(ksoftirqd) == current)
> @@ -307,7 +319,9 @@ asmlinkage __visible void __softirq_entry 
> __do_softirq(void)
>
> pending = local_softirq_pending();
> if (pending) {
> -   if (time_before(jiffies, end) && !need_resched() &&
> +   end = ktime_get();
> +   delta = ktime_to_us(end - start);
> +   if (delta < MAX_SOFTIRQ_TIME && !need_resched() &&
> --max_restart)
> goto restart;
>
> --
> 1.8.3.1
>

[RFC PATCH v2] Softirq:avoid large sched delay from the pending softirqs

[qianjun . kernel]

From: jun qian 

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 
---
 kernel/softirq.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index c4201b7f..f8e5be9 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -210,7 +210,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int 
cnt)
  * we want to handle softirqs as soon as possible, but they
  * should not be able to lock up the box.
  */
-#define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
+#define MAX_SOFTIRQ_TIME 2000  /* In microseconds */
 #define MAX_SOFTIRQ_RESTART 10
 
 #ifdef CONFIG_TRACE_IRQFLAGS
@@ -248,7 +248,8 @@ static inline void lockdep_softirq_end(bool in_hardirq) { }
 
 asmlinkage __visible void __softirq_entry __do_softirq(void)
 {
-   unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
+   ktime_t end, start;
+   s64 delta;
unsigned long old_flags = current->flags;
int max_restart = MAX_SOFTIRQ_RESTART;
struct softirq_action *h;
@@ -256,6 +257,8 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
__u32 pending;
int softirq_bit;
 
+   start = ktime_get();
+
/*
 * Mask out PF_MEMALLOC as the current task context is borrowed for the
 * softirq. A softirq handled, such as network RX, might set PF_MEMALLOC
@@ -299,6 +302,15 @@ asmlinkage __visible void __softirq_entry 
__do_softirq(void)
}
h++;
pending >>= softirq_bit;
+
+   end = ktime_get();
+   delta = ktime_to_us(end - start);
+   /*
+* the softirq's action has been running for too much time
+* so it may need to wakeup the ksoftirqd
+*/
+   if (delta > MAX_SOFTIRQ_TIME && need_resched())
+   break;
}
 
if (__this_cpu_read(ksoftirqd) == current)
@@ -307,7 +319,9 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 
pending = local_softirq_pending();
if (pending) {
-   if (time_before(jiffies, end) && !need_resched() &&
+   end = ktime_get();
+   delta = ktime_to_us(end - start);
+   if (delta < MAX_SOFTIRQ_TIME && !need_resched() &&
--max_restart)
goto restart;
 
-- 
1.8.3.1