On Thu, Oct 13, 2022 at 04:15:04PM +0200, Jan Beulich wrote: > On 13.10.2022 15:10, Roger Pau Monné wrote: > > 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 <[email protected]> > >>>> > >>>> 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 <[email protected]> > >>>> Signed-off-by: Peter Zijlstra (Intel) <[email protected]> > >>>> Signed-off-by: Borislav Petkov <[email protected]> > >>>> Reviewed-by: Josh Poimboeuf <[email protected]> > >>>> Signed-off-by: Borislav Petkov <[email protected]> > >>>> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > >>>> bf5835bcdb96 > >>>> Signed-off-by: Jan Beulich <[email protected]> > >>> > >>> Acked-by: Roger Pau Monné <[email protected]> > >> > >> 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. > > Correct. But it's also code not being altered by this commit.
Indeed, that's why it's an unrelated comment. I was just wondering whether we should drop those or not in a separate patch. I'm concerned over hitting that path on a virtualized environment, where changing the spec controls is likely not that cheap. Thanks, Roger.
