lkml.org 
[lkml]   [2023]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 2/3] softirq: avoid spurious stalls due to need_resched()
    Date
    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

    \
     
     \ /
      Last update: 2023-03-27 00:43    [W:4.564 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site