linux-kernel - Re: [RFC PATCH 3/3] softirq: Avoid waking up ksoftirqd from flush_smp_call_function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240710150557.GB27299@noisy.programming.kicks-ass.net>
Date: Wed, 10 Jul 2024 17:05:57 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: K Prateek Nayak <kprateek.nayak@....com>
Cc: Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	linux-kernel@...r.kernel.org,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Daniel Bristot de Oliveira <bristot@...hat.com>,
	Valentin Schneider <vschneid@...hat.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Imran Khan <imran.f.khan@...cle.com>,
	Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>,
	Rik van Riel <riel@...riel.com>, Tejun Heo <tj@...nel.org>,
	Cruz Zhao <CruzZhao@...ux.alibaba.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Zqiang <qiang.zhang1211@...il.com>,
	Julia Lawall <julia.lawall@...ia.fr>,
	"Gautham R. Shenoy" <gautham.shenoy@....com>
Subject: Re: [RFC PATCH 3/3] softirq: Avoid waking up ksoftirqd from
 flush_smp_call_function_queue()

On Wed, Jul 10, 2024 at 09:02:10AM +0000, K Prateek Nayak wrote:

> diff --git a/kernel/sched/smp.h b/kernel/sched/smp.h
> index 21ac44428bb0..3731e79fe19b 100644
> --- a/kernel/sched/smp.h
> +++ b/kernel/sched/smp.h
> @@ -9,7 +9,9 @@ extern void sched_ttwu_pending(void *arg);
>  extern bool call_function_single_prep_ipi(int cpu);
>  
>  #ifdef CONFIG_SMP
> +extern bool do_softirq_pending(void);
>  extern void flush_smp_call_function_queue(void);
>  #else
> +static inline bool do_softirq_pending(void) { return false; }
>  static inline void flush_smp_call_function_queue(void) { }
>  #endif
> diff --git a/kernel/smp.c b/kernel/smp.c
> index f085ebcdf9e7..2eab5e1d5cef 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -559,6 +559,36 @@ static void __flush_smp_call_function_queue(bool warn_cpu_offline)
>  	}
>  }
>  
> +/* Indicate an impending call to do_softirq_post_smp_call_flush() */
> +static DEFINE_PER_CPU_ALIGNED(bool, will_do_softirq_post_flush);
> +
> +static __always_inline void __set_will_do_softirq_post_flush(void)
> +{
> +	this_cpu_write(will_do_softirq_post_flush, true);
> +}
> +
> +static __always_inline void __clr_will_do_softirq_post_flush(void)
> +{
> +	this_cpu_write(will_do_softirq_post_flush, false);
> +}
> +
> +/**
> + * do_softirq_pending - Check if do_softirq_post_smp_call_flush() will
> + *			be called after the invocation of
> + *			__flush_smp_call_function_queue()
> + *
> + * When flush_smp_call_function_queue() executes in the context of idle,
> + * migration thread, a softirq raised from the smp-call-function ends up
> + * waking ksoftirqd despite an impending softirq processing via
> + * do_softirq_post_smp_call_flush().
> + *
> + * Indicate an impending do_softirq() to should_wake_ksoftirqd() despite
> + * not being in an interrupt context.
> + */
> +__always_inline bool do_softirq_pending(void)
> +{
> +	return this_cpu_read(will_do_softirq_post_flush);
> +}
>  
>  /**
>   * flush_smp_call_function_queue - Flush pending smp-call-function callbacks
> @@ -583,7 +613,9 @@ void flush_smp_call_function_queue(void)
>  	local_irq_save(flags);
>  	/* Get the already pending soft interrupts for RT enabled kernels */
>  	was_pending = local_softirq_pending();
> +	__set_will_do_softirq_post_flush();
>  	__flush_smp_call_function_queue(true);
> +	__clr_will_do_softirq_post_flush();
>  	if (local_softirq_pending())
>  		do_softirq_post_smp_call_flush(was_pending);
>  
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 02582017759a..b39eeed03042 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -34,6 +34,8 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/irq.h>
>  
> +#include "sched/smp.h"
> +
>  /*
>     - No shared variables, all the data are CPU local.
>     - If a softirq needs serialization, let it serialize itself
> @@ -413,7 +415,13 @@ static inline void ksoftirqd_run_end(void)
>  
>  static inline bool should_wake_ksoftirqd(void)
>  {
> -	return true;
> +	/*
> +	 * Avoid waking up ksoftirqd when a softirq is raised from a
> +	 * call-function executed by flush_smp_call_function_queue()
> +	 * in idle, migration thread's context since it'll soon call
> +	 * do_softirq_post_smp_call_flush().
> +	 */
> +	return !do_softirq_pending();
>  }

On first reading I wonder why you've not re-used and hooked into the
PREEMPT_RT variant of should_wake_ksoftirqd(). That already has a per
CPU variable to do exactly this.