Re: [PATCH v6 0/4] x86/speculation: Disable IBRS when idle

[Waiman Long <longman@redhat.com>]

On 7/27/23 14:45, Waiman Long wrote:
>   v6:
>    - Fix allyesconfig build error by moving __update_spec_ctrl()
>      helper from nospec-branch.h to spec-ctrl.h and include it in files
>      that need the helper.
>
>   v5:
>    - Update comment in patch 1.
>    - Minor doc update and code twist in patch 4 as suggested by Peter and
>      Randy.
>
>   v4:
>    - Add a new __update_spec_ctrl() helper in patch 1.
>    - Rebased to the latest linux kernel.
>
>   v3:
>    - Drop patches 1 ("x86/speculation: Provide a debugfs file to dump
>      SPEC_CTRL MSRs") and 5 ("x86/idle: Disable IBRS entering mwait idle
>      and enable it on wakeup") for now.
>    - Drop the MSR restoration code in ("x86/idle: Disable IBRS when cpu
>      is offline") as native_play_dead() does not return.
>    - For patch ("intel_idle: Add ibrs_off module parameter to force
>      disable IBRS"), change the name from "no_ibrs" to "ibrs_off" and
>      document the new parameter in intel_idle.rst.
>
> For Intel processors that need to turn on IBRS to protect against
> Spectre v2 and Retbleed, the IBRS bit in the SPEC_CTRL MSR affects
> the performance of the whole core even if only one thread is turning
> it on when running in the kernel. For user space heavy applications,
> the performance impact of occasionally turning IBRS on during syscalls
> shouldn't be significant. Unfortunately, that is not the case when the
> sibling thread is idling in the kernel. In that case, the performance
> impact can be significant.
>
> When DPDK is running on an isolated CPU thread processing network packets
> in user space while its sibling thread is idle. The performance of the
> busy DPDK thread with IBRS on and off in the sibling idle thread are:
>
>                                  IBRS on         IBRS off
>                                  -------         --------
>    packets/second:                  7.8M           10.4M
>    avg tsc cycles/packet:         282.26          209.86
>
> This is a 25% performance degradation. The test system is a Intel Xeon
> 4114 CPU @ 2.20GHz.
>
> Commit bf5835bcdb96 ("intel_idle: Disable IBRS during long idle")
> disables IBRS when the CPU enters long idle (C6 or below). However, there
> are existing users out there who have set "intel_idle.max_cstate=1"
> to decrease latency. Those users won't be able to benefit from this
> commit. This patch series extends this commit by providing a new
> "intel_idle.ibrs_off" module parameter to force disable IBRS even when
> "intel_idle.max_cstate=1" at the expense of increased IRQ response
> latency. It also includes a commit to allow the disabling of IBRS when
> a CPU becomes offline.
>
> Waiman Long (4):
>    x86/speculation: Add __update_spec_ctrl() helper
>    x86/idle: Disable IBRS when cpu is offline
>    intel_idle: Use __update_spec_ctrl() in intel_idle_ibrs()
>    intel_idle: Add ibrs_off module parameter to force disable IBRS
>
>   Documentation/admin-guide/pm/intel_idle.rst | 17 ++++++++++++++++-
>   arch/x86/include/asm/spec-ctrl.h            | 11 +++++++++++
>   arch/x86/kernel/smpboot.c                   |  8 ++++++++
>   drivers/idle/intel_idle.c                   | 18 +++++++++++++-----
>   4 files changed, 48 insertions(+), 6 deletions(-)
>
Peter,

Is this patch series good enough to be merged?

Thanks,
Longman