Re: [PATCH v2 12/35] sched: separate PREEMPT_DYNAMIC config logic

[Ankur Arora]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:

> 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 ?

Now that you mention it, it does seem quite odd.

Assuming I keep the separation maybe it makes sense to make the runtime
configuration it's own configuration option, say CONFIG_PREEMPT_RUNTIME.

And, PREEMPT_AUTO and PREEMPT_DYNAMIC could select it?


>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
>> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
>> ---
>>  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
>>


--
ankur