Received-SPF: pass (google.com: domain of linux-kernel+bounces-73749-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: <20240213055554.1802415-1-ankur.a.arora@oracle.com>
 <20240213055554.1802415-4-ankur.a.arora@oracle.com>
 <ZczJU8uZdbRKvcAE@FVFF77S0Q05N> <87il2qmksh.fsf@oracle.com>
 <ZdNJ5x1XmRZwa0Zp@FVFF77S0Q05N>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Ankur Arora <ankur.a.arora@oracle.com>, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, peterz@infradead.org,
        torvalds@linux-foundation.org, paulmck@kernel.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, 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,
        Arnd Bergmann
 <arnd@arndb.de>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 03/30] thread_info: tif_need_resched() now takes
 resched_t as param
In-reply-to: <ZdNJ5x1XmRZwa0Zp@FVFF77S0Q05N>
Date: Tue, 20 Feb 2024 14:09:38 -0800
Message-ID: <87le7ekct9.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?gCsEry4mwOlSl6945GqaaAE7hB/axJ4zylWRUo4TxpUm6iMdSLD9kN5UNsAe?=
 =?us-ascii?Q?L7vkU7ibaiVBbpCrRLaN34qF2AM2yJRtO5xm48pkltYea8cJakrk27sKFzrI?=
 =?us-ascii?Q?WeDTtp5dvngVqiSh07aQ0RQpB8Rus0AZN9bQ7bA1Av12YH9YHYsmvSSZxAIX?=
 =?us-ascii?Q?piXVt1j78/+mjmqnACC3LYzxzWMx+nAUoFb9Ii98C4t3j9BovT8uBMx4TFns?=
 =?us-ascii?Q?XqQgBcpgUe+ItHoPVDBJLeosdTqMBZMMozqPRlA4wjwGcWqLoJGrsk3PNMz9?=
 =?us-ascii?Q?ZpuNuUiKITc2vZhRos1SJCp1O9gVQCFKuJhoxg33Q11iOol2+dG2RgMDNeBV?=
 =?us-ascii?Q?QuA7ZTW5i3rxu+snWyOGDw01RKhSwCukR1fV2Ld/z65q3beg5cHrBqwdULo+?=
 =?us-ascii?Q?JeNsSqKx3+SepCvKrT1EKX6cupbWaZ4Bmq13/W0Qdo3lVtTIqeQz2c4RTlOF?=
 =?us-ascii?Q?pGowC6ih6deeCxW6UzMWvgWqgDP1UZ+JcLi83zXx5O1Z3JEWA8rY1DhNQNe+?=
 =?us-ascii?Q?Cd664F5gb5ieiKC1GBjauhxT96je4wC/lNIWSCKueg0nsMqtgrXdVpax8oM0?=
 =?us-ascii?Q?97xNJ4/g1b0LNdFUJOPkc/5VIOgpN569suYN4bnLrrvFwXrfEP0de+cD/PQv?=
 =?us-ascii?Q?kN6dU08vupfCVd8sudnaI3xuZiNH6a+Xj4JfBRdqrp6MjVeKihiwy0EoG1J9?=
 =?us-ascii?Q?/7sLRK6+eEK4M0cp+DLf1c25o+zKW2vTaMBK7lVLkV4S75tk4qw3RBiyTZCs?=
 =?us-ascii?Q?cEVR/u2xFKuDw7BhcRYWO6zmW36/AOPurKemIYp1basxaVOd7YaQsWw8mOKj?=
 =?us-ascii?Q?veuJDm9J5BjHcAMK5giHbEj2QveL1dVSj6xNJ2Ab6jLAYIEw/KdpKJ7nvRJg?=
 =?us-ascii?Q?rPnW4yzOJpDviw1lTaCtms3XeyLOzCIpuBaMum0k3vc12vYzOlmCYxQmrGEG?=
 =?us-ascii?Q?WxK8HNyNgLLz9VSiSJ23IcOvTv8exYQ/gDBYSEuTWbdFKo94TjcQcGc9HLoT?=
 =?us-ascii?Q?22Kr75LOrSMRBH1tx6WmTYjXi+hjsjO/h0oYfxIzc6K7Oqj1oBxj1z9mo+LD?=
 =?us-ascii?Q?MFRhzAOYgR5lfteDcYDFIWKntdHemv7/bB7cPZeYqPeo5YBv3+6x4OOfQwDk?=
 =?us-ascii?Q?gLtkP5bOLQsAxXrqK1LX123pH7islIxFQLN1IDREMmCbH4YPNpL9OMtBN08c?=
 =?us-ascii?Q?XhOVI9BAZAu2Z55zOZHAHRc7rZBv1tNxPSqQKXnrH6rsVlJpRBHDYpz4KDnS?=
 =?us-ascii?Q?I2rR0wtv7T8lenxziFWERVCz58mIpZ9BdsRlPPEplSj5IYzRs5CFEc9qS3OL?=
 =?us-ascii?Q?lTYYReqoxAb7eXhqNj21qDDAv7x0p/0ETJSEAJAgg8/GyoinkVyIfxfG90ni?=
 =?us-ascii?Q?ztyuQ8MfW2lvsjuEKSx8UNmT1oRIX40QrT3hYMnUaFyANp60O63xmDRmZHmg?=
 =?us-ascii?Q?4Ehi/VXuOe+gb7qvCJbI0AxjQarIwDaVE4X5OHL3vNqEnSjAALIjKfjs24wF?=
 =?us-ascii?Q?L8NwTI5GVqr93OGlGaK/B3V7SpFF0GXL3/XHhFls8nF9P9Kz3D60+obCiCip?=
 =?us-ascii?Q?07X6gkijQHxy8ABdWYvHuuvujc4KYB2I4Wo7hpZDJaSIHryqM///P/+yoBDm?=
 =?us-ascii?Q?kQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	lM5c3CCfIoDydYiIt18kj6p+cnVbafkPDmWOg+nssl1/DxRPOiYFbDSBOQlzy/Te2nVioyJVNTN75eEooRi7Y5j2cqvf6zWC9q2r4okamlSxm9NO/QeQk8Onl0mo1ODKvR9p8tN3/Gvujntrn2ivHyeHHWM7KbFkzzICX8I2cSKgXwwgeGMm9u98xRw7z1ZvwXCp7hQMKxm0BjdiZFs9VYqg+lswUzBgxA8VjT+MoxPuIqkE7PqSXXA1mJBJ8UpY9PS1os2ar3pNO9WFd0QcJUYEYgS6vlIDTVt0w2L2WuHULR+H3lWt7xJ0Z4j3/x2e9rfLRVdOzHw1kSTzHOKAedue3SE7jTWwai6046n2knnWLCnS9XdarLz8CWqtVvOEepXhxxnrMVaSkKTGW7zGWug6C2DdsLO0nXsOfTvOqPdM++kNMIpYHtX2ltMd3M4yeYITxXhzfnIYMMbnuafqQLC+wkVfXwqQYEqIjcbOEpj/u1ExckBcTuA8nQhdMmIxvJY3euuNyvbSa/Fvy4ja9NUfMZN+Z1Y5dJ3msuX+wbW4BLk8ONF+Q9eCYuDICQsvXKmFuw/FpiNAHs1REPW4hyKyVfcLT8fSib32KefkY4k=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 43273527-9df3-42ff-d0cc-08dc32609443
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 22:09:16.7187
 (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: 21UgBDINuoG1+EUDxbsEknJr7zAliAnt45CD0LtGR8apEpgkNzotRX1bPqWv7zEvWelcViG522oRUFqio7DwrO91yXCtje8GqnRiwqxwTjI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7496
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-02-20_06,2024-02-20_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402200159
X-Proofpoint-ORIG-GUID: a2zOBGkiGKCnMIRO3PDa0O22GQr78vKu
X-Proofpoint-GUID: a2zOBGkiGKCnMIRO3PDa0O22GQr78vKu


Mark Rutland <mark.rutland@arm.com> writes:

> On Wed, Feb 14, 2024 at 08:08:30PM -0800, Ankur Arora wrote:
>>
>> Mark Rutland <mark.rutland@arm.com> writes:
>>
>> > On Mon, Feb 12, 2024 at 09:55:27PM -0800, Ankur Arora wrote:
>> >> tif_need_resched() now takes a resched_t parameter to decide the
>> >> immediacy of the need-resched.
>> >
>> > I see at the end of the series, most callers pass a constant:
>> >
>> > [mark@lakrids:~/src/linux]% git grep -w tif_need_resched
>> > arch/s390/include/asm/preempt.h:        return !--S390_lowcore.preempt_count && tif_need_resched(NR_now);
>> > arch/s390/include/asm/preempt.h:                        tif_need_resched(NR_now));
>> > include/asm-generic/preempt.h:  return !--*preempt_count_ptr() && tif_need_resched(NR_now);
>> > include/asm-generic/preempt.h:                  tif_need_resched(NR_now));
>> > include/linux/preempt.h:        if (tif_need_resched(NR_now)) \
>> > include/linux/sched.h:  return unlikely(tif_need_resched(NR_now));
>> > include/linux/sched.h:          unlikely(tif_need_resched(NR_lazy));
>> > include/linux/thread_info.h:static __always_inline bool tif_need_resched(resched_t rs)
>> > include/linux/thread_info.h:     * With !PREEMPT_AUTO tif_need_resched(NR_lazy) is defined
>> > kernel/entry/common.c:          if (tif_need_resched(NR_now))
>> > kernel/sched/debug.c:   nr = tif_need_resched(NR_now) ? "need_resched" : "need_resched_lazy";
>> > kernel/trace/trace.c:   if (tif_need_resched(NR_now))
>> > kernel/trace/trace.c:   if (tif_need_resched(NR_lazy))
>> >
>> > I think it'd be clearer if we had tif_need_resched_now() and
>> > tif_need_resched_lazy() wrappers rather than taking a parameter. I think that
>> > if we did similar elsewhere (e.g. {set,test}_tsk_need_resched_{now,lazy}()),
>> > it'd be a bit cleaner overall, since we can special-case the lazy behaviour
>> > more easily/clearly.
>>
>> So, we have three need-resched interfaces:
>>
>> 1. need_resched(), need_resched_lazy()
>>  These are used all over non-core (and idle) code, and I don't
>>  see a case where the user would find it useful to dynamically
>>  choose one or the other.
>>  So, here two separate interfaces, need_resched()/need_resched_lazy()
>>  make sense.
>>
>> 2. tif_need_resched()
>>  This is mostly used from preempt.h or scheduler adjacent code to drive
>>  preemption and at least in current uses, the resched_t param is a
>>  compile time constant.
>>
>>  I think the scheduler might find it useful in some cases to parametrize
>>  it (ex. maybe the scheduler knows how long which bit has been set for
>>  over long and wants to pass that on to resched_latency_warn().)
>>
>>  But that's a contrived example. I think this one would be fine
>>  either way. Will try it out and see which (tif_need_resched(rs),
>>  or tif_need_resched_now()/tif_need_resched_lazy()) seems cleaner.
>>
>> 3. *_tsk_need_resched()
>>  This is is used almost entirely from the scheduler and RCU.
>>
>>  One place where I found the ability to parametrize quite useful
>>  was __resched_curr(). So this I would like to keep.
>>
>> All of that said, and I wonder if we need these new interfaces at all.
>> Most of the code only uses the NR_now interface. Only the scheduler and
>> the entry code need to distinguish between lazy and eager.
>> (Plus, this way lazy and eager becomes an implementation detail which
>> doesn't need to be known outside the scheduler. Which is also kind of
>> the point of PREEMPT_AUTO.)
>>
>> Say something like the patch below (and similar for tif_need_resched(),
>> need_resched() etc.)
>>
>> What do you think?
>>
>> Ankur
>>
>> ---------
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 58e6ea7572a0..b836b238b117 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1953,7 +1953,7 @@ static inline bool test_tsk_thread_flag(struct task_struct *tsk, int flag)
>>   * tif_resched(NR_now). Add a check in the helpers below to ensure
>>   * we don't touch the tif_reshed(NR_now) bit unnecessarily.
>>   */
>> -static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> +static inline void __set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>>  {
>>  	if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == NR_now)
>>  		set_tsk_thread_flag(tsk, tif_resched(rs));
>> @@ -1964,6 +1964,11 @@ static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>>  		BUG();
>>  }
>>
>> +static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> +{
>> +	__set_tsk_need_resched(tsk, NR_now);
>> +}
>
> I assume for this (and test_tsk_need_resched() below), you mean to drop the
> resched_t argument, i.e. this should be:
>
> 	static inline void set_tsk_need_resched(struct task_struct *tsk)
> 	{
> 		__set_tsk_need_resched(tsk, NR_now);
> 	}
>
> Assuming so, this looks good to me!

Yup. Great.

--
ankur