Received-SPF: pass (google.com: domain of linux-kernel+bounces-99502-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
References: <20240213055554.1802415-27-ankur.a.arora@oracle.com>
 <65e3cd87.050a0220.bc052.7a29@mx.google.com> <87frx514jz.fsf@oracle.com>
 <12a20651-5429-43df-88d7-9d01ff6212c6@joelfernandes.org>
 <63380f0a-329c-43df-8e6c-4818de5eb371@paulmck-laptop>
 <d956c2e9-492d-4559-b9f9-400f37f523bf@joelfernandes.org>
 <6054a8e0-eb95-45a3-9901-fe2a31b6fe4e@paulmck-laptop>
 <87plw5pd2x.fsf@oracle.com>
 <e36b84bc-09c4-4b2e-bad0-f72530a9b15e@paulmck-laptop>
 <87wmq9mkx2.fsf@oracle.com>
 <36eef8c5-8ecd-4c90-8851-1c2ab342e2bb@paulmck-laptop>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: paulmck@kernel.org
Cc: Ankur Arora <ankur.a.arora@oracle.com>,
        Joel Fernandes
 <joel@joelfernandes.org>, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, peterz@infradead.org,
        torvalds@linux-foundation.org, akpm@linux-foundation.org,
        luto@kernel.org, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, mingo@redhat.com, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, willy@infradead.org, mgorman@suse.de,
        jpoimboe@kernel.org, mark.rutland@arm.com, jgross@suse.com,
        andrew.cooper3@citrix.com, bristot@kernel.org,
        mathieu.desnoyers@efficios.com, geert@linux-m68k.org,
        glaubitz@physik.fu-berlin.de, anton.ivanov@cambridgegreys.com,
        mattst88@gmail.com, krypton@ulrich-teichert.org, rostedt@goodmis.org,
        David.Laight@aculab.com, richard@nod.at, mjguzik@gmail.com,
        jon.grimm@amd.com, bharata@amd.com, raghavendra.kt@amd.com,
        boris.ostrovsky@oracle.com, konrad.wilk@oracle.com
Subject: Re: [PATCH 26/30] sched: handle preempt=voluntary under PREEMPT_AUTO
In-reply-to: <36eef8c5-8ecd-4c90-8851-1c2ab342e2bb@paulmck-laptop>
Date: Mon, 11 Mar 2024 13:09:54 -0700
Message-ID: <87cys0il7x.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?iJkDCCBVhuXs5mP46+Thg8nimggIhVQVETeend+6HkeywKHTVIKesK+uE6Fe?=
 =?us-ascii?Q?45Svtf/TyMI4mdb6UU1bQ93KMU6TXs9myzfztsHScJsHfhULI1ROhcT+xTVl?=
 =?us-ascii?Q?Rt8/1i9T3E1Soog5OGp041uhgvMoY8EQWeZVqOcG83IFHVYvIP109tj34V8Z?=
 =?us-ascii?Q?ARVNRQD0xaW0LVSmtKCHtZ1V8cdF/Ln9jXj/VVPPmIhJyoo2hO66wRIdkTeG?=
 =?us-ascii?Q?FMbmGZQqRatolYsBpL5vzEIHU/H+Y73yIQsEdX2DdqBiyneJstYeO2w6p490?=
 =?us-ascii?Q?eKmku07ESq9GwwqsMvhEWx5sLT8rK+RppaaJ0Qp1ic+LSNKU8xbzhGOSKuhC?=
 =?us-ascii?Q?gClSGzSQYJApyv/2QylCKqi1ynTHtG2stT53gJtLxw58ngGVCfuAEal5ICrY?=
 =?us-ascii?Q?f1v/JRfloieb5lWuUeFoAqmBwfje1ukY+64DQFAK7zyvxWcW8v09tr5vi9KE?=
 =?us-ascii?Q?g911dw7z+bUJsAsBOO0Qz+0/rmlDupm4jPULdUG0aUz9g2z7uVE4t8FZqBuV?=
 =?us-ascii?Q?RmHd1LglvRu/LeeF9ndBNy8rB38kO1rqgcoIKkxyrPjPQmTTV0U1qmt7oagc?=
 =?us-ascii?Q?DpN8DhEKT7J4mx6B/lkZqckkjGaSp0LZGo+p/matr2mcD4z6lfiuQfPmechg?=
 =?us-ascii?Q?0nmhieWNA3bz2OAqUEPqrhXxynqUG6x3EF3n3AABYwsh0JIwIWv4UcKH55zA?=
 =?us-ascii?Q?a7SCGw4LQL/solkqctEiEaT1pFIx3DCgDG/aanqr+D6lVlmgzmIgpQVQ2ebn?=
 =?us-ascii?Q?EwsfLKW4FUskOs6IQyKqxlhyJAzxeMpQgV9cM0hyqhgSwTm5/E4ldjmXrYKA?=
 =?us-ascii?Q?2elewMvbXPARzTR2+TyYVtLekQMLxEFRFDQh070zNIruYM6+KSCgokqtOult?=
 =?us-ascii?Q?ATcErI+FU9194xWymX6Fje67TnbW4tIxRXq/EujJMA+OQ2ZCqgzGLHGMOp16?=
 =?us-ascii?Q?Tf4Oxw14dIbwGMSKd0cIVXbAPjyk3+1SMLqCb4hzvyvaR0DuWLRUBAlF8Mvo?=
 =?us-ascii?Q?dhVzsk/WTvtEV5Rzl4xtOZnpc5nXUxgvlfVVk58ik/QYzBP0vhH1/B3iDalM?=
 =?us-ascii?Q?LC2Q8a/10MLDqC8NIBY0mr7JTHj4QiNqgijxLhO/+nQiGgg94VJ+MZ1ijQv4?=
 =?us-ascii?Q?6wqrsz+3C70WSKXkA9On4Ix6KLNGFWmIV8MgTEA+CTdwt+UzpAD4PYWbKVHT?=
 =?us-ascii?Q?X2kfj+AsWU2yTgRe0q58c2BM88nV4piCe5sb/lhLk3aCvJSd5MNNCUxGOcLJ?=
 =?us-ascii?Q?FHVlgvvFdN/T/3bOqJ4DD+nFt2rjEgwsg4u+cKsg+ywF7KtX6yOlVHnVIpZL?=
 =?us-ascii?Q?Rfylc0ScyC9KsmHp1N/g/gn1g+6s6HjjRn/REbFjGZUd3cdgqP7Fler17HBZ?=
 =?us-ascii?Q?b6aKuS7XuXd7o8RD7zSkyu1us0Fq93M5ImB1HpiN9iubvcnDixhwaujh8l4w?=
 =?us-ascii?Q?G0L0U3aQK+gMPSGXVpv+PpXDr6N2VWB47+do1s6P5nOGhOz6UlQZpmRyCc7c?=
 =?us-ascii?Q?KOt+3JuphXVbvADYSffHzdZQEaddQ1L3/LhPvPlCXCv7gfneFI2egMfWlqGo?=
 =?us-ascii?Q?J9THXrsXnyPdAYOVER4pR08jIDKrIMY8R9MX3kASLLtDFdeggBedsdPTBMim?=
 =?us-ascii?Q?+A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Q3LTdMyePjG72fFoskVIHx4EIz8KfQYKxALkTIzHqjaGUhgRgqJ5F0S+UqYnPBTldSFy5vZcscwmiLrkdgKf4ERPnhUnHGY0dFMT2ZqGL04cNSqgb71hQslQkF9OwTKlZ5SY3cOvjCXSCX7zHecNEDvaZRQ4hwvjDAUqD3KhhJeH3xMn1/96mgqe0XOCEVKgk5vcrl1VtIAefOchk+4lR2NSOWaEwA+xSZNgHOAIttpjRyv0h/B9PCNb/4RoCwt7xs3zxU8E3PKkkHUxOJyB20ujbUTDhlmp7+2luu6FQYioeMroPpYpqPPb3JUNbwoD/NxgWhlrgmetCqJtpfgrWPI+1lI0sSwTke21UGERNgiizSvuTe/90jnJLRMjE+/p4HipTQp3qhEwl8oIgWlEsvTtkfboCXhw4CPwBUFYzKFxqcBcX5T4BDtz75OlGJKBeH9huOZT0mswEs4OHVMdECxw/2KplJk1Ak5Hb7KGUnPeFMYHAfEUCAf+2QoUbH6lg0D1Hg2esrAv3DTIVr6SDGUK7l6GQ2hZGpATY80hnzcSxwPxC2Ts8eskgIIBjhWJERvpSGUktnByZml3iPSNcDf1fTLecoYNvonueU2+DHg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e3f1fef6-98e0-49c7-1065-08dc42072454
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 20:09:22.3298
 (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: 1eUWwXr3PWIJqs9yyntCqG/8tgZO7w4R27Dg7NKbjbh7F0wkL7hIA81nqOk6qQjbtbfztAKsm9WHnguApcVlolIYDTZcLbUR9NfJJMlTKKQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB7315
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-11_11,2024-03-11_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 spamscore=0
 mlxlogscore=975 bulkscore=0 phishscore=0 malwarescore=0 mlxscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403110154
X-Proofpoint-GUID: nBs9r2HqfPcmUp1Z5BOQsj6ly1pgTn_2
X-Proofpoint-ORIG-GUID: nBs9r2HqfPcmUp1Z5BOQsj6ly1pgTn_2


Paul E. McKenney <paulmck@kernel.org> writes:

> On Sun, Mar 10, 2024 at 09:50:33PM -0700, Ankur Arora wrote:
>>
>> Paul E. McKenney <paulmck@kernel.org> writes:
>>
>> > On Thu, Mar 07, 2024 at 08:22:30PM -0800, Ankur Arora wrote:
>> >>
>> >> Paul E. McKenney <paulmck@kernel.org> writes:
>> >>
>> >> > On Thu, Mar 07, 2024 at 07:15:35PM -0500, Joel Fernandes wrote:
>> >> >>
>> >> >>
>> >> >> On 3/7/2024 2:01 PM, Paul E. McKenney wrote:
>> >> >> > On Wed, Mar 06, 2024 at 03:42:10PM -0500, Joel Fernandes wrote:
>> >> >> >> Hi Ankur,
>> >> >> >>
>> >> >> >> On 3/5/2024 3:11 AM, Ankur Arora wrote:
>> >> >> >>>
>> >> >> >>> Joel Fernandes <joel@joelfernandes.org> writes:
>> >> >> >>>
>> >> >> >> [..]
>> >> >> >>>> IMO, just kill 'voluntary' if PREEMPT_AUTO is enabled. There is no
>> >> >> >>>> 'voluntary' business because
>> >> >> >>>> 1. The behavior vs =none is to allow higher scheduling class to preempt, it
>> >> >> >>>> is not about the old voluntary.
>> >> >> >>>
>> >> >> >>> What do you think about folding the higher scheduling class preemption logic
>> >> >> >>> into preempt=none? As Juri pointed out, prioritization of at least the leftmost
>> >> >> >>> deadline task needs to be done for correctness.
>> >> >> >>>
>> >> >> >>> (That'll get rid of the current preempt=voluntary model, at least until
>> >> >> >>> there's a separate use for it.)
>> >> >> >>
>> >> >> >> Yes I am all in support for that. Its less confusing for the user as well, and
>> >> >> >> scheduling higher priority class at the next tick for preempt=none sounds good
>> >> >> >> to me. That is still an improvement for folks using SCHED_DEADLINE for whatever
>> >> >> >> reason, with a vanilla CONFIG_PREEMPT_NONE=y kernel. :-P. If we want a new mode
>> >> >> >> that is more aggressive, it could be added in the future.
>> >> >> >
>> >> >> > This would be something that happens only after removing cond_resched()
>> >> >> > might_sleep() functionality from might_sleep(), correct?
>> >> >>
>> >> >> Firstly, Maybe I misunderstood Ankur completely. Re-reading his comments above,
>> >> >> he seems to be suggesting preempting instantly for higher scheduling CLASSES
>> >> >> even for preempt=none mode, without having to wait till the next
>> >> >> scheduling-clock interrupt. Not sure if that makes sense to me, I was asking not
>> >> >> to treat "higher class" any differently than "higher priority" for preempt=none.
>> >> >>
>> >> >> And if SCHED_DEADLINE has a problem with that, then it already happens so with
>> >> >> CONFIG_PREEMPT_NONE=y kernels, so no need special treatment for higher class any
>> >> >> more than the treatment given to higher priority within same class. Ankur/Juri?
>> >> >>
>> >> >> Re: cond_resched(), I did not follow you Paul, why does removing the proposed
>> >> >> preempt=voluntary mode (i.e. dropping this patch) have to happen only after
>> >> >> cond_resched()/might_sleep() modifications?
>> >> >
>> >> > Because right now, one large difference between CONFIG_PREEMPT_NONE
>> >> > an CONFIG_PREEMPT_VOLUNTARY is that for the latter might_sleep() is a
>> >> > preemption point, but not for the former.
>> >>
>> >> True. But, there is no difference between either of those with
>> >> PREEMPT_AUTO=y (at least right now).
>> >>
>> >> For (PREEMPT_AUTO=y, PREEMPT_VOLUNTARY=y, DEBUG_ATOMIC_SLEEP=y),
>> >> might_sleep() is:
>> >>
>> >> # define might_resched() do { } while (0)
>> >> # define might_sleep() \
>> >>         do { __might_sleep(__FILE__, __LINE__); might_resched(); } while (0)
>> >>
>> >> And, cond_resched() for (PREEMPT_AUTO=y, PREEMPT_VOLUNTARY=y,
>> >> DEBUG_ATOMIC_SLEEP=y):
>> >>
>> >> static inline int _cond_resched(void)
>> >> {
>> >>         klp_sched_try_switch();
>> >>         return 0;
>> >> }
>> >> #define cond_resched() ({                       \
>> >>         __might_resched(__FILE__, __LINE__, 0); \
>> >>         _cond_resched();                        \
>> >> })
>> >>
>> >> And, no change for (PREEMPT_AUTO=y, PREEMPT_NONE=y, DEBUG_ATOMIC_SLEEP=y).
>> >
>> > As long as it is easy to restore the prior cond_resched() functionality
>> > for testing in the meantime, I should be OK.  For example, it would
>> > be great to have the commit removing the old functionality from
>> > cond_resched() at the end of the series,
>>
>> I would, of course, be happy to make any changes that helps testing,
>> but I think I'm missing something that you are saying wrt
>> cond_resched()/might_sleep().
>>
>> There's no commit explicitly removing the core cond_reshed()
>> functionality: PREEMPT_AUTO explicitly selects PREEMPT_BUILD and selects
>> out PREEMPTION_{NONE,VOLUNTARY}_BUILD.
>> (That's patch-1 "preempt: introduce CONFIG_PREEMPT_AUTO".)
>>
>> For the rest it just piggybacks on the CONFIG_PREEMPT_DYNAMIC work
>> and just piggybacks on (!CONFIG_PREEMPT_DYNAMIC && CONFIG_PREEMPTION):
>>
>> #if !defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)
>> 	/* ... */
>> #if defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
>> 	/* ... */
>> #elif defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
>> 	/* ... */
>> #else /* !CONFIG_PREEMPTION */
>> 	/* ... */
>> #endif /* PREEMPT_DYNAMIC && CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
>>
>> #else /* CONFIG_PREEMPTION && !CONFIG_PREEMPT_DYNAMIC */
>> static inline int _cond_resched(void)
>> {
>> 	klp_sched_try_switch();
>> 	return 0;
>> }
>> #endif /* !CONFIG_PREEMPTION || CONFIG_PREEMPT_DYNAMIC */
>>
>> Same for might_sleep() (which really amounts to might_resched()):
>>
>> #ifdef CONFIG_PREEMPT_VOLUNTARY_BUILD
>>        /* ... */
>> #elif defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
>>       /* ... */
>> #elif defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
>>       /* ... */
>> #else
>> # define might_resched() do { } while (0)
>> #endif /* CONFIG_PREEMPT_* */
>>
>> But, I doubt that I'm telling you anything new. So, what am I missing?
>
> It is really a choice at your end.
>
> Suppose we enable CONFIG_PREEMPT_AUTO on our fleet, and find that there
> was some small set of cond_resched() calls that provided sub-jiffy
> preemption that matter to some of our workloads.  At that point, what
> are our options?
>
> 1.	Revert CONFIG_PREEMPT_AUTO.
>
> 2.	Revert only the part that disables the voluntary preemption
> 	semantics of cond_resched().  Which, as you point out, ends up
> 	being the same as #1 above.
>
> 3.	Hotwire a voluntary preemption into the required locations.
> 	Which we would avoid doing due to upstream-acceptance concerns.
>
> So, how easy would you like to make it for us to use as much of
> CONFIG_PREEMPT_AUTO=y under various possible problem scenarios?

Ah, I see your point. Basically, keep the lazy semantics but -- in
addition -- also provide the ability to dynamically toggle
cond_resched(), might_reshed() as a feature to help move this along
further.

So, as I mentioned earlier, the callsites are already present, and
removing them needs work (with livepatch and more generally to ensure
PREEMPT_AUTO is good enough for the current PREEMPT_* scenarios so
we can ditch cond_resched()).

I honestly don't see any reason not to do this -- I would prefer
this be a temporary thing to help beat PREEMPT_AUTO into shape. And,
this provides an insurance policy for using PREEMPT_AUTO.

That said, I would like Thomas' opinion on this.

> 3.	Hotwire a voluntary preemption into the required locations.
> 	Which we would avoid doing due to upstream-acceptance concerns.

Apropos of this, how would you determine which are the locations
where we specifically need voluntary preemption?

> Yes, in a perfect world, we would have tested this already, but I
> am still chasing down problems induced by simple rcutorture testing.
> Cowardly of us, isn't it?  ;-)

Cowards are us :).

--
ankur