Received-SPF: pass (google.com: domain of linux-kernel+bounces-158059-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
References: <20240312193911.1796717-1-seanjc@google.com>
 <20240312193911.1796717-3-seanjc@google.com>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Jonathan Corbet <corbet@lwn.net>, Ingo Molnar <mingo@redhat.com>,
        Peter
 Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Will Deacon
 <will@kernel.org>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Valentin Schneider
 <valentin.schneider@arm.com>,
        Marco Elver <elver@google.com>,
        Frederic
 Weisbecker <frederic@kernel.org>,
        David Matlack <dmatlack@google.com>,
        Friedrich Weber <f.weber@proxmox.com>,
        Ankur Arora
 <ankur.a.arora@oracle.com>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 2/2] sched/core: Drop spinlocks on contention iff
 kernel is preemptible
In-reply-to: <20240312193911.1796717-3-seanjc@google.com>
Date: Wed, 24 Apr 2024 23:18:02 -0700
Message-ID: <87cyqedkyt.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?2EW5ll2Wtbc22IJd8jw5o+IeM1U5RLhTtZbelxMyf8M/ouB3yk9vRqfA4Nj0?=
 =?us-ascii?Q?zZZg5msJXJrUmR1JeStlneiv5iFomgjK/zW0HB7UkS8Mw8DBi53Swmf2k2jO?=
 =?us-ascii?Q?eZpOFPA1di5XVBi6ucCDIE3y+PgV7HeIm2KLxxvubsUiKgcehtSIWGZ3hDwl?=
 =?us-ascii?Q?OidPkVE05DQnuevIQfnAVsQrxcojgEp9zXV2sdvwnupfhuzUXHHijGkHdqNr?=
 =?us-ascii?Q?Hwl86JX76pW59ST6ZaIcX4MNlOjtQGsrOnkuxS6eLjAWOmtr4JLm6R4BOHWG?=
 =?us-ascii?Q?syPdoMXeo01+VMhyxlYujhmp0JT3yT2ELLa3g25buZPMghhYeH2zZHuE53Wa?=
 =?us-ascii?Q?0hgLA2UVdGmoop0oV/D+rzMqY9gippZJ5yEpmPDhu2myKXYaxSIWaiM4/R4z?=
 =?us-ascii?Q?dot6Wpm3+X31ZCxKzZ8qSYQm4GGWjgFN/Cr61n55s7QQmfqkpjcKOJw+KB88?=
 =?us-ascii?Q?iWmu/93uGQZOj9sObxYevx6zhNiO/tFn3vaFF5+19hMwD6/1I8SHmCNYYnxF?=
 =?us-ascii?Q?6G8tBKVW1gZxjFccopAbYVx2O5rQIx2hFIVPhwfYtjn7dk6mZuilehEkBYF0?=
 =?us-ascii?Q?35dG6ju8GmCT535CIDpau6lmI5XTY3siU8u+S7K79/LJCa574lsyimvGIKAu?=
 =?us-ascii?Q?PQ14iHG8DoBzoBqcS9KV23fb8xPBSkqYyYllcxbONmWPXkzesPXLiWEhl8wF?=
 =?us-ascii?Q?ZjWFRX7hf+N3y+XfefgEWd5XojiSCBxnKqODu1LQViLGvODx2u/PWN0NFbpL?=
 =?us-ascii?Q?sz9xqD6GqIFYsX3kVzhNfekPjpnX0GRzeJQtAwU+0R5Bd+caEOc0vC7xReY+?=
 =?us-ascii?Q?WNImmO/JP0Ft3Cn6UqAW9d0CIbbbGZYNe8Gh/LxXCoDGxsvXlJFDxe7QAQVp?=
 =?us-ascii?Q?aNMYpU66yXXMD1eQJz8rRriSg/Q7QgOA4YtQhiZFlpp9W3pCch94EA6CipAX?=
 =?us-ascii?Q?jbN7iV7y9J4Vh2m7lTj+vc6DCSaGhZmbbgKFMEkbr2FMVIE9hVBQSi8Bog07?=
 =?us-ascii?Q?mMpGtabYW/THXRYJtb8WfnmkiMFHN/+4+gez3k2aoo++0s5t5y9k0pU7F22W?=
 =?us-ascii?Q?+zSUSgoIcj25pJ97ocBTwhIHtwGaLP/VpYt+nIeOtWxh/9n5U0HOv3b7D3KR?=
 =?us-ascii?Q?xwb1KNyJ4JxbYtHcqLjM0n5rUFLq9RnToVfXfAkG//ryBTTieSQMCxazzuUa?=
 =?us-ascii?Q?JKUUw+5JSjyzwOtYXaWs29/iMfdeInhiVvys2aJlH72N57zgeJHKoCSHj5nH?=
 =?us-ascii?Q?T+812GCrrZrN94Kfd+W0A1/yfG4gkIoSFlc+wM5DkihIZVkO2OeFPOw4ORYW?=
 =?us-ascii?Q?1sX5OQ/qklnpfh8/G7IhLPwhM6m/8J2Va+rFRduKLaKajiG1X01hQACZP0gI?=
 =?us-ascii?Q?8f3fCJ3IZ3M78ELph0A84Kl/y8D5wQhGlJ+YoqB/a3R5Z2bFbkbwX8pEJ4iH?=
 =?us-ascii?Q?/kUA+5JObzO676rS/4uRML2Qi9GrQV1etXK9yfH5vPPQ/Sp0lc6jFe41OF/K?=
 =?us-ascii?Q?PGAZny6YfJIPyaYVRFMrfiAZzHo62iEtBUwBFuKLnIwjVHbw/RW9dMJHLCXM?=
 =?us-ascii?Q?JuohJS7dkQV5aP+S5fLMB0xjjLrTsH7zCCTwQ9VDhTBIhe5FrzjKokgMQiu8?=
 =?us-ascii?Q?uA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	1RDGrtSvduh0W7FoPcXM68uKKXK+rVBgTJl6YNHFV+EJG8QwMcoh5Fvye92HjMkiVZz27C8yyfzuPVcqKcT31shA8L3hrm9LIjj5mQujmwq976g5AmHRuuZJ3CpS+t4Tai8CcY7W4ngOyyxNS2WC9F0iUnOPat3ImWb+rzX1g/pvgUJF6QbcBgeSkrmfYnDU4xF7v7mrBO5fC6Qk40Qoi4gZ4g+T+dERP+DLqBo89hzdMgzTyuW/B44Vo9a//4EdEm5FwgvVK+VPwqn7dMfS9qzPhF/ZHoCCm8MaEhtTZR6Wfxh5mg0T12qbYodEmscNzA4Nl+Gw9DwqYA3XjfHphlXEFjIxPL1j7sMdksOiUYJuiFGE1/A7aFnFTftwbK31aUyQOLN1KMtlAiHzzuu01az5HebMDcpZfMODYfX5vyLK1cNSkKATTYVmejMRt91AVH/6I/LbX5SA9TeIyCh16a2hYr7JyaAY6DA19CPzxa1CUs/D6Py2dJ3fgT0y9iV8sbz1sT1uI6R7kH5XfT9WxBAoQZmYzGPLmJw0Ks5G8LqKlKMLGCsvzQN2XnyKxAJy1T/+TmJQBmpAcdNyeyFF7Z2nICFVhIxG2ny/ukceVCI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bd5b295-8086-45a3-597f-08dc64ef7765
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2024 06:18:04.4815
 (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: mdOSUvNP5OFssj19zcOcdeGOVwg8SbemUjc+7caGE5SlqbyibL5yoyVS5CfcZcY/dOEN0l7FsttCRaL9wj1Gd4swuzSr8uR+TStzd1OInjg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5683
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-25_04,2024-04-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999
 mlxscore=0 phishscore=0 spamscore=0 adultscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404250043
X-Proofpoint-ORIG-GUID: FuHvl78EzC42BP8KN8PsSShBAN5E6pmV
X-Proofpoint-GUID: FuHvl78EzC42BP8KN8PsSShBAN5E6pmV


Sean Christopherson <seanjc@google.com> writes:

> Use preempt_model_preemptible() to detect a preemptible kernel when
> deciding whether or not to reschedule in order to drop a contended
> spinlock or rwlock.  Because PREEMPT_DYNAMIC selects PREEMPTION, kernels
> built with PREEMPT_DYNAMIC=y will yield contended locks even if the live
> preemption model is "none" or "voluntary".  In short, make kernels with
> dynamically selected models behave the same as kernels with statically
> selected models.

Agreed. This behaviour makes sense. Should also be useful for PREEMPT_AUTO.

The only thing that gives me pause is that now there is an extra
call+ret even when we don't yield the lock.

But maybe that could be addressed separately by converting
preempt_model_* to use a static key or similar.

> Somewhat counter-intuitively, NOT yielding a lock can provide better
> latency for the relevant tasks/processes.  E.g. KVM x86's mmu_lock, a
> rwlock, is often contended between an invalidation event (takes mmu_lock
> for write) and a vCPU servicing a guest page fault (takes mmu_lock for
> read).  For _some_ setups, letting the invalidation task complete even
> if there is mmu_lock contention provides lower latency for *all* tasks,
> i.e. the invalidation completes sooner *and* the vCPU services the guest
> page fault sooner.
>
> But even KVM's mmu_lock behavior isn't uniform, e.g. the "best" behavior
> can vary depending on the host VMM, the guest workload, the number of
> vCPUs, the number of pCPUs in the host, why there is lock contention, etc.
>
> In other words, simply deleting the CONFIG_PREEMPTION guard (or doing the
> opposite and removing contention yielding entirely) needs to come with a
> big pile of data proving that changing the status quo is a net positive.
>
> Opportunistically document this side effect of preempt=full, as yielding
> contended spinlocks can have significant, user-visible impact.
>
> Fixes: c597bfddc9e9 ("sched: Provide Kconfig support for default dynamic preempt mode")
> Link: https://lore.kernel.org/kvm/ef81ff36-64bb-4cfe-ae9b-e3acf47bff24@proxmox.com
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Marco Elver <elver@google.com>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: David Matlack <dmatlack@google.com>
> Cc: Friedrich Weber <f.weber@proxmox.com>
> Cc: Ankur Arora <ankur.a.arora@oracle.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Ankur Arora <ankur.a.arora@oracle.com>

> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 +++-
>  include/linux/spinlock.h                        | 14 ++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 825398d66c69..fdeddb066439 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4689,7 +4689,9 @@
>  			none - Limited to cond_resched() calls
>  			voluntary - Limited to cond_resched() and might_sleep() calls
>  			full - Any section that isn't explicitly preempt disabled
> -			       can be preempted anytime.
> +			       can be preempted anytime.  Tasks will also yield
> +			       contended spinlocks (if the critical section isn't
> +			       explicitly preempt disabled beyond the lock itself).

This seems to read a bit better:

+			       can be preempted anytime.  Tasks will also yield
+			       contended spinlocks (unless the critical section is
+			       explicitly preempt disabled beyond the lock itself).


Ankur

>  	print-fatal-signals=
>  			[KNL] debug: print fatal signals
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 3fcd20de6ca8..63dd8cf3c3c2 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -462,11 +462,10 @@ static __always_inline int spin_is_contended(spinlock_t *lock)
>   */
>  static inline int spin_needbreak(spinlock_t *lock)
>  {
> -#ifdef CONFIG_PREEMPTION
> +	if (!preempt_model_preemptible())
> +		return 0;
> +
>  	return spin_is_contended(lock);
> -#else
> -	return 0;
> -#endif
>  }
>
>  /*
> @@ -479,11 +478,10 @@ static inline int spin_needbreak(spinlock_t *lock)
>   */
>  static inline int rwlock_needbreak(rwlock_t *lock)
>  {
> -#ifdef CONFIG_PREEMPTION
> +	if (!preempt_model_preemptible())
> +		return 0;
> +
>  	return rwlock_is_contended(lock);
> -#else
> -	return 0;
> -#endif
>  }
>
>  /*