Received-SPF: pass (google.com: domain of linux-kernel+bounces-201781-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
References: <20240430183730.561960-1-ankur.a.arora@oracle.com>
 <20240430183730.561960-5-ankur.a.arora@oracle.com>
 <3f72519e-bf82-458f-a066-0c296a7d655f@oracle.com>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: Ankur Arora <ankur.a.arora@oracle.com>, catalin.marinas@arm.com,
        will@kernel.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        x86@kernel.org, hpa@zytor.com, pbonzini@redhat.com,
        wanpengli@tencent.com, vkuznets@redhat.com, rafael@kernel.org,
        daniel.lezcano@linaro.org, peterz@infradead.org, arnd@arndb.de,
        lenb@kernel.org, mark.rutland@arm.com, harisokn@amazon.com,
        boris.ostrovsky@oracle.com, konrad.wilk@oracle.com,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 4/9] cpuidle-haltpoll: define arch_haltpoll_supported()
In-reply-to: <3f72519e-bf82-458f-a066-0c296a7d655f@oracle.com>
Date: Tue, 04 Jun 2024 22:47:17 -0700
Message-ID: <87mso0ym8q.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?Jv+buNIuokNT44aEKp26TsUT642uCK2GELzwSjX3LbXwChMdFSLMlx1HibiT?=
 =?us-ascii?Q?0S+4xYX3kc70fPbk4AR9IPuI3a1cWWAtj+iy1hYVTutEL2QCUh7MZTDvef0z?=
 =?us-ascii?Q?52DUJgmf5fRDGifORa+CrPaceRILhw4Ze7Z0j4MFn1VSX0ST6EJw7yVol5eO?=
 =?us-ascii?Q?47I8UYmG8uVTHqUnUJtGioj+QNxF77w4DWbi1nBcqBSr9k9gaMWGL2Ndq4Pn?=
 =?us-ascii?Q?Xvbt6eoiPBwTCu3kQKDH8q/Jaele4axSRYMQHCzG355dE2en21ETjdOTu3p2?=
 =?us-ascii?Q?NvVFVdFkVK4JCIB5WGrpnlgQASWPcYkI1NzBIHS3pyf8kpZlcrhjDKQCQLXO?=
 =?us-ascii?Q?0hfhfTxQdoQ/XqG1GhkXDYcRSlNUrs+l2fVTm3m6+d2VKK5WH3k/CbSy+D96?=
 =?us-ascii?Q?qfOXGm43MYDpVgptY2DJ0eZU6MwzkaDNq05iKhAD7Eqw6CGgYkrnwYr1ez44?=
 =?us-ascii?Q?H7Iy9jntZsA08RpFOKjnnz6HePiT8yApwodgJNflSjJxQX4/t/H17CTK3HUz?=
 =?us-ascii?Q?ZiB0CkYpDOCRO9fv20SrpwVK/5uBOPZ7r1zP9ap4SueRk0wrTux308a52y7c?=
 =?us-ascii?Q?R2JsVUNTzrAAEnCO+aWN3nbxHR/KSmxK/wg7uarGKJEPYwxLBI54sSi4YU/V?=
 =?us-ascii?Q?c45MzCszdbE0ubNB+SRWU3Ir5REDMusNLccxYO9z3v+LGOaTvbJGitakQtj3?=
 =?us-ascii?Q?2KYyF8hvWzvVH/prZ5A4oMNIrEpXh/oQoSnKW6UbvTHwLw5lIbEBCYtUg053?=
 =?us-ascii?Q?Oc7TkIN5pPX5b0FgPABMXWNWfTjg4/SVnblHnbFpH5Zfp1XJDb+2kgtuYhw+?=
 =?us-ascii?Q?xvdy218CcEV2v+9BLdUMAmU8odQ4FJ5nAyRrukUc2+om2s7XeDWA5mUnbUgG?=
 =?us-ascii?Q?ExAfmwhDc8Mmy8Z34K2P1Mvmj/+8Mm+8zQwFKycN0tdlwDxIh+WtjHvVt/XH?=
 =?us-ascii?Q?dx+GJQonRwCATA7xTKJhkPsAz9DsHmNs8A1iLWa+gEeWZfYwhrMFIQITX+Re?=
 =?us-ascii?Q?GMir5OGeEACUJg5zvmCsxi7INgJF1U/TVaaJU+whDr2O0OKUvbp2HfZ1OMi2?=
 =?us-ascii?Q?rIi/46fmXQRcOYsJEJcpCDpb/peVDXptKmLdNlfH597LMCKHdiBuzAg7hDFt?=
 =?us-ascii?Q?/jZqcTbcmNW3OLsb7GCBe66jev/MYurWOFZflGf8ltNagn8WFocr9uMA3joF?=
 =?us-ascii?Q?aojJ8Fq5V/6go6hSZK+0m6wFk52NRPxBhDq/S0xzW7jrmEUKJU+GjwKbk9/J?=
 =?us-ascii?Q?TlzY/RuMbKelWmxmxKgzX0Gdn1cqPyanFfaztobdW5GZhLnElWqHpTaOvHvt?=
 =?us-ascii?Q?ipEJeGwnFS6ucnFUERL10QCRoF02k6KLY7dzPrVeLZ9FTI546pQx4XhhqJ3x?=
 =?us-ascii?Q?UsLoixKGv8bUCwJzNyb2DjMc4fBKbVtk/btnrkK71AGI0d+g2tRuM9ENUc0U?=
 =?us-ascii?Q?pUO+iQctB+Ibtr9o3RvVy4vUJZMYBRjZE85LO99fpbxXzW7ck8x0ir5qkt0r?=
 =?us-ascii?Q?Ed/ZFUu2xStt6EUCPzhzWXqNDnvZcYb6W/1U7QxFwUauoZg+wrHqcBsL5aVe?=
 =?us-ascii?Q?BM7mlH6Kmb3hoszWqUzTwI6ORNQnP1L6ObRDkLqEGzT0KowA2mI9HWPCCBMN?=
 =?us-ascii?Q?CQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	KQTzzMr3HCHmyhiZ3mdAYnKOFrvkYJ2krCVpheNN97LYAQdM+t5PGKhwLvKwFitpvLzPY1OiXJ2X9Ca9gbuhd89aRc8KCUtDVH4hdPpN2MoILaCRfcsHM9cUSesPmxuLQmi5B63Q933HnH1wgB6fPA/glOoVSioIdByusg+J2gKzz1W2PY647za+BTmS+JASYaIw59tvLl+XxGqrWC+aLTGg4HBsEj3O0LOLQViK7LkdfJ5mVL3Y4QRKT9ymkHkgpA5WauHF3LLPZCOKfzhiginmamWnFIjyA7BKP2VE5LH1cIN/t5dGGGAcenZHbwNoQGUYCtNtImaV1sgRMoXjS8ullb5yQ0qWsYIJGGrrUW5Xy/YySYEh6Zh1TE12dvFdSI/p7X+9hm1666pCRnhVmna6PZ+DzCN1qQ/0yhTGh9WPYW8/QZYNCcDKekEiLPIk7x2DSRsu6+uMVt1vzqVZDiIL2sFRKrgMQcn86b+Ixe9zzkwhTOo/Ybbv2LxXGb7SThjRW6XM3ytu44z1QHn1jMm2BPETAYA32NpR7XgvY2Xo2WTUFWpQw081PJVrH5uoE5jmX14xDcjzhSZ+88E3vblvheQ9gaEMBXfYDpuV9BE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d20ef472-7c34-4927-2f80-08dc8522f831
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 05:47:22.1883
 (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: bd6L31MdF1YoGl44ODtL9++f0AUuDc4dtapJIme9zX+RfhtL1PEzHcn9BVhb7W+67jed5ezHRug5hIwsLXPCY23JLSwQYG6PQHrKXo8T0o4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4345
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-04_11,2024-06-05_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 adultscore=0
 phishscore=0 suspectscore=0 bulkscore=0 spamscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2406050042
X-Proofpoint-GUID: QeYDQyVsnHR272sEtG40Re-flUm7GPyj
X-Proofpoint-ORIG-GUID: QeYDQyVsnHR272sEtG40Re-flUm7GPyj


Joao Martins <joao.m.martins@oracle.com> writes:

> On 30/04/2024 19:37, Ankur Arora wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>>
>> Right now kvm_para_has_hint(KVM_HINTS_REALTIME) is x86 only. In
>> pursuit of making cpuidle-haltpoll architecture independent, define
>> arch_haltpoll_supported() which handles the architectural check for
>> enabling haltpoll.
>>
>> Move the (kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME))
>> check to the x86 specific arch_haltpoll_supported().
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>
>> ---
>> Changelog:
>>
>>   - s/arch_haltpoll_want/arch_haltpoll_supported/
>
>
> I am not sure it's correct to call supported() considering that it's supposed to
> always supported (via WFE or cpu_relax()) and it's not exactly what it is doing.
> The function you were changing is more about whether it's default enabled or
> not. So I think the old name in v4 is more appropriate i.e. arch_haltpoll_want()
>
> Alternatively you could have it called arch_haltpoll_default_enabled() though
> it's longer/verbose.

