On Thu, Oct 13, 2022 at 02:17:54PM +0200, Jan Beulich wrote:
> On 13.10.2022 14:03, Roger Pau Monné wrote:
> > On Thu, Aug 18, 2022 at 03:04:51PM +0200, Jan Beulich wrote:
> >> From: Peter Zijlstra <pet...@infradead.org>
> >>
> >> Having IBRS enabled while the SMT sibling is idle unnecessarily slows
> >> down the running sibling. OTOH, disabling IBRS around idle takes two
> >> MSR writes, which will increase the idle latency.
> >>
> >> Therefore, only disable IBRS around deeper idle states. Shallow idle
> >> states are bounded by the tick in duration, since NOHZ is not allowed
> >> for them by virtue of their short target residency.
> >>
> >> Only do this for mwait-driven idle, since that keeps interrupts disabled
> >> across idle, which makes disabling IBRS vs IRQ-entry a non-issue.
> >>
> >> Note: C6 is a random threshold, most importantly C1 probably shouldn't
> >> disable IBRS, benchmarking needed.
> >>
> >> Suggested-by: Tim Chen <tim.c.c...@linux.intel.com>
> >> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> >> Signed-off-by: Borislav Petkov <b...@suse.de>
> >> Reviewed-by: Josh Poimboeuf <jpoim...@kernel.org>
> >> Signed-off-by: Borislav Petkov <b...@suse.de>
> >> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >> bf5835bcdb96
> >> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> >
> > Acked-by: Roger Pau Monné <roger....@citrix.com>
>
> Thanks.
>
> > One unrelated comment below.
> > [...]
> >> @@ -932,8 +939,6 @@ static void cf_check mwait_idle(void)
> >> pm_idle_save();
> >> else
> >> {
> >> - struct cpu_info *info = get_cpu_info();
> >> -
> >> spec_ctrl_enter_idle(info);
> >> safe_halt();
> >> spec_ctrl_exit_idle(info);
> >
> > Do we need to disable speculation just for the hlt if there's no
> > C state change?
> >
> > It would seem to me like the MSR writes could add a lot of latency
> > when there's no C state change.
>
> HLT enters (at least) C1, so is a C-state change to me as well. Plus
> we may remain there for a while, and during that time we'd like to
> not unduly impact the other thread.
OK, but it's not a "deeper C state" as mentioned in the commit
message.
Thanks, Roger.
