Skip to content

[PATCH v2] linux-gen: cpu_pause: added wfet instruction along with isb #2257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sanvai01 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[PATCH v2] linux-gen: cpu_pause: added wfet instruction along with isb #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5c10c9
linux-gen: cpu_pause: added wfet instruction along with isb
sanvai01 Sep 11, 2025
51ac996
Merge branch 'master' into added_wfet_instruction_in_cpu_pause
sanvai01 Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,17 @@ if test "x$enable_host_optimization" = "xno" ; then
ARCH_ABI=default-linux
fi

AC_ARG_ENABLE([wfxt-cpu-pause],
Copy link
Collaborator

@JannePeltonen JannePeltonen Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message could be improved:

  • The rationale of the use of WFET in odp_cpu_pause() could be explained
  • The function is not cpu_pause but odp_cpu_pause.
  • Use of WFET instead of ISB is selected through configure flag, not compiler flag
  • The title (and often the message body too) are typically in the imperative form in ODP git

Copy link
Collaborator

@JannePeltonen JannePeltonen Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ODP pull requests are not supposed to contain merge commits as ODP development in this repository is based on rebases and linear commit history. So could you please rebase your branch on top of the latest master instead of merging it.

[AS_HELP_STRING([--enable-wfxt-cpu-pause],
[Enable WFXT support in cpu_pause code])],
[enable_wfxt_cpu_pause=$enableval],
[enable_wfxt_cpu_pause=no])
AM_CONDITIONAL([ENABLE_WFXT_CPU_PAUSE], [test "x$enable_wfxt_cpu_pause" = "xyes"])

if test "x$enable_wfxt_cpu_pause" = "xyes"; then
AC_DEFINE([ENABLE_WFXT_CPU_PAUSE], [1], [Enable WFXT in cpu_pause])
fi

##########################################################################
# Warn on the defaults if arch is undefined
##########################################################################
Expand Down
6 changes: 6 additions & 0 deletions platform/linux-generic/arch/aarch64/odp/api/abi/cpu_inlines.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define ODP_ARCH_CPU_INLINES_H_

#include <odp/api/abi/time_cpu.h>
#include <odp/autoheader_build.h>

#include <stdint.h>

Expand All @@ -28,12 +29,17 @@ extern _odp_cpu_cycles_global_t _odp_cpu_cycles_glob;

static inline void _odp_cpu_pause(void)
{
#if defined(ENABLE_WFXT_CPU_PAUSE) && defined(__ARM_FEATURE_WFXT)
__asm__ volatile("sevl" : : : "memory");
Copy link
Collaborator

@JannePeltonen JannePeltonen Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the SEVL done here? Doesn't is basically defeat the purpose of the following WFET as the WFET would wake immediately and not wait for the timeout? If the SEVL is intentional and serves a purpose, then maybe that could be explained in a comment, unless it is just me who cannot see it. But if the SEVL stays, then why WFET instead of just WFE which would be available in much earlier arch levels?

__asm__ volatile("wfet %x0" : : "r"(100) : "memory");
Copy link
Collaborator

@JannePeltonen JannePeltonen Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the delay is 100? I presume the unit is clock cycles. The constant could be defined as a macro outside the inline asm and have some comment about why the particular value is selected. I wonder if the timeout of 100 cycles adds too much unwanted latency in some uses of odp_cpu_pause().

odp_cpu_pause() is being used in quite many places in the linux-gen ODP implementation. It appears that many of the call sites could use WFE through _odp_wait_until_equal_acq_u32() so that they would not spin unnecessarily and would resume with lower latency when ready. Maybe many of the call sites should first be changed before making them use WFET through the odp_cpu_pause() call?

#else
/* YIELD hints the CPU to switch to another thread if possible
* and executes as a NOP otherwise.
* ISB flushes the pipeline, then restarts. This is guaranteed to
* stall the CPU a number of cycles.
*/
__asm volatile("isb" ::: "memory");
#endif
}

static inline uint64_t _odp_cpu_cycles(void)
Expand Down
Loading