[PATCH v2] linux-gen: cpu_pause: added wfet instruction along with isb #2257
Conversation
Added wfet instruction in cpu_pause function along with isb instruction. These instructions are separated by compiler flags Signed-off-by: Sanjyot Vaidya <Sanjyot.Vaidya@arm.com>
odpbuild
changed the title
odpbuild
changed the title
| ARCH_ABI=default-linux | ||
| fi | ||
|
|
||
| AC_ARG_ENABLE([wfxt-cpu-pause], |
There was a problem hiding this comment.
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
| ARCH_ABI=default-linux | ||
| fi | ||
|
|
||
| AC_ARG_ENABLE([wfxt-cpu-pause], |
There was a problem hiding this comment.
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.
| static inline void _odp_cpu_pause(void) | ||
| { | ||
| #if defined(ENABLE_WFXT_CPU_PAUSE) && defined(__ARM_FEATURE_WFXT) | ||
| __asm__ volatile("sevl" : : : "memory"); |
There was a problem hiding this comment.
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?
| { | ||
| #if defined(ENABLE_WFXT_CPU_PAUSE) && defined(__ARM_FEATURE_WFXT) | ||
| __asm__ volatile("sevl" : : : "memory"); | ||
| __asm__ volatile("wfet %x0" : : "r"(100) : "memory"); |
There was a problem hiding this comment.
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?
2 participants
Added wfet instruction in cpu_pause function along with isb instruction.