So, I thought about it some and the driver loading decision tree
should be:

1. bail out based on the value of boot_option_idle_override.
2. if arch_haltpoll_supported(), enable haltpoll
3. if cpuidle-haltpoll.force=1, enable haltpoll,

Note: in the posted versions, cpuidle-haltpoll.force is allowed to
override boot_option_idle_override, which is wrong. With that fixed
the x86 check should be:

bool arch_haltpoll_supported(void)
{
       return kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME);
}

and arm64:

static inline bool arch_haltpoll_supported(void)
{
       /*
        * Ensure the event stream is available to provide a terminating
        * condition to the WFE in the poll loop.
        */
       return arch_timer_evtstrm_available();
}

Now, both of these fit reasonably well with arch_haltpoll_supported().
My personal preference for that is because it seems to me that the
architecture code should just deal with mechanism and not policy.
However, as you imply arch_haltpoll_supported() is a more loaded name
and given that the KVM side of arm64 haltpoll is not done yet, it's
best to have a more neutral label like arch_haltpoll_want() or
arch_haltpoll_do_enable().

> Though if you want a true supported() arch helper *I think* you need to make a
> bigger change into introducing arch_haltpoll_supported() separate from
> arch_haltpoll_want() where the former would ignore the .force=y modparam and
> never be able to load if a given feature wasn't present e.g. prevent arm64
> haltpoll loading be conditioned to arch_timer_evtstrm_available() being present.
>
> Though I don't think that you want this AIUI

Yeah I don't. I think the cpuidle-haltpoll.force=1, should be allowed to
override arch_haltpoll_supported(), so long as smp_cond_load_relaxed()
is well defined (as it is here).

It shouldn't, however, override the user's choice of boot_option_idle_override.

--
ankur