Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Mi, Dapeng1" <dapeng1.mi@intel.com>
To:     "Christopherson,, Sean" <seanjc@google.com>
CC:     "rafael@kernel.org" <rafael@kernel.org>,
        "daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "zhenyuw@linux.intel.com" <zhenyuw@linux.intel.com>
Subject: RE: [PATCH] KVM: x86: use TPAUSE to replace PAUSE in halt polling
Thread-Topic: [PATCH] KVM: x86: use TPAUSE to replace PAUSE in halt polling
Thread-Index: AQHYt5jfiwON7Q5LUE+jIx7HUxEKwK2+LJ2AgAFKPtA=
Date:   Thu, 25 Aug 2022 11:31:53 +0000
Message-ID: <PH0PR11MB4824201DABEFF588B4E0FE34CD729@PH0PR11MB4824.namprd11.prod.outlook.com>
References: <20220824091117.767363-1-dapeng1.mi@intel.com>
 <YwZDL4yv7F2Y4JBP@google.com>
In-Reply-To: <YwZDL4yv7F2Y4JBP@google.com>
Accept-Language: en-US
Content-Language: en-US
dlp-reaction: no-action
dlp-version: 11.6.500.17
dlp-product: dlpe-windows
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Jd9KSLqkNeWnTWyaHaIUByCNOzsd2ifDrvNPyOf2XqptP7Mdm7ywKctW6ZcR?=
 =?us-ascii?Q?X0lZv87jLx7kJ6ZkclsJNDmpotjMaHRgtyr0Q2r4dFkbmug43Uf5k59cJeLH?=
 =?us-ascii?Q?AbBAu7Z9NrbIS+Uee/io8bPMpDIoUejO/j+sn8dey2qDVkxBQes5o07pEOwo?=
 =?us-ascii?Q?aP9rj65lz3Lu2hydDj3PBl2TWQyBiFGIyUBJyaWoWROBpD3GowrSSiO8MTeQ?=
 =?us-ascii?Q?sYOCgZqv0HQmQeK9j2tz7fUac1/3ZSIAi8/x6hvcRt7dKpZH1Lp/w0FJaw/r?=
 =?us-ascii?Q?t2m8CPWcNE6Wh5eQdXy77sMh6qqX4GqGTe2gbeZr41HnxQImFimgTB1TdAxH?=
 =?us-ascii?Q?eHZqA6bE8ZUMpjxFjtHVLvy53GnJo/iwbdcWgehLRQTsRJHlrwPqESf5Fk+e?=
 =?us-ascii?Q?JZ6YUL7uXkijU/obiv08yaD9Ds7oMyZRG33YsuIX2PyDr6TX9IQ29Z5mX4Ot?=
 =?us-ascii?Q?4V82KBNsIXIMyus4o3LYWNmEuzXcXVdQ8zdslTFhkAk136XLyUuok7ugV5uj?=
 =?us-ascii?Q?J/+/5WrpI96C94MkYcXyyI6g4bc1A+nqZllY89tBF3tYBVHPTSOpMJLQwPnn?=
 =?us-ascii?Q?/GnMdlwUGp7ZU1aeJyklI80Chv5ANCirq+LUrrXUpqweYbda7dNbQBEW1YFy?=
 =?us-ascii?Q?tVlndwb6Li1jsq1uJ9QAtOSl/WTXNCVFF+1T/1r/OxhyFm1ifx6owDsJ3UCm?=
 =?us-ascii?Q?GFJjqYf5pGvvNeZ0kO80Qtof2ZTMmvpqlR3M6DiIciYMgxe85hZLSUDHB7fy?=
 =?us-ascii?Q?+9IFDSUeHWt7jdIhOfMB1EmMBNClM+OMgh8In8D31ifiHRyXMv+FlR4GbJ6w?=
 =?us-ascii?Q?YO+k1bL8nkE3lHm/cFsqXUOVQ+xqpgOKuAS4l6c97+pietdYGvHGMzQe1WPT?=
 =?us-ascii?Q?0bhC6rFNkyPiBuUdlHM7H/ozkxC70d+qpANBg4kXvLSN4BEsxMHVGAQUSrpN?=
 =?us-ascii?Q?a683lTRnaS/Auw9snuodVf6WobGcA3SPPfUPf9NAcSygjgcDqzn1tnViSBY0?=
 =?us-ascii?Q?OOVvt0T4t6RebsJjxPtLLMgOoyZt7tcGtCSDRu6hQgvdpMSbUV15OKQikJMx?=
 =?us-ascii?Q?gnZJEOjttEX5CG8Upou3KKobei2roxchcwuwGoBJAFIDMuheFze1nvKjdxF6?=
 =?us-ascii?Q?27P59pw+iTL2SCtaESxSE8F8jlIkAJgu78oDedw3wGmK1XEOL9E9dst5Bgv0?=
 =?us-ascii?Q?PwOveBggdr/E7HSEUJ6ckmRokhOX+yu/A9ml3UQpdIowFOH7NZgLuShuBUos?=
 =?us-ascii?Q?qqzsimsraPciLz4HUXgmg5zUWDD41bDX9ZS02K2HD+UJD4kJL4OHSJp2mvGd?=
 =?us-ascii?Q?3k2zUGQejhqNX99fYmjONLYstqCEqudOykKXQEzPmsp3+fyUoZg6w3I9JGYC?=
 =?us-ascii?Q?CyoKuFE26LTOIFYxDz+S7nxMgaeQY9KyxqUIKPNZ6SUQYLfzFQBWfLLg6abc?=
 =?us-ascii?Q?sfeUY/Hqjj5dxTNs29x+A0phW2wTScyrMO5dwFCIKGAiAAc/gB7Hjj1fMlOH?=
 =?us-ascii?Q?QDOxNq847DEyfaMQ/8Mg6VwnQYq9WgvrQpm6URAbO3Vd/PS2w3qVZHMYLybk?=
 =?us-ascii?Q?yUmX6xaZ1cAeR7iKbm4x3f4AKkoOo0V3oPwsN/5n?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4824.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 69e8e0aa-a3f9-423b-54a7-08da868d6919
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Aug 2022 11:31:53.7656
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: LNk/x1WWUk/wErQp6jYUt5tPszZH8zsnzPcoSi+LTBs+7yUXzK7B1myowoBqhun2kqQGrc0IUa8SVGODw2VsmA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6356
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Sean Christopherson <seanjc@google.com>
> Sent: Wednesday, August 24, 2022 11:27 PM
> To: Mi, Dapeng1 <dapeng1.mi@intel.com>
> Cc: rafael@kernel.org; daniel.lezcano@linaro.org; pbonzini@redhat.com; li=
nux-
> pm@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> zhenyuw@linux.intel.com
> Subject: Re: [PATCH] KVM: x86: use TPAUSE to replace PAUSE in halt pollin=
g
>=20
> On Wed, Aug 24, 2022, Dapeng Mi wrote:
> > TPAUSE is a new instruction on Intel processors which can instruct
> > processor enters a power/performance optimized state. Halt polling
> > uses PAUSE instruction to wait vCPU is waked up. The polling time
> > could be long and cause extra power consumption in some cases.
> >
> > Use TPAUSE to replace the PAUSE instruction in halt polling to get a
> > better power saving and performance.
>=20
> Better power savings, yes.  Better performance?  Not necessarily.  Using =
TPAUSE
> for  a "successful" halt poll is likely to yield _worse_ performance from=
 the
