Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/3] softirq: avoid spurious stalls due to need_resched() | Date | Fri, 03 Mar 2023 14:30:46 +0100 |
| |
Jakub!
On Thu, Dec 22 2022 at 14:12, Jakub Kicinski wrote: > DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > +static DEFINE_PER_CPU(unsigned long, overload_limit); > > const char * const softirq_to_name[NR_SOFTIRQS] = { > "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "IRQ_POLL", > @@ -89,10 +90,15 @@ static void wakeup_softirqd(void) > static bool ksoftirqd_should_handle(unsigned long pending) > { > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > + unsigned long ov_limit; > > if (pending & SOFTIRQ_NOW_MASK) > return false; > - return tsk && task_is_running(tsk) && !__kthread_should_park(tsk); > + if (likely(!tsk || !task_is_running(tsk) || __kthread_should_park(tsk))) > + return false; > + > + ov_limit = __this_cpu_read(overload_limit); > + return time_is_after_jiffies(ov_limit);
return time_is_after_jiffies(__this_cpu_read(overload_limit));
Plus a comment explaining the magic, please.
> } > > #ifdef CONFIG_TRACE_IRQFLAGS > @@ -492,6 +498,9 @@ asmlinkage __visible void do_softirq(void) > #define MAX_SOFTIRQ_TIME msecs_to_jiffies(2) > #define MAX_SOFTIRQ_RESTART 10 > > +#define SOFTIRQ_OVERLOAD_TIME msecs_to_jiffies(100) > +#define SOFTIRQ_DEFER_TIME msecs_to_jiffies(2) > + > #ifdef CONFIG_TRACE_IRQFLAGS > /* > * When we run softirqs from irq_exit() and thus on the hardirq stack we need > @@ -588,10 +597,16 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) > > pending = local_softirq_pending(); > if (pending) { > - if (time_before(jiffies, end) && !need_resched() && > - --max_restart) > + unsigned long limit; > + > + if (time_is_before_eq_jiffies(end) || !--max_restart) > + limit = SOFTIRQ_OVERLOAD_TIME; > + else if (need_resched()) > + limit = SOFTIRQ_DEFER_TIME; > + else > goto restart; > > + __this_cpu_write(overload_limit, jiffies + limit);
The logic of all this is non-obvious and I had to reread it 5 times to conclude that it is matching the intent. Please add comments.
While I'm not a big fan of heuristical duct tape, this looks harmless enough to not end up in an endless stream of tweaking. Famous last words...
But without the sched_clock() changes the actual defer time depends on HZ and the point in time where limit is set. That means it ranges from 0 to 1/HZ, i.e. the 2ms defer time ends up with close to 10ms on HZ=100 in the worst case, which perhaps explains the 8ms+ stalls you are still observing. Can you test with that sched_clock change applied, i.e. the first two commits from
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/softirq
59be25c466d9 ("softirq: Use sched_clock() based timeout") bd5a5bd77009 ("softirq: Rewrite softirq processing loop")
whether that makes a difference? Those two can be applied with some minor polishing. The rest of that series is broken by f10020c97f4c ("softirq: Allow early break").
There is another issue with this overload limit. Assume max_restart or timeout triggered and limit was set to now + 100ms. ksoftirqd runs and gets the issue resolved after 10ms.
So for the remaining 90ms any invocation of raise_softirq() outside of (soft)interrupt context, which wakes ksoftirqd again, prevents processing on return from interrupt until ksoftirqd gets on the CPU and goes back to sleep, because task_is_running() == true and the stale limit is not after jiffies.
Probably not a big issue, but someone will notice on some weird workload sooner than later and the tweaking will start nevertheless. :) So maybe we fix it right away. :)
Thanks,
tglx
| |