Received-SPF: pass (google.com: domain of linux-kernel+bounces-207224-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
References: <20240528003521.979836-1-ankur.a.arora@oracle.com>
 <20240528003521.979836-14-ankur.a.arora@oracle.com>
 <20240528162725.GH26599@noisy.programming.kicks-ass.net>
 <87ttif641i.fsf@oracle.com>
 <20240606115101.GC8774@noisy.programming.kicks-ass.net>
 <87ikymw1g2.fsf@oracle.com>
 <20240606173218.GH8774@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 13/35] sched: allow runtime config for PREEMPT_AUTO
In-reply-to: <20240606173218.GH8774@noisy.programming.kicks-ass.net>
Date: Sat, 08 Jun 2024 17:46:26 -0700
Message-ID: <87frtnt02l.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?hJUPwf1t/usCSwOKo6juGD9Zo1+FDgNB8Lld0NXO0dParbQf0ltbmDmcUwFu?=
 =?us-ascii?Q?hOVG79caKDPjfztHF2C7LrE5capHhT8stnP6eI7lhu0SNW9Lf8RLQ/dcBXmm?=
 =?us-ascii?Q?DKPBL2apG69xyQ9hmItEFqeeP9x4qyYTM8SuJAQF5e9IVzVtFgvoHpoUj2W3?=
 =?us-ascii?Q?OZyMGwqODIBm5hcALW7R0Uoe2jh7iaX6boHWH1sOA8iPr2qb3lV0k2NzDv3r?=
 =?us-ascii?Q?EXIQ3WtmQ88WFdBl7REXlm1xiP8TMhooRG1tBYNG76GaUwLM6EFzuwQgJFcV?=
 =?us-ascii?Q?c6E3h2mwAYRy+j9s2qAQWWzbDp+ABL0r8uDKQwac144dgjjf8GxF4AfLRCem?=
 =?us-ascii?Q?0phN0rFNv6UUHZ5J9IG76tTCOhZ+viBKIFYndv38zMMnDLCKLKY8QHno6XR5?=
 =?us-ascii?Q?k8xTtGPUYZySgsXMWZY2BwIHYVOoEhUMrxdWluNvs37xl66jimUaP6ErRGpA?=
 =?us-ascii?Q?MD9QdkPvwbuZonzztt4enYGOkfvavxUCZpfFu+4g9ozYtOAwRBdz2BoPJXDo?=
 =?us-ascii?Q?kwa/O7y2VGwyHRTJnPXHqctEl+PZYcCnJdURw+KGvN49Pd7xWcI6xW/ouW20?=
 =?us-ascii?Q?xTXuzcgATY6R0CCAG9MIQgDXugVfMgqF3PaE6sUF2AkY9ranaYPjZvoyG0rF?=
 =?us-ascii?Q?ZnhZFVOzWi8O/4Piz9kvuom4t83aXyqAzQnd132PF5l+r1pLFcNocU8RI9Z7?=
 =?us-ascii?Q?fNYnAmrsHQ+DSTgmTrc2uFE/ks1KdSupFwUIyqr95PgZiQgaKQMwN/+rZJdr?=
 =?us-ascii?Q?W7s/MXN2NCb9WQoxrvjDJswZIE66xtaxoFsGMPoMM4cU6GPQew3mH7XnMsF1?=
 =?us-ascii?Q?2N5cTgFp3AJe92aGV/roNPW89JMv0DRECKOfkPxJ+KbBRLrRtRS++Y1kEJJW?=
 =?us-ascii?Q?sZux5aJHW0Jo85a5/ZOEnKUL2FHAU/Fi/1/w2EHl4Xuz0FiXQ981LjsNO2gI?=
 =?us-ascii?Q?JNqt83y+TpRz6eGY9zq2CkwXACSz3SOe1XZOuaR9dFh5wPyOcmJqqrrYBxF2?=
 =?us-ascii?Q?fP89yrO69lUR4bTOSHyLJ56Sr8wdMkvpB2th3cIBxlUHdU++xtvjuaeJpL2c?=
 =?us-ascii?Q?kEdB9WkcwoCSLb+PAsKw1uWXXjPrvgOXf/ImVIDE4SqXBfvKkjV9pnTNCALG?=
 =?us-ascii?Q?/F8ETajXtnqBeInu9gyifWAk5Hp6FVNUQa27dxQit6+F5yZX0A1MBJANt4kl?=
 =?us-ascii?Q?kF3YA9AdDMbNe/S9O1a+3EGqj24dPLyqUt0lQtsx2xwcdqYVc8SIEwNbupFi?=
 =?us-ascii?Q?+MaLcX76DdpzAlgIZP6aMP6AesHMTj1MVsUMDEgEGBtx39+h1vnfedaHe4bT?=
 =?us-ascii?Q?Ko8iWlVnAF6RVC1oPC0XNfC/+TLCfkGa3lVOmOWXIHQLCjGI7HO91Tfj3SNA?=
 =?us-ascii?Q?/wIkeINxg3sF/Ji+LQ8hb7YciLroRB0zCfKNGxfnWs481VxirYBst0aalMK+?=
 =?us-ascii?Q?zFU+CTzfr9zGryvQ9EgVEtKCKvKczgyIOiw4mATfmbloEj6ErJMndNQGdZhw?=
 =?us-ascii?Q?c7o3ekcIBW8gTQyEyXC0jwVgiBrfsE4iuPpSSuHfD3+J2ZXGeienPCV9TeWA?=
 =?us-ascii?Q?cXq5ehM4G50B40PXwgfYTlyNuVXGLu1LAfFWt6DEiy44FUgZZPr7mNTn43Mv?=
 =?us-ascii?Q?Yw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	dn4ALchQh/BULkU7NkkBptTSyJi9vzZXt+mewgCJNVrshAHlQ8WzanDAuOCwtltJs3B+zO97C90e/EHoBKEJbdOWEaLc0Zcov3zbzK0Rm3e4CTWFZiMc5ZJJ+NbU3e4V44hL64hDG/H2bqVYaRdoN7n4WJWSq/bIAGQRL1hlZ3zqGazkiIEmr4rr4hHQSqyKagogkIJ2Zq5rNVGx8TmAOZX0y3FrRP6aGZ9G+f1BjMasARVJhftF9efDWpWK86WzwsjpX2Vcm4WoXHZ0p8sMr15ny/w+pPbwwaWCQlmgWoxXfSYeitoJlu0FC/65+f1sp0Nl6K+hlgHW8XBmMNLTafffpMkvAVkwY6j9gCKrecTYlTKL1K0yyKKjr6uyY2QXhIeZLpX5QY/WN35rAKqOLp4Dyz+/XInN4nPFUdH63ekWS+etqQ8/rgkQdY6fVVN0BrH+jBvXXCOesebN1rNr6Xez9PVYigdwwFTJ5mHvlafHRFRaJYtpPQdKpRs3SJJzG0pxA7jrsRX+ZQqdI6D5S4qot4LeM/f7mCxD/yZ3ufQzlWFWzkC7RMXxPCuigRn7pyvQPQ5yq9O+JogUo9c8RBvtGTyBjrzYPd+NoevJSg8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cc43c600-ff5b-42c4-6f0f-08dc881d9923
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2024 00:46:28.6385
 (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: AhXwemRvxonG7Hj34OoER5pSDhda6VzdddkuMGoEW+OP6zZa4rpYeSUrV8litEW1E8Gois5wf4vhBv/sH9nogiI3Ia73wT3qztxCCYWh4TY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7161
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-08_16,2024-06-06_02,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0
 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0 mlxlogscore=990
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406090004
X-Proofpoint-GUID: NYqOvPr7aDOE7RvaYB3SmaijzxZP1Nul
X-Proofpoint-ORIG-GUID: NYqOvPr7aDOE7RvaYB3SmaijzxZP1Nul


Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Jun 06, 2024 at 08:11:41AM -0700, Ankur Arora wrote:
>>
>> Peter Zijlstra <peterz@infradead.org> writes:
>>
>> > On Thu, May 30, 2024 at 02:29:45AM -0700, Ankur Arora wrote:
>> >>
>> >> Peter Zijlstra <peterz@infradead.org> writes:
>> >>
>> >> > On Mon, May 27, 2024 at 05:34:59PM -0700, Ankur Arora wrote:
>> >> >> Reuse sched_dynamic_update() and related logic to enable choosing
>> >> >> the preemption model at boot or runtime for PREEMPT_AUTO.
>> >> >>
>> >> >> The interface is identical to PREEMPT_DYNAMIC.
>> >> >
>> >> > Colour me confused, why?!? What are you doing and why aren't just just
>> >> > adding AUTO to the existing DYNAMIC thing?
>> >>
>> >> You mean have a single __sched_dynamic_update()? AUTO doesn't use any
>> >> of the static_call/static_key stuff so I'm not sure how that would work.
>> >
>> > *sigh*... see the below, seems to work.
>>
>> Sorry, didn't mean for you to have to do all that work to prove the
>> point.
>
> Well, for a large part it was needed for me to figure out what your
> patches were actually doing anyway. Peel away all the layers and this is
> what remains.
>
>> I phrased it badly. I do understand how lazy can be folded in as
>> you do here:
>>
>> > +	case preempt_dynamic_lazy:
>> > +		if (!klp_override)
>> > +			preempt_dynamic_disable(cond_resched);
>> > +		preempt_dynamic_disable(might_resched);
>> > +		preempt_dynamic_enable(preempt_schedule);
>> > +		preempt_dynamic_enable(preempt_schedule_notrace);
>> > +		preempt_dynamic_enable(irqentry_exit_cond_resched);
>> > +		preempt_dynamic_key_enable(preempt_lazy);
>> > +		if (mode != preempt_dynamic_mode)
>> > +			pr_info("Dynamic Preempt: lazy\n");
>> > +		break;
>> >  	}
>>
>> But, if the long term goal (at least as I understand it) is to get rid
>> of cond_resched() -- to allow optimizations that needing to call cond_resched()
>> makes impossible -- does it make sense to pull all of these together?
>
> It certainly doesn't make sense to add yet another configurable thing. We
> have one, so yes add it here.
>
>> Say, eventually preempt_dynamic_lazy and preempt_dynamic_full are the
>> only two models left. Then we will have (modulo figuring out how to
>> switch over klp from cond_resched() to a different unwinding technique):
>>
>> static void __sched_dynamic_update(int mode)
>> {
>>         preempt_dynamic_enable(preempt_schedule);
>>         preempt_dynamic_enable(preempt_schedule_notrace);
>>         preempt_dynamic_enable(irqentry_exit_cond_resched);
>>
>>         switch (mode) {
>>         case preempt_dynamic_full:
>>                 preempt_dynamic_key_disable(preempt_lazy);
>>                 if (mode != preempt_dynamic_mode)
>>                         pr_info("%s: full\n", PREEMPT_MODE);
>>                 break;
>>
>> 	case preempt_dynamic_lazy:
>> 		preempt_dynamic_key_enable(preempt_lazy);
>> 		if (mode != preempt_dynamic_mode)
>> 			pr_info("Dynamic Preempt: lazy\n");
>> 		break;
>>         }
>>
>>         preempt_dynamic_mode = mode;
>> }
>>
>> Which is pretty similar to what the PREEMPT_AUTO code was doing.
>
> Right, but without duplicating all that stuff in the interim.

Yeah, that makes sense. Joel had suggested something on these lines
earlier [1], to which I was resistant.

However, the duplication (and the fact that the voluntary model
was quite thin) should have told me that (AUTO, preempt=voluntary)
should just be folded under PREEMPT_DYNAMIC.

I'll rework the series to do that.

That should also simplify RCU related choices which I think Paul will
like. Given that the lazy model is meant to eventually replace
none/voluntary, so PREEMPT_RCU configuration can just be:

--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -18,7 +18,7 @@ config TREE_RCU

 config PREEMPT_RCU
        bool
-       default y if PREEMPTION
+       default y if PREEMPTION && !PREEMPT_LAZY


Or, maybe we should instead have this:

--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -18,7 +18,7 @@ config TREE_RCU

 config PREEMPT_RCU
        bool
-       default y if PREEMPTION
+       default y if PREEMPT || PREEMPT_RT
        select TREE_RCU

Though this would be a change in behaviour for current PREEMPT_DYNAMIC
users.

[1] https://lore.kernel.org/lkml/fd48ea5c-bc74-4914-a621-d12c9741c014@joelfernandes.org/

Thanks
--
ankur