Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230403052233.1880567-1-ankur.a.arora@oracle.com>
 <20230403052233.1880567-9-ankur.a.arora@oracle.com>
 <20230405202240.GE365912@hirez.programming.kicks-ass.net>
 <87o7o1lzko.fsf@oracle.com>
 <20230406201359.GC405948@hirez.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, linux-mm@kvack.org, x86@kernel.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,
        willy@infradead.org, mgorman@suse.de, rostedt@goodmis.org,
        tglx@linutronix.de, vincent.guittot@linaro.org, jon.grimm@amd.com,
        bharata@amd.com, boris.ostrovsky@oracle.com, konrad.wilk@oracle.com
Subject: Re: [PATCH 8/9] irqentry: define irqentry_exit_allow_resched()
In-reply-to: <20230406201359.GC405948@hirez.programming.kicks-ass.net>
Date:   Thu, 06 Apr 2023 19:29:59 -0700
Message-ID: <87lej4ifvc.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gi9bh+3SSXuProYE98KVVmUsg8TnAlMZosPBhJgprlInJvzwE384zXjjiai0?=
 =?us-ascii?Q?AImpE5rb8938rU0UDMEByufxHf+GSgQryW9GDtoEcpL2QTtNIYckOkaf6ZTL?=
 =?us-ascii?Q?6BCoLj6k3mSXfI0KxBOqJd8t9glZXKJKAQtcpXGgnDi8G7DtA5B8gdSIrp98?=
 =?us-ascii?Q?G7qLICs8WFG4oY701V5VCOUyOy3tZPvJ04oXVMeCP4+ycU/2Mtl73Z+SF4Jg?=
 =?us-ascii?Q?GLXKOhhmI7Sb4ivjPz98s1198KmAa038cfsFZMUYePzGE1HoX7/ov08823l1?=
 =?us-ascii?Q?T1Yxe5Cmb+Mt1MJf/vUu4PtfqvUygpx4IGIkvUw9Xw2/bQB6KciEi0lnc/Wo?=
 =?us-ascii?Q?qdIYBa+CjHyOIRuJTdPTou6vtueceactIhBt5kZ21+XgT3DihCx5QPW5lXT1?=
 =?us-ascii?Q?qhY9C6AjmwSYs9LPPId1cVklNmETdy1yLEbXQGS7j9B6dMbCzRFCLA7Ix7jR?=
 =?us-ascii?Q?JVt5X+n0sYTDwWvo8bsJUI+nRzfDug3fyUJ0GiWnJg5+Y2xBWofRQRx6yzpZ?=
 =?us-ascii?Q?aFR/6kRl2d7aW9L82UJeMAR1HqRRYjD+Znzb7mMCb0vcsfsfZqWxYuO128Sy?=
 =?us-ascii?Q?hedQZc21GzuzJ4KjUR+TxKavngKomd65AoOn2N6KwopqFxK7eu4PdAhpA0k+?=
 =?us-ascii?Q?CC7WgOIBNfrGreQiZiThX8FEAK6tT6uNRUFtoOqNf6x5rySXvFGMqVybUPEi?=
 =?us-ascii?Q?0snj1A235nYjsaJE5H7STEVkoSCGpRQULfMA8V9n+ENbAcBHiSyOZpNxiO2v?=
 =?us-ascii?Q?BxxqCGHAtfytLoknzP9ddQxMbnq4eiPqXKre5okFTYWZ49nRQ6WC75CcEonQ?=
 =?us-ascii?Q?x4gfBz5t5x5eS8LXe4CK0FQFirNCXFJcayyK00ucEI0eMlYJncmU1E/DJAbb?=
 =?us-ascii?Q?PyJKR70QlTMyLM89eSqv/RW6BA6raPmWB9vZCqESBD2TFCPSXDYRBX9mnhZm?=
 =?us-ascii?Q?bBPqHi2Uqg0HUWEMM5thZ9wQONRV36AyzDFQ04QfhiWDxiZ+TTtF4abzceng?=
 =?us-ascii?Q?U9gMedl06xp/BFzdogMz5J5oAEC2vCf9wQ17cIrWbdpoXijojX+841Ih9zYN?=
 =?us-ascii?Q?28NeG4W1lBv32KIYxvZ7l1y1g4cxnNuIPV2N4rZZTYZBRV3P0kpmNxahjA+H?=
 =?us-ascii?Q?cbDqdbB6hH245F3Ma0X4oiItI3RDfbcnQW7qEc866c6ROLr2jA3ydyWba2he?=
 =?us-ascii?Q?iU3IEZk2UYJRuuYq/Mi2I9wv9brSw4QOI0X9uZmdqCR7y7zWX7l3QoYzd4ga?=
 =?us-ascii?Q?M5J/QPx+XJ9Yg/Rp3nYpr5yn/Tk2ADBmSR2uPHoOa9MK9m5Qv3acUvrJQgo7?=
 =?us-ascii?Q?UWUeSRL2g0QVCJLHrOEXSwoCgSg16JtHehzLMHfUJeDrYx/Ny/BK3n2z5yta?=
 =?us-ascii?Q?wBsKFGdim2gBFVK5co55/XkOEXk6oeixkyZL3W0pVCq6cR9xjdx3C3vRpYUZ?=
 =?us-ascii?Q?eusNVQkC5fCE9X4N29z89zRvKNlqCXmbsvXObM0+Pq4HetH67hUnbhYbzfKK?=
 =?us-ascii?Q?BibbWiTX3z3q3AwkLYDrmSvna0ec73/D/Yq3f2GcSI2Cx0atPHXTjJLR6Ui4?=
 =?us-ascii?Q?tkGdYWfe5ZwuOElXwX6zpqj52vnTINSa5Bn00CuvhBKLTSmkO6ejlPAxRKyG?=
 =?us-ascii?Q?zQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?bgxJuPlrUUimXS0HIVI0NSnsKK46vvmhIqJWqeeHVT1HtU7kceZlnlFSmG/L?=
 =?us-ascii?Q?YzjE3DuIVw4NKt5h6+6ehh4Y0UEleoP+bVlSaCj7rULVgoEJc2P3sH9ZwR1u?=
 =?us-ascii?Q?xmExPnJkdpsu+e3eP2HRZDqXz8rITxbGCdHNrzWCrJEc+iyf/NLIELCFNILG?=
 =?us-ascii?Q?yc7ROff724EPGw7yIbxGDSc+vaAOCeVe/4aklDbeScYAj1ZaXKhw0OQ4kriq?=
 =?us-ascii?Q?jYzQIrvaOPkhDfLap+tUBYQEFEA11hycw/EYyy8jF3IS5SyjV46Pq819f3S7?=
 =?us-ascii?Q?hJLJ9WpUG7Zbt8+tsjk7AnCIC46a+3ieMvSXv8z38o6A7VDpbtwiqpbNAnHB?=
 =?us-ascii?Q?17IGR4wj4xP7iDQqisFnRAGWTtiV+jhMK1qOt5gJkSltBgSmVAMrQZj5h1OR?=
 =?us-ascii?Q?75S9eLIWIOZH9sMpDwfoSyNOH5zKtP1NiSDj5XEksEqtKJ7bZwzSr2XG9OHZ?=
 =?us-ascii?Q?dDD+g3mX0Tt33+ym0VdMLXG71BCXTnEq+G7aOhW20ToTL0wYQ3ZAOU20tLOr?=
 =?us-ascii?Q?M4JRy/4TaVPUaTfz60naS/cMZsrAK+gH7db8mdWhc2ZuW5bKbyZcJog+cQJT?=
 =?us-ascii?Q?GZRLT9Y4TAl3l8Bi+YRJlw2Ub9r6ZMTyGcBjim/K2ZNZRSz6V673Wd16JFN6?=
 =?us-ascii?Q?4O4n3DcyThOyipZ8qc3vyHpWPXEiOQSeKKJxnJupgcqZr1FabgrKyvJwEeeC?=
 =?us-ascii?Q?ffUlMcVy3H0gH5TVSKWyohRUptT4CKBlXZsIn3zwAgRD123rCl3vJtQ/tTf4?=
 =?us-ascii?Q?VN7nRLxCxG4j+uE01FURbRG5oEuYL+mJxRIWI5ohjaYBke4yUQBbw40mxaf9?=
 =?us-ascii?Q?5W3XfI6S15i3b/cFCosTKIxLxHf3s6M2noDiNjcBttUQBoXY7/fPWfVFPrF6?=
 =?us-ascii?Q?QWlm/fgi943j3hxwhJO6dv9x6rK0l/5d2c9qKgYkgj/kzIUrB2v2ttELRQ4p?=
 =?us-ascii?Q?UPsc1KvygH+e3AMsYT/2nx/ElBe9y1W/j+WtqTP64TUuxpTuIyYua143Ne8a?=
 =?us-ascii?Q?m0E0K3he+ou32pVnnv7PXHGG0tNyPCs8KCJm+01nrZCGvK0t5BpSVuqYUKFK?=
 =?us-ascii?Q?ddEpZY1t2SmOV82yLYeXNQDWqBoXCGjI7J1QylJEeRz6/fdp2HX5UxMa9gAI?=
 =?us-ascii?Q?3ELJ+RKhr/ie02ijT9w/oCIMgzQzN2efpLDV8UnPhdb/+quXKHeovyM=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d2a56ae-6296-475b-5cff-08db370ff5b6
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Apr 2023 02:29:49.2802
 (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: kiVqYh0ZmeU6AQZ0g+fuEgxGO1OBhS/FKfOdDRLDoTrCM7jBuL83aG7gZfEZhh1iiqEbpLoSvlkcwpCPo8Y91iWxTTpFmcHLiMYGHrKs8Rk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB4118
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-06_14,2023-04-06_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 phishscore=0 bulkscore=0 mlxscore=0 malwarescore=0 mlxlogscore=773
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2304070022
X-Proofpoint-GUID: ouK_bn8Msw1t0LLyfBUUSYj24kRoKMsD
X-Proofpoint-ORIG-GUID: ouK_bn8Msw1t0LLyfBUUSYj24kRoKMsD
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Apr 06, 2023 at 09:56:07AM -0700, Ankur Arora wrote:
>
>>
>> Right, so:
>>
>> CONFIG_PREEMPTION: raw_foo()
>> CONFIG_PREEMPTION && (preempt_dynamic_mode == preempt_dynamic_full): raw_foo()
>>
>> This is NOP if CONFIG_PREEMPTION && preempt_dynamic_mode != preempt_dynamic_full.
>>
>> > 	if (!A)
>> > 		raw_foo();
>>
>> So I would call raw_foo() for the CONFIG_PREEMPTION=n case.
>>
>> > What you really care about is the CONFIG_PREEMPTION=n case, but that you
>> > don't actually handle.
>>
>> I don't see that. The CONFIG_PREEMPTION=n (or its dynamic version)
>> is being handled here.
>
> Bah, I overlooked we have multiple definitions of the
> preempt_model_foo() things. For some reason I thought all that was only
> for DYNAMIC_PREEMPT.
>
>> > And yeah, you've got the extra resched_allowed() thing in there, but
>> > that doesn't make it any better -- this is all quite horrible.
>>
>> I don't disagree. There's a quite a lot of static/dynamic config options
>> here and adding this clause didn't improve things.
>>
>> I think just going with a static call here for the allow-resched case
>> might have been cleaner. Alternately I'll see if it can be cleanly
>> folded in with the irqentry_exit_cond_resched() logic.
>
> Something like the below perhaps?
>
> ---
>  include/linux/entry-common.h |  6 ++++++
>  kernel/entry/common.c        | 23 +++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
> index d95ab85f96ba..0c365dc1f1c2 100644
> --- a/include/linux/entry-common.h
> +++ b/include/linux/entry-common.h
> @@ -415,10 +415,16 @@ irqentry_state_t noinstr irqentry_enter(struct pt_regs *regs);
>   * Conditional reschedule with additional sanity checks.
>   */
>  void raw_irqentry_exit_cond_resched(void);
> +void irqentry_exit_cond_resched_tif(void);
> +
>  #ifdef CONFIG_PREEMPT_DYNAMIC
>  #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
>  #define irqentry_exit_cond_resched_dynamic_enabled	raw_irqentry_exit_cond_resched
> +#ifdef TIF_RESCHED_ALLOW
> +#define irqentry_exit_cond_resched_dynamic_disabled	irqentry_exit_cond_resched_tif
> +#else
>  #define irqentry_exit_cond_resched_dynamic_disabled	NULL
> +#endif

So this is clever. Essentially this would toggle between the two kinds
for the preempt_model_preemptible()/!preempt_model_preemptible() dynamic
case. Do I have that right?

>  DECLARE_STATIC_CALL(irqentry_exit_cond_resched, raw_irqentry_exit_cond_resched);
>  #define irqentry_exit_cond_resched()	static_call(irqentry_exit_cond_resched)()
>  #elif defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index be61332c66b5..211d118aa672 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -390,6 +390,21 @@ void raw_irqentry_exit_cond_resched(void)
>  			preempt_schedule_irq();
>  	}
>  }
> +
> +void irqentry_exit_cond_resched_tif(void)
> +{
> +#ifdef TIF_RESCHED_ALLOW
> +	if (resched_allowed()) {
> +		/* Sanity check RCU and thread stack */
> +		rcu_irq_exit_check_preempt();
> +		if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
> +			WARN_ON_ONCE(!on_thread_stack());
> +		if (need_resched())
> +			preempt_schedule_irq();
> +	}
> +#endif
> +}
> +
>  #ifdef CONFIG_PREEMPT_DYNAMIC
>  #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
>  DEFINE_STATIC_CALL(irqentry_exit_cond_resched, raw_irqentry_exit_cond_resched);
> @@ -397,8 +412,10 @@ DEFINE_STATIC_CALL(irqentry_exit_cond_resched, raw_irqentry_exit_cond_resched);
>  DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
>  void dynamic_irqentry_exit_cond_resched(void)
>  {
> -	if (!static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched))
> -		return;
> +	if (!static_branch_unlikely(&sk_dynamic_irqentry_exit_cond_resched)) {
> +		irqentry_exit_cond_resched_tif();
> +		return
> +	}
>  	raw_irqentry_exit_cond_resched();
>  }
>  #endif
> @@ -431,6 +448,8 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
>  		instrumentation_begin();
>  		if (IS_ENABLED(CONFIG_PREEMPTION))
>  			irqentry_exit_cond_resched();
> +		else
> +			irqentry_exit_cond_resched_tif();

And, if we choose between the two resched modes at compile time then this
would work.

Might massage the names a little but this should work as is. Okay if I
use your Codeveloped-by/Suggested-by on this patch?

--
ankur