Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Dec 2022 17:15:05 +0000
From:   Joel Fernandes <joel@joelfernandes.org>
To:     John Stultz <jstultz@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Pavankumar Kondeti <pkondeti@codeaurora.org>,
        John Dias <joaodias@google.com>,
        Connor O'Brien <connoro@google.com>,
        Rick Yiu <rickyiu@google.com>, John Kacur <jkacur@redhat.com>,
        Qais Yousef <qyousef@google.com>,
        Chris Redpath <chris.redpath@arm.com>,
        Abhijeet Dharmapurikar <adharmap@quicinc.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        kernel-team@android.com,
        Satya Durga Srinivasu Prabhala <satyap@codeaurora.org>,
        "J . Avila" <elavila@google.com>
Subject: Re: [PATCH v5 3/3] softirq: defer softirq processing to ksoftirqd if
 CPU is busy with RT
Message-ID: <Y5IbmWjZTw2t3SH9@google.com>
References: <20221116075929.453876-1-jstultz@google.com>
 <20221116075929.453876-4-jstultz@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221116075929.453876-4-jstultz@google.com>
Precedence: bulk

Hi John,

On Wed, Nov 16, 2022 at 07:59:28AM +0000, John Stultz wrote:
> From: Pavankumar Kondeti <pkondeti@codeaurora.org>
> 
> Defer the softirq processing to ksoftirqd if a RT task is
> running or queued on the current CPU. This complements the RT
> task placement algorithm which tries to find a CPU that is not
> currently busy with softirqs.
> 
> Currently NET_TX, NET_RX, BLOCK and IRQ_POLL softirqs are only
> deferred as they can potentially run for long time.
> 
> Additionally, this patch stubs out ksoftirqd_running() logic,
> in the CONFIG_RT_SOFTIRQ_AWARE_SCHED case, as deferring
> potentially long-running softirqs will cause the logic to not
> process shorter-running softirqs immediately. By stubbing it out
> the potentially long running softirqs are deferred, but the
> shorter running ones can still run immediately.
> 
> This patch includes folded-in fixes by:
>   Lingutla Chandrasekhar <clingutla@codeaurora.org>
>   Satya Durga Srinivasu Prabhala <satyap@codeaurora.org>
>   J. Avila <elavila@google.com>
> 
[...]
> ---
> v4:
> * Fix commit message to accurately note long-running softirqs
>   (suggested by Qais)
> * Switch to using rt_task(current) (suggested by Qais)
> v5:
> * Switch to using CONFIG_RT_SOFTIRQ_AWARE_SCHED (suggested by
>   Joel Fernandes <joel@joelfernandes.org>)
> ---
>  kernel/softirq.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index dd92ce8f771b..5db2afd0be68 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -95,6 +95,7 @@ static void wakeup_softirqd(void)
>  		wake_up_process(tsk);
>  }
>  
> +#ifndef CONFIG_RT_SOFTIRQ_AWARE_SCHED
>  /*
>   * If ksoftirqd is scheduled, we do not want to process pending softirqs
>   * right now. Let ksoftirqd handle this at its own rate, to get fairness,
> @@ -109,6 +110,9 @@ static bool ksoftirqd_running(unsigned long pending)
>  		return false;
>  	return tsk && task_is_running(tsk) && !__kthread_should_park(tsk);
>  }
> +#else
> +#define ksoftirqd_running(pending) (false)
> +#endif /* CONFIG_RT_SOFTIRQ_AWARE_SCHED */
>  
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  DEFINE_PER_CPU(int, hardirqs_enabled);
> @@ -540,6 +544,21 @@ static inline bool lockdep_softirq_start(void) { return false; }
>  static inline void lockdep_softirq_end(bool in_hardirq) { }
>  #endif
>  
> +#ifdef CONFIG_RT_SOFTIRQ_AWARE_SCHED
> +static __u32 softirq_deferred_for_rt(__u32 *pending)
> +{
> +	__u32 deferred = 0;
> +
> +	if (rt_task(current)) {

Over here, I suggest also check dl_task(current). SCHED_DEADLINE is being
used for the 'sugov' (schedutil governor threads) on currently shipping
products, and DL should be treated greater than RT priority.

On the other hand, the DL counterpart to avoid softirq is not there, but at
least softirq can defer for DL threads.

> +		deferred = *pending & LONG_SOFTIRQ_MASK;
> +		*pending &= ~LONG_SOFTIRQ_MASK;
> +	}
> +	return deferred;
> +}
> +#else
> +#define softirq_deferred_for_rt(x) (0)
> +#endif
> +
>  asmlinkage __visible void __softirq_entry __do_softirq(void)
>  {
>  	unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> @@ -547,6 +566,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
>  	int max_restart = MAX_SOFTIRQ_RESTART;
>  	struct softirq_action *h;
>  	bool in_hardirq;
> +	__u32 deferred;
>  	__u32 pending;
>  	int softirq_bit;
>  
> @@ -558,14 +578,16 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
>  	current->flags &= ~PF_MEMALLOC;
>  
>  	pending = local_softirq_pending();
> +	deferred = softirq_deferred_for_rt(&pending);
>  
>  	softirq_handle_begin();
> +
>  	in_hardirq = lockdep_softirq_start();
>  	account_softirq_enter(current);
>  
>  restart:
>  	/* Reset the pending bitmask before enabling irqs */
> -	set_softirq_pending(0);
> +	set_softirq_pending(deferred);

Over here, the local pending mask is set to whatever was deferred [1]..

>  	set_active_softirqs(pending);
>  
>  	local_irq_enable();
> @@ -604,13 +626,16 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
>  	local_irq_disable();
>  
>  	pending = local_softirq_pending();
> +	deferred = softirq_deferred_for_rt(&pending);

... so over here, pending should always be set to atleast the deferred bits
if deferred is set (unless something wokeup and executed softirqs while
interrupts are enabled -- which should not happen because BH is disabled
through out) [2].

> +
>  	if (pending) {
>  		if (time_before(jiffies, end) && !need_resched() &&
>  		    --max_restart)

So then here, pending (which also now contains deferred due to [1] [2])
should already take care off waking up ksoftirqd, except that perhaps it
should be like this:

  	if (pending) {
		// Also, don't restart if something was deferred, let the RT task
		// breath a bit.
  		if (time_before(jiffies, end) && !need_resched() &&
			!deferred && --max_restart)
			goto restart;

		wakeup_softirqd();
	}

Or, will that not work?

My point being that we probably don't want to go through the retry-cycle,
if something was deferred since the plan is to wake up ksoftirqd in such
cases.

> +	if (pending | deferred)
>  		wakeup_softirqd();

And then perhaps this check can be removed.

Thoughts?

thanks,

 - Joel


>  			goto restart;
> +	}
>  
> +	if (pending | deferred)
>  		wakeup_softirqd();
> -	}
>  
>  	account_softirq_exit(current);
>  	lockdep_softirq_end(in_hardirq);
> -- 
> 2.38.1.431.g37b22c650d-goog
>