Received-SPF: pass (google.com: domain of linux-kernel+bounces-167224-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
References: <20240430183730.561960-1-ankur.a.arora@oracle.com>
 <20240430183730.561960-2-ankur.a.arora@oracle.com>
 <7473bd3d-f812-e039-24cf-501502206dc9@gentwo.org>
User-agent: mu4e 1.4.10; emacs 27.2
From: Ankur Arora <ankur.a.arora@oracle.com>
To: "Christoph Lameter (Ampere)" <cl@gentwo.org>
Cc: Ankur Arora <ankur.a.arora@oracle.com>, linux-pm@vger.kernel.org,
        kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, 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, joao.m.martins@oracle.com,
        boris.ostrovsky@oracle.com, konrad.wilk@oracle.com
Subject: Re: [PATCH 1/9] cpuidle: rename ARCH_HAS_CPU_RELAX to
 ARCH_HAS_OPTIMIZED_POLL
In-reply-to: <7473bd3d-f812-e039-24cf-501502206dc9@gentwo.org>
Date: Thu, 02 May 2024 21:13:40 -0700
Message-ID: <877cgba5xn.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?3IwFbjXwt2iAG+QiK/os5HhtxPhPi9ybGzu4ATyxeiN05zkZnGVDgrrAM+iw?=
 =?us-ascii?Q?chtrB83Ctb3O7dyINEPJidg3GslgJeFFlPlpBpxYDxizqt6gwFSCpVY2NBgr?=
 =?us-ascii?Q?9Xx9svl/4fPdi5k4rMM8mvgLMJFcwDGLImE3f/graZEVGRgVPbJBx/OwwQGZ?=
 =?us-ascii?Q?Zxd7TtOYHNnf+xrfvyvKj5GpNbn+DN5EEu1Aa0T2GUMNGt0GkpCZe+FzMDgI?=
 =?us-ascii?Q?lMM07PQggmbqoKUbooi5UfyifuEt75rzOh30zBzzSrSTKWJQ7PtxhAaQJWQw?=
 =?us-ascii?Q?/cvYaVj68DP7QRcli3RMW+V5VOAC8/9rwDWvfVdj5755P2JfjPVbZ1u2iE9d?=
 =?us-ascii?Q?bocjrW6J2Yi6sytHxuFD+JXnZlrr8xx2vlFCe5mcbhil+guUZChnKVwBXHtV?=
 =?us-ascii?Q?rieIP7+AgaTivwvCDE16WLFaQ6EKUCwCS6I4RcAOYYebRUmRvOOsMEnhc11x?=
 =?us-ascii?Q?I+hkkswCYXqwKiu/QUOH3MB0irN9rr5AJni8vSabrFuqqCGvBkZKLaHKe8L9?=
 =?us-ascii?Q?csWp900k3kUvpb/tWk3yzVlAu9BlePtiyKCB+aKTS3K0J5OnSJiwvCbHe6DV?=
 =?us-ascii?Q?mexruJKsTsRyQLfXgybMKVFAckw2AmG5EeXlWTBpbDusW6Tx8SSyJlN76xkZ?=
 =?us-ascii?Q?jEFdwh+k5OmI+Cxubg/fcVWcYVjMwsoLlor29xuhHdMqWARN2QOcFe2OEZsh?=
 =?us-ascii?Q?aThzNUTsHaCjCqgyWbYWhaqwIscJoF/mT2May0v7UZv9UEDbnMIYAFcxE9D4?=
 =?us-ascii?Q?ZNt6OcfKa1+Hp6xSFqC1HWN2YDMFDWYu9/cjodQ50DJYZiGQlks5EobyTMVh?=
 =?us-ascii?Q?yav0SMxJNbarW92Ms5i2vs9g6XRavwhUT1TmwiElvQS9hAouec7E/dsYPGmM?=
 =?us-ascii?Q?Z/rj/R8XgTOTWB7nBqVfLcxtmLuZrcQDhnIRBC+2wd+QcAVyXQbgNUbTUa3T?=
 =?us-ascii?Q?+Ue/DsL7QGG4uwpo3tSNFZiFCpr4XakLLrqNqvvX0NNiX/SHBJ0qGWbRL28F?=
 =?us-ascii?Q?sutO5D2gbEb+adryY8z4U0lb87pE0ht7awEjTKGYjBckzEMK0MZUJQYNMmPr?=
 =?us-ascii?Q?sDf8Hdn8jCIcTbrupuyo/4KYGS7/xwzeYQPZEvVHzSiAtDg+qMEGCKGEfMzj?=
 =?us-ascii?Q?9nqUhwLJiUQOWOpwuTSb00CNvgFJqxh0TCBSbDuM4qzsVIPCMEw8gSbbp6bS?=
 =?us-ascii?Q?jBThG609ytmGYTgnJr/eX5Ug4zFUFzuNi0+T4w7+t/+PraaWCO8+gWlUfXcv?=
 =?us-ascii?Q?ozYQKqAUCO40TPg00COWf9KGlRjYCPnQWj9oZMMhbyt7MDPgcwksTzs2mYhu?=
 =?us-ascii?Q?nha+Ons19Gxy6fBqWbVzkP6OV3AEXmi0oJ9ec+zm3whjzfERfzq8sSPlF6ze?=
 =?us-ascii?Q?x1cqUnVKfY+PuNc3b9J6ZyzcMA+96+AiVXAQAiXhIPFA0OIzQ8zWk9ipZRn8?=
 =?us-ascii?Q?oAL6enFdrcwMaWw86aGMKhTfkr6hNFozdEiNMJCNwzSFRVg+zbYYjrzh5B/R?=
 =?us-ascii?Q?Af5yUnzd3Dl/cFws/sofU6ds9VlLJSN/J89cguR3K4Ez84d+bUHWrXzeS2su?=
 =?us-ascii?Q?LQuvpeYctgsP1rckadTJ2pt6haXwR1OtO73agcB/VQ6yPApk8j0NhKG1fuQS?=
 =?us-ascii?Q?VQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	0GUq305l/cOkq5EQQcv6BCbKOI7hJmLHKwLaq+Iqh+sMiudAGeUvMLD1jq4SrCctiyfdl65ej1JC836xAMPNRcMrGi8D29iFjmE+lGlsxgbR7Tu/WFvkL4wqXODERbqfHEcl1N0LC+HDtMWexZPj0GEKfwg3zpPjErTZmWudGW7CpLjyXW1osVGhx1RWu+D6jzMO8hTxMq87i19vBgZS/vnv4/a+E4et/jMcNj4Wi+cO6A0X+772IxZt/upB26g7vtHDCf30+BE+znqRvv6F1T615uw5RrJuLZ/5/3Hch1P9UvCjNvOibIew9Z0u1udJ0jHZlGft0n3S8qUqpj2ITA7Hs5ze89JOzDPY83fBKNxveJJfgGRm2LENcULI88IEGaDgRWiRWkB4Y6FEuKL9ZoaJasxj8N2AGaE5/YwHR/rvBdDmcp6rRdrfUvz33+Zd2aWlfIrWhQTfhPcxcR+3ZNWier1s8Ef07FH6WBL/Vg64aea03o1uQ2+jzbAjGTnK3grIW4AY/OqDf9IrrrBjsPf2I+fT+LBs+1XOIWwX+rsDF/uacr1pNJSTHXjcNuh4OICrN3FkBUXSBcbVzHIK2VoZ7TKbgtliAMuE0edm9mY=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 97c99ea5-caaf-42e2-232e-08dc6b276b78
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 04:13:43.2306
 (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: tl1/0lZtcboCdwSrbxkKDxO2XqolEhiPn49gYtiOGGzPTb5FGnjk8JcdpcsfksKSsMIBqXQ/zps6D3ANsmQzL4SNf35KJC1J7rDp0uEkkp4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYXPR10MB7949
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-05-03_02,2024-05-02_03,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=810 malwarescore=0
 adultscore=0 mlxscore=0 suspectscore=0 phishscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2405030027
X-Proofpoint-GUID: t---21aPXg4hwl6L23O02oGEARWdxbzZ
X-Proofpoint-ORIG-GUID: t---21aPXg4hwl6L23O02oGEARWdxbzZ


Christoph Lameter (Ampere) <cl@gentwo.org> writes:

> On Tue, 30 Apr 2024, Ankur Arora wrote:
>
>> ARCH_HAS_CPU_RELAX is a bit of a misnomer since all architectures
>> define cpu_relax(). Not all, however, have a performant version, with
>> some only implementing it as a compiler barrier.
>>
>> In contexts that this config option is used, it is expected to provide
>> an architectural primitive that can be used as part of a polling
>> mechanism -- one that would be cheaper than spinning in a tight loop.
>
> The intend of cpu_relax() is not a polling mechanism. Initial AFAICT it was
> introduced on x86 as the REP NOP instruction. Aka as PAUSE. And it was part of a
> spin loop. So there was no connection to polling anything.

Agreed, cpu_relax() is just a mechanism to tell the pipeline that
we are in a spin-loop.

> The intend was to make the processor aware that we are in a spin loop. Various
> processors have different actions that they take upon encountering such a cpu
> relax operation.

Sure, though most processors don't have a nice mechanism to do that.
x86 clearly has the REP; NOP thing. arm64 only has a YIELD which from my
measurements is basically a NOP when executed on a system without
hardware threads.

And that's why only x86 defines ARCH_HAS_CPU_RELAX.

> The polling (WFE/WFI) available on ARM (and potentially other platforms) is a
> different mechanism that is actually intended to reduce the power requirement of
> the processor until a certain condition is met and that check is done in
> hardware.

Sure. Which almost exactly fits the bill for the poll-idle loop -- except for the
timeout part.

> These are not the same and I think we need both config options.

My main concern is that poll_idle() conflates polling in idle with
ARCH_HAS_CPU_RELAX, when they aren't really related.

So, poll_idle(), and its users should depend on ARCH_HAS_OPTIMIZED_POLL
which, if defined by some architecture, means that poll_idle() would
be better than a spin-wait loop.

Beyond that I'm okay to keep ARCH_HAS_CPU_RELAX around.

That said, do you see a use for ARCH_HAS_CPU_RELAX? The only current
user is the poll-idle path.

> The issues that you have with WFET later in the patchset arise from not making
> this distinction.

Did you mean the issue with WFE? I'm not using WFET in this patchset at all.

With WFE, sure there's a problem in that you depend on an interrupt or
the event-stream to get out of the wait. And, so sometimes you would
overshoot the target poll timeout.

> The polling (waiting for an event) could be implemented for a processor not
> supporting that in hardware by using a loop that checks for the condition and
> then does a cpu_relax().

Yeah. That's exactly what patch-6 does. smp_cond_load_relaxed() uses
cpu_relax() internally in its spin-loop variant (non arm64).

On arm64, this would use LDXR; WFE. Or are you suggesting implementing
the arm64 loop via cpu_relax() (and thus YIELD?)

Ankur

> With that you could f.e. support the existing cpu_relax() and also have some
> form of cpu_poll() interface.