> vCPU's perspective due to the increased latency.

The Intel SDM says the C0.2 state which TPAUSE instruction currently enters=
 in kernel would "Improves performance of the other SMT thread(s) on the sa=
me core." For some multi-thread workload, it could improve the performance.=
 But considering the increased latency, I'm not sure how large the performa=
nce can improve. But we don't see obvious performance downgrade at least in=
 our tests.

>=20
> > Signed-off-by: Dapeng Mi <dapeng1.mi@intel.com>
> > ---
> >  drivers/cpuidle/poll_state.c |  3 ++-
> >  include/linux/kvm_host.h     | 20 ++++++++++++++++++++
> >  virt/kvm/kvm_main.c          |  2 +-
> >  3 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpuidle/poll_state.c
> > b/drivers/cpuidle/poll_state.c index f7e83613ae94..51ec333cbf80 100644
> > --- a/drivers/cpuidle/poll_state.c
> > +++ b/drivers/cpuidle/poll_state.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/sched/clock.h>
> >  #include <linux/sched/idle.h>
> > +#include <linux/kvm_host.h>
> >
> >  #define POLL_IDLE_RELAX_COUNT	200
> >
> > @@ -25,7 +26,7 @@ static int __cpuidle poll_idle(struct cpuidle_device =
*dev,
> >  		limit =3D cpuidle_poll_time(drv, dev);
> >
> >  		while (!need_resched()) {
> > -			cpu_relax();
> > +			kvm_cpu_poll_pause(limit);
>=20
> poll_idle() absolutely should not be calling into KVM code.

Ok, so how should we handle this case? Duplicate a piece of code?

>=20
> >  			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
> >  				continue;
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> > f4519d3689e1..810e749949b7 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -35,6 +35,7 @@
> >  #include <linux/interval_tree.h>
> >  #include <linux/rbtree.h>
> >  #include <linux/xarray.h>
> > +#include <linux/delay.h>
> >  #include <asm/signal.h>
> >
> >  #include <linux/kvm.h>
> > @@ -2247,6 +2248,25 @@ static inline void
> > kvm_handle_signal_exit(struct kvm_vcpu *vcpu)  }  #endif /*
> > CONFIG_KVM_XFER_TO_GUEST_WORK */
> >
> > +/*
> > + * This function is intended to replace the cpu_relax function in
> > + * halt polling. If TPAUSE instruction is supported, use TPAUSE
> > + * instead fo PAUSE to get better power saving and performance.
> > + * Selecting 1 us is a compromise between scheduling latency and
> > + * power saving time.
> > + */
> > +static inline void kvm_cpu_poll_pause(u64 timeout_ns) { #ifdef
> > +CONFIG_X86
>=20
> This is not preferred the way to insert arch-specific behavior into commo=
n KVM
> code.
> Assuming the goal is to avoid a function call, use an #ifndef here and th=
en
> #define the flag in x86's kvm_host.h, e.g.
>=20
> #ifndef CONFIG_HAVE_KVM_ARCH_HALT_POLL_PAUSE
> static inline kvm_cpu_halt_poll_pause(u64 timeout_ns) {
> 	cpu_relax();
> }
> #endif
>=20
> It's not obvious that we need to avoid a call here though, in which case =
a
>=20
>   __weak void kvm_arch_cpu_halt_poll_pause(struct kvm *kvm)
>   {
>=20
>   }
>=20
> with an x86 implementation will suffice.

Sure. Thanks.

>=20
>=20
> > +	if (static_cpu_has(X86_FEATURE_WAITPKG) && timeout_ns > 1000)
> > +		udelay(1);
>=20
> This is far too arbitrary.  Wake events from other vCPU are not necessari=
ly
> accompanied by an IRQ, which means that delaying for 1us may really truly
> delay for 1us before detecting a pending wake event.
>=20
> If this is something we want to utilize in KVM, it should be controllable=
 by
> userspace, probably via module param, and likely off by default.
>=20
> E.g.
>=20
>   unsigned int halt_poll_tpause_ns;
>=20
> and then
>=20
>   if (timeout_ns >=3D halt_poll_tpause_ns)
>   	udelay(halt_poll_tpause_ns);
>=20
> with halt_poll_tpause_ns zeroed out during setup if TPAUSE isn't supporte=
d.

I see. Thanks.

>=20
> I say "if", because I think this needs to come with performance numbers t=
o show
> the impact on guest latency so that KVM and its users can make an informe=
d
> decision.
> And if it's unlikely that anyone will ever want to enable TPAUSE for halt=
 polling,
> then it's not worth the extra complexity in KVM.

I ever run two scheduling related benchmarks, hackbench and schbench, I did=
n't see  there are obvious performance impact.

Here are the hackbench and schbench data on Intel ADL platform.

Hackbench 		base		TPAUSE		%delta
Group-1		0.056		0.052		7.14%
Group-4		0.165		0.164		0.61%
Group-8		0.313		0.309		1.28%
Group-16		0.834		0.842		-0.96%

Schbench - Latency percentiles (usec)		base 		TPAUSE=09
./schbench -m 1
	50.0th					15		13	=09
	99.0th					221		203
./schbench -m 2
	50.0th					26		23
	99.0th					16368		16544
./schbench -m 4
	50.0th					56		60
	99.0th					33984		34112

Anyway, I would run more performance tests and see whether there are obviou=
s performance impact.

>=20
> > +	else
> > +		cpu_relax();
> > +#else
> > +	cpu_relax();
> > +#endif
> > +}
> > +
> >  /*
> >   * This defines how many reserved entries we want to keep before we
> >   * kick the vcpu to the userspace to avoid dirty ring full.  This
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index
> > 584a5bab3af3..4afa776d21bd 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3510,7 +3510,7 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu)
> >  			 */
> >  			if (kvm_vcpu_check_block(vcpu) < 0)
> >  				goto out;
> > -			cpu_relax();
> > +			kvm_cpu_poll_pause(vcpu->halt_poll_ns);
>=20
> This is wrong, vcpu->halt_poll_ns is the total poll time, not the time re=
maining.
> E.g. if the max poll time is 1001 ns, and KVM has already waited for 1000=
 ns,
> then
> udelay(1) will cause KVM to wait for ~2000ns total.  There's always going=
 to be
> some amount of overrun, but overrun by a few ns is quite different than o=
verrun
> by a few thousand ns.

Yes, actually I calculate the remaining time and pass it to TPAUSE in earli=
er change. But this would make the code become complex comparing "cpu_relax=
". I'm concerning the complex code may impact the performance, so just simp=
ly it.

>=20
> >  			poll_end =3D cur =3D ktime_get();
> >  		} while (kvm_vcpu_can_poll(cur, stop));
> >  	}
> > --
> > 2.34.1
> >