Re: [PATCH v3 4/5] x86/mwait-idle: disable IBRS during long idle

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <peterz@xxxxxxxxxxxxx>
> >>>>
> >>>> 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.chen@xxxxxxxxxxxxxxx>
> >>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> >>>> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> >>>> Reviewed-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> >>>> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> >>>> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> >>>> bf5835bcdb96
> >>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> 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.