Received-SPF: pass (google.com: domain of linux-kernel+bounces-192750-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 28 May 2024 18:25:34 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Ankur Arora <ankur.a.arora@oracle.com>
Cc: linux-kernel@vger.kernel.org, tglx@linutronix.de,
	torvalds@linux-foundation.org, paulmck@kernel.org,
	rostedt@goodmis.org, mark.rutland@arm.com, juri.lelli@redhat.com,
	joel@joelfernandes.org, raghavendra.kt@amd.com,
	sshegde@linux.ibm.com, boris.ostrovsky@oracle.com,
	konrad.wilk@oracle.com, Ingo Molnar <mingo@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH v2 12/35] sched: separate PREEMPT_DYNAMIC config logic
Message-ID: <20240528162534.GG26599@noisy.programming.kicks-ass.net>
References: <20240528003521.979836-1-ankur.a.arora@oracle.com>
 <20240528003521.979836-13-ankur.a.arora@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240528003521.979836-13-ankur.a.arora@oracle.com>

On Mon, May 27, 2024 at 05:34:58PM -0700, Ankur Arora wrote:
> Pull out the PREEMPT_DYNAMIC setup logic to allow other preemption
> models to dynamically configure preemption.

Uh what ?!? What's the point of creating back-to-back #ifdef sections ?

> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>  kernel/sched/core.c | 165 +++++++++++++++++++++++---------------------
>  1 file changed, 86 insertions(+), 79 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 0c26b60c1101..349f6257fdcd 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8713,6 +8713,89 @@ int __cond_resched_rwlock_write(rwlock_t *lock)
>  }
>  EXPORT_SYMBOL(__cond_resched_rwlock_write);
>  
> +#if defined(CONFIG_PREEMPT_DYNAMIC)
> +
> +#define PREEMPT_MODE "Dynamic Preempt"
> +
> +enum {
> +	preempt_dynamic_undefined = -1,
> +	preempt_dynamic_none,
> +	preempt_dynamic_voluntary,
> +	preempt_dynamic_full,
> +};
> +
> +int preempt_dynamic_mode = preempt_dynamic_undefined;
> +static DEFINE_MUTEX(sched_dynamic_mutex);
> +
> +int sched_dynamic_mode(const char *str)
> +{
> +	if (!strcmp(str, "none"))
> +		return preempt_dynamic_none;
> +
> +	if (!strcmp(str, "voluntary"))
> +		return preempt_dynamic_voluntary;
> +
> +	if (!strcmp(str, "full"))
> +		return preempt_dynamic_full;
> +
> +	return -EINVAL;
> +}
> +
> +static void __sched_dynamic_update(int mode);
> +void sched_dynamic_update(int mode)
> +{
> +	mutex_lock(&sched_dynamic_mutex);
> +	__sched_dynamic_update(mode);
> +	mutex_unlock(&sched_dynamic_mutex);
> +}
> +
> +static void __init preempt_dynamic_init(void)
> +{
> +	if (preempt_dynamic_mode == preempt_dynamic_undefined) {
> +		if (IS_ENABLED(CONFIG_PREEMPT_NONE)) {
> +			sched_dynamic_update(preempt_dynamic_none);
> +		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)) {
> +			sched_dynamic_update(preempt_dynamic_voluntary);
> +		} else {
> +			/* Default static call setting, nothing to do */
> +			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT));
> +			preempt_dynamic_mode = preempt_dynamic_full;
> +			pr_info("%s: full\n", PREEMPT_MODE);
> +		}
> +	}
> +}
> +
> +static int __init setup_preempt_mode(char *str)
> +{
> +	int mode = sched_dynamic_mode(str);
> +	if (mode < 0) {
> +		pr_warn("%s: unsupported mode: %s\n", PREEMPT_MODE, str);
> +		return 0;
> +	}
> +
> +	sched_dynamic_update(mode);
> +	return 1;
> +}
> +__setup("preempt=", setup_preempt_mode);
> +
> +#define PREEMPT_MODEL_ACCESSOR(mode) \
> +	bool preempt_model_##mode(void)						 \
> +	{									 \
> +		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
> +		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
> +	}									 \
> +	EXPORT_SYMBOL_GPL(preempt_model_##mode)
> +
> +PREEMPT_MODEL_ACCESSOR(none);
> +PREEMPT_MODEL_ACCESSOR(voluntary);
> +PREEMPT_MODEL_ACCESSOR(full);
> +
> +#else /* !CONFIG_PREEMPT_DYNAMIC */
> +
> +static inline void preempt_dynamic_init(void) { }
> +
> +#endif /* !CONFIG_PREEMPT_DYNAMIC */
> +
>  #ifdef CONFIG_PREEMPT_DYNAMIC
>  
>  #ifdef CONFIG_GENERIC_ENTRY
> @@ -8749,29 +8832,6 @@ EXPORT_SYMBOL(__cond_resched_rwlock_write);
>   *   irqentry_exit_cond_resched <- irqentry_exit_cond_resched
>   */
>  
> -enum {
> -	preempt_dynamic_undefined = -1,
> -	preempt_dynamic_none,
> -	preempt_dynamic_voluntary,
> -	preempt_dynamic_full,
> -};
> -
> -int preempt_dynamic_mode = preempt_dynamic_undefined;
> -
> -int sched_dynamic_mode(const char *str)
> -{
> -	if (!strcmp(str, "none"))
> -		return preempt_dynamic_none;
> -
> -	if (!strcmp(str, "voluntary"))
> -		return preempt_dynamic_voluntary;
> -
> -	if (!strcmp(str, "full"))
> -		return preempt_dynamic_full;
> -
> -	return -EINVAL;
> -}
> -
>  #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
>  #define preempt_dynamic_enable(f)	static_call_update(f, f##_dynamic_enabled)
>  #define preempt_dynamic_disable(f)	static_call_update(f, f##_dynamic_disabled)
> @@ -8782,7 +8842,6 @@ int sched_dynamic_mode(const char *str)
>  #error "Unsupported PREEMPT_DYNAMIC mechanism"
>  #endif
>  
> -static DEFINE_MUTEX(sched_dynamic_mutex);
>  static bool klp_override;
>  
>  static void __sched_dynamic_update(int mode)
> @@ -8807,7 +8866,7 @@ static void __sched_dynamic_update(int mode)
>  		preempt_dynamic_disable(preempt_schedule_notrace);
>  		preempt_dynamic_disable(irqentry_exit_cond_resched);
>  		if (mode != preempt_dynamic_mode)
> -			pr_info("Dynamic Preempt: none\n");
> +			pr_info("%s: none\n", PREEMPT_MODE);
>  		break;
>  
>  	case preempt_dynamic_voluntary:
> @@ -8818,7 +8877,7 @@ static void __sched_dynamic_update(int mode)
>  		preempt_dynamic_disable(preempt_schedule_notrace);
>  		preempt_dynamic_disable(irqentry_exit_cond_resched);
>  		if (mode != preempt_dynamic_mode)
> -			pr_info("Dynamic Preempt: voluntary\n");
> +			pr_info("%s: voluntary\n", PREEMPT_MODE);
>  		break;
>  
>  	case preempt_dynamic_full:
> @@ -8829,20 +8888,13 @@ static void __sched_dynamic_update(int mode)
>  		preempt_dynamic_enable(preempt_schedule_notrace);
>  		preempt_dynamic_enable(irqentry_exit_cond_resched);
>  		if (mode != preempt_dynamic_mode)
> -			pr_info("Dynamic Preempt: full\n");
> +			pr_info("%s: full\n", PREEMPT_MODE);
>  		break;
>  	}
>  
>  	preempt_dynamic_mode = mode;
>  }
>  
> -void sched_dynamic_update(int mode)
> -{
> -	mutex_lock(&sched_dynamic_mutex);
> -	__sched_dynamic_update(mode);
> -	mutex_unlock(&sched_dynamic_mutex);
> -}
> -
>  #ifdef CONFIG_HAVE_PREEMPT_DYNAMIC_CALL
>  
>  static int klp_cond_resched(void)
> @@ -8873,51 +8925,6 @@ void sched_dynamic_klp_disable(void)
>  
>  #endif /* CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
>  
> -static int __init setup_preempt_mode(char *str)
> -{
> -	int mode = sched_dynamic_mode(str);
> -	if (mode < 0) {
> -		pr_warn("Dynamic Preempt: unsupported mode: %s\n", str);
> -		return 0;
> -	}
> -
> -	sched_dynamic_update(mode);
> -	return 1;
> -}
> -__setup("preempt=", setup_preempt_mode);
> -
> -static void __init preempt_dynamic_init(void)
> -{
> -	if (preempt_dynamic_mode == preempt_dynamic_undefined) {
> -		if (IS_ENABLED(CONFIG_PREEMPT_NONE)) {
> -			sched_dynamic_update(preempt_dynamic_none);
> -		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)) {
> -			sched_dynamic_update(preempt_dynamic_voluntary);
> -		} else {
> -			/* Default static call setting, nothing to do */
> -			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT));
> -			preempt_dynamic_mode = preempt_dynamic_full;
> -			pr_info("Dynamic Preempt: full\n");
> -		}
> -	}
> -}
> -
> -#define PREEMPT_MODEL_ACCESSOR(mode) \
> -	bool preempt_model_##mode(void)						 \
> -	{									 \
> -		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
> -		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
> -	}									 \
> -	EXPORT_SYMBOL_GPL(preempt_model_##mode)
> -
> -PREEMPT_MODEL_ACCESSOR(none);
> -PREEMPT_MODEL_ACCESSOR(voluntary);
> -PREEMPT_MODEL_ACCESSOR(full);
> -
> -#else /* !CONFIG_PREEMPT_DYNAMIC */
> -
> -static inline void preempt_dynamic_init(void) { }
> -
>  #endif /* #ifdef CONFIG_PREEMPT_DYNAMIC */
>  
>  /**
> -- 
> 2.31.1
>