On 14.10.2022 10:03, Roger Pau Monné wrote:
> 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 <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.
>>
>> 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.
Perhaps we want to make spec_ctrl_{enter,exit}_idle() a no-op when
we're running virtualized ourselves?
Jan
