Received-SPF: pass (google.com: domain of linux-kernel+bounces-195178-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
References: <20240528003521.979836-1-ankur.a.arora@oracle.com>
 <20240528003521.979836-13-ankur.a.arora@oracle.com>
 <20240528162534.GG26599@noisy.programming.kicks-ass.net>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ankur Arora <ankur.a.arora@oracle.com>, 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
In-reply-to: <20240528162534.GG26599@noisy.programming.kicks-ass.net>
Date: Thu, 30 May 2024 02:30:25 -0700
Message-ID: <87r0dj640e.fsf@oracle.com>
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?7BMy4FmQBRXeqqTACrdPbVWNngckZ46459PpoYbNMKjLuI0fUq2nDcbByyvE?=
 =?us-ascii?Q?qi2jvqMCW1/1AZvimjrpyFkmAJ1tN1GGK4zGuePIX0ARz/X3ZbkIaJKzCs1l?=
 =?us-ascii?Q?lZweT/M0XG23+hfjx0O4dMehiLHSTfPniiZgwsGcRwkBcDnzO/CIbauQP0SX?=
 =?us-ascii?Q?8u/xIiJKw6AfwZsXDaZe5yiO+mCpvpUNf6Bhkj9bJkIGP0pe2kIlp6jLPaOa?=
 =?us-ascii?Q?XuzWmE2xjk5qzc4InybMf8qMoQ8hjVADcXEyHxsdgCpIAAttNcWoqLDzDIyy?=
 =?us-ascii?Q?jN81wqShfUoO/v5KurqZP4+IQFW8zCZ42z9RiYQb9sl6FgSZhVzUQrTutGxD?=
 =?us-ascii?Q?TO8ecXeCuya03wCITclij7J6/4LMTRFbjuPAB74EffYd4ua2TWCZXycvC8/P?=
 =?us-ascii?Q?fYhE1dZWOSCER0Yxp+buv73cf9a34y/tG5VKkaEya68hXWiJz+ixwgj1NxSL?=
 =?us-ascii?Q?iN070t//4BDKamNJ8NmNW5mi4ofHxLDEtDg+FuEp8b3K7Y2EcCqHO5BWB6k0?=
 =?us-ascii?Q?L3eyIzBjLV6jcsLsvhDGK8Xyy5gDGUnrGtXuKUi5LdshWIYIzgBShslQoqgw?=
 =?us-ascii?Q?UgjfcPTHl8phwbtBBzO7wNiGzEf2mahxgGupJzSRB9zHlxqX4fOEwBANWdRE?=
 =?us-ascii?Q?ChRcODk/qzvwohfrGMhsTngndnNoCUFOoY+TT+GDFgL7EKDHi4UAXgeLLtYn?=
 =?us-ascii?Q?1lfhR5OCX5tWZ0VBWjEvrG3SJCEeHXM7CPLg0dAxW3murKxZj+FPRyCF1Vqo?=
 =?us-ascii?Q?l1FjQqQYItuSNeEQm2o6vpVWot169smrwHlyqWsd7FxkwwkvLd4vs6MCf/jJ?=
 =?us-ascii?Q?eQul5quE6wSbhEoRpKdmW3khvVQ0kUac/OG+JgwDUNvcN6vnSZazmXDA0X4F?=
 =?us-ascii?Q?ESgm0suvTaFhhLvMLquYF4lYMtlXJ08/DCuQXmP2HI28j6EI/1Mv0sKhcYf+?=
 =?us-ascii?Q?sRxgAJ+/VMKvDz12Z6xa/EdYLJpgAI/WXam9nQ1pJtv0K9vsqFQfLdR7AT4Z?=
 =?us-ascii?Q?xHA4VWQs39SR1Bee63mjyNN7wJWqIdCcVNlIG+eA24GeGTMoHAewWOJR7aTh?=
 =?us-ascii?Q?5o3VNcXc+E3g4bmABgt/KOllVuCWmHyOUu2b2+mZGbu532fByqfjTulfdfzC?=
 =?us-ascii?Q?ZioMIs6SFJU5615jtKJ0gdnHHOFBWvUi+B/EF07F0n9AkbZQqH2pbJZh5xjV?=
 =?us-ascii?Q?9nBhWzb30hE6Wu0jJJ+R7uTFLyHkFnwbboHbC0t4T6h3Ea7i5Ps1Ir9Or8zl?=
 =?us-ascii?Q?m5rb4dZ6Ak4gHqGoBYIR1BCldRCIoO/gaq7+astXwcnRik7DjhrMm48HZ/xX?=
 =?us-ascii?Q?q0E8YOmbPULIwO6AulDMG19jnHJvSGpAw9ar/zJhMEo+TsE0JDLJO840hGqh?=
 =?us-ascii?Q?qFTKUMGeYLWBufDyZbcxfDlNLtrviUFqnctwuMdq4LJCnkeq+9413fk+JW/M?=
 =?us-ascii?Q?QhAinMTULcNbcA6zTeX/k9hVvSTC1lYqFVsGm2mhy8eiPIraOBghfJVUwgdw?=
 =?us-ascii?Q?24ZTpX7ZkUF2vPVRIy5SNEnNvhax75RrhJpsHluT9EesOa7sVFCE70RUlqJ/?=
 =?us-ascii?Q?bULlnbcCWkDJIrhWrs8dZKM+H2Jl1WJFdWP1B8uL8J5vaIrVJoXjUHRgFzVl?=
 =?us-ascii?Q?tQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Pq5QFdfVgH2AaC4p4ern+g8qZ52eoP8+pporgwUurgNe+eu9h6znI8a5ZHfwUDCJWHkpmPZOPeP6shtYK9WQW5dcx7UFkktp4I4/GEM0X7JiGIOLgSiaK9dfiRsnVVQfcgiLT7M8AgoFH6rOE8eurAydMB05ehjL+uT3UwM6szMPQ36ico8gIJOb9ERfLMgMP41XgMT7Dtxx1bos+koj7Y9bpOfWYuPFpR0vv+7MTGRrUkxFeLkjP9li39TEg1MhiWrONDbMBKUjSCLx6VP7fRpuiPBeT2+9bluFRVaDfz30d7umGHtxtjgN4UIzGHv+EE9xwO4XY0RpMLWfPC6MbRh4E4jlvzXXlANUSVY748/LypMl5mhfTuwzrOGcz6SOeQ/M+9sQG8C3LTGU6LknhEfzVJXIQxaT2J/DoZdDZsq0HePE7WRUmYKYdGr1/hfE7UgQDm9X60dqGGN1pSTCYnrawieYVeYfKqDAnT8CsOKfN75kmNmgHTRjaQpBuHRHyMxxypJVpzJa9MLraHtjWc+60humLl6ufrvDxk7X7PPlpSqbwQRcZaNVggN/NOWIyRktsi5X344oYb/VoTcu84Ci9QbANU5ZJC5HSFoJkyI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f9b5899c-ba13-48b1-da41-08dc808b276b
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 09:30:33.5082
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kP/NEsZwTwfVd9FKK6mOzQLB03JRO21pKzDi0WYAubXxzWmCr6kyJdRBky4rat3kYZ84txtgs6RVwL0RDJWKyLA7g+vkoWxC2Gv4lt5Lzi0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6823
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-30_07,2024-05-28_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0
 phishscore=0 bulkscore=0 malwarescore=0 adultscore=0 mlxscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405300071
X-Proofpoint-GUID: DEa15Z2qrgI14iZl-z367oWCFVNQ5oz_
X-Proofpoint-ORIG-GUID: DEa15Z2qrgI14iZl-z367oWCFVNQ5oz_


Peter Zijlstra <peterz@infradead.org> 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@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
>>


--
ankur