[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/2] x86/mwait-idle: squash stats update when not actually entering C-state
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Date: Tue, 1 Feb 2022 13:42:37 +0100
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=MveKamJP1IIkhybTRw7omUmM7WLAurbY3BVisEryNS4=; b=CW4xmc6SbE62UJJ+rf0eZLhWjuvdaKwq1JO6blB5qtlJQaN6Etb3+x8SK5V4ekeAckte3tW6unrhmHJ9z8nIxTiSVTFghaMOshsWvddDjT4l9dWrGAZdAr0ANvVXcuelpcMeivdY5RsJVFYOPJ7NKQxRPS2DUz0iDU6V3AJvI3bARyiVT2/BzNLirLKOZR1OW1BdJqSFjtlHRr4s5B/iSqPiHOXxz5mi95QCaVFvMlBizxV1pIILADlEBYkFFgkmZQ87wVaIV2qc6Y8/K+wKb6g65YDXnr7rJtDAcji0REUj/2KtJxaYcG0EI9fEoKA7IXd8n67wNpMescJgjSKvng==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=V+r6wUL3KqI4GWB919uZrSodm+now+L9U8G/hwoDnY/PPVfE/T8G+8DFxQ2bcUQGVMvS5f3oy3FpNTdAX8JxutMR2iJURmBPRovIB3lK12sjyvFu57UEi5Dr5CbgN7BX+jqB4ILz/REMFkrHChIGufZrz1tPd9mJsPSSAL+f9lDP93Vn56w1/t2+STdzDuevyNBskgJ+qhPQmGkucsAnQTnF3jXUcFNjuKpgbbYnv1WKkduZgDaax+CmZ/ShcuWPSJ2L+09S4z39LAivAUsrfOcRyix2gdzzBFwHrsNCTAx12tMVbtiESqlOmaFi94t0u2axa3pVf6m1svK8+e7C9A==
- Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
- Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
- Delivery-date: Tue, 01 Feb 2022 12:42:52 +0000
- Ironport-data: A9a23:6UQOVK5ixP8et4Bz4KrHsAxRtN7AchMFZxGqfqrLsTDasY5as4F+v jEcUWmEb/2DNGryKotzbdi2pxsBvZ6ByN5iSVRk+y80Hi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuV3zyIQUBUjclkfJKlYAL/En03FV8MpBsJ00o5wbZj2tcw2LBVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Z6 OhgmKH3GCwTYKjspM4xTSgfSTxXIvgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALNs7kMZlZonh95TrYEewnUdbIRKCiCdpwgmxp3J4QQaq2i 8wxcQpLMzbGPkF0P0YpVrZlpuLyrGvdfGgNwL6SjfVuuDWCpOBr65DyNPLFd9rMQt9a9m6Iq 2SD82nnDxUyMN2E1SHD4n+qnvXIny7wRMQVDrLQ3vxgjUCXx2cTIAYLTlb9qv684nNSQPoGd RZSoHB36/Fvqgr7FbERQiFUvlakgzMxZp0BONdk7SGx4IbK0kHDG0EbG2sphMMdiOc6Qjkj1 1msltzvBCByvLD9dU9x5ot4vhvpZ3FLcDZqiTssCFJcvoK9+N1bYgfnE447eJNZmOEZDt0ZL 9qiiCElz4segscQv0lQ1QCW2mn8znQlo+Nc2+k2Yo5Hxl4jDGJGT9bxgbQ+0RqmBNzDJrVml CNc8/VyFMhUUfmweNWlGY3h5o2B6fefKyH7ilVyBZQn/DnF0yf9IdsNsG4mdBk4bpdsldrVj Kn741k5CHh7ZyPCUEOKS9jpV5RCIVbISLwJqcw4nvIRO8MsJWdrDQllZFKK3nCFraTfufpXB HtvSu71VSxyIf0+lFKeHr5BuZd2mHxW7T6NFPjTkkT2uZLDNSX9YepUbzOzghURsfnsTPP9q YgPbqNnCnx3DYXDX8Ug2dVCcAlXfSVnXs2eRg4+XrfrHzeK0VoJUpf56bggZ5Zkj+JSkOLJ9 Wu6QUhW1Bz0gnivFOlAQikLhGrHUcktoHQlEzYrOFr0iXEvbZz2tPUUdoctfKlh/+tmlKYmQ /4AcsSGI/JOVjWYpGhNMcij9NRvJEaxmAaDHyu5ezxjLZRucBPEp43/dQz1+ShQUifu7Zkio 6et3x/wSIYYQ1gwF97fbf+ilgvjvXUUlO9ocVHPJ91fJBfl/IRwcnSjhf4rOcAcbx7Ew2LCh QqRBB4Zo8jLopM0r4aV1fzV8d/xHrInTERAHmTd4bKnDgXg/zKukd1aTeKFXTHBT2eoqq+sU vpYkqPnO/odkVcU74clS+R3zbgz7sfErqNBylg2B23CalmmB+8yInSC2sUT5KRByqUA5FmzU 0OLvNJbJa+IKIXuF1tIfFgpaeGK1Pc1nDjO7KtqfBWmtXEvpLfXA19POxSsiTBGKOonOYwo9 u4tpcoK5lHtkREtKNuH0nhZ+mnkwqbsiEn7WkX22LPWtzc=
- Ironport-hdrordr: A9a23:4uotnqp7xJFXkEP/JsLnJuIaV5uzL9V00zEX/kB9WHVpm5Oj+P xGzc526farslsssREb+OxpOMG7MBThHLpOkPMs1NCZLXTbUQqTXfpfBO7ZrQEIdBeOlNK1uZ 0QFpSWTeeAcWSS7vyKkTVQcexQueVvmZrA7Yy1rwYPcegpUdAZ0+4QMHfkLqQcfnghOXNWLu v52iIRzADQBkj/I/7LTUUtbqzmnZnmhZjmaRkJC1oO7xSPtyqh7PrfHwKD1hkTfjtTyfN6mF K13jDR1+GGibWW2xXc32jc49B/n8bg8MJKAIiphtIOIjvhpw60bMBKWqGEvhoyvOazgWxa2u XkklMFBYBe+nnRdma6rV/E3BTh6i8n7zvYxVqRkRLY0LrEbQN/L/AEqZNScxPf5UZllsp7yr h302WQsIcSJQ/cnQzmjuK4GS1Cpw6Rmz4PgOQTh3tQXc81c7lKt7ES+0tTDdMpAD/60oY6C+ NjZfusq8q+SWnqL0wxg1Mfg+BFBh8Ib1W7qwk5y4CoOgFt7TFEJxBy/r1bop8CnKhNPKWsqd 60dpiAr4s+PfP+W5gNcNvpcfHHelAlfii8Ql56AW6XXZ3vaEi946Ie3t0OlZSXkdozvdwPpK g=
- Ironport-sdr: pjyNzUDOy2I6UTixsbdiVhblYN7hD7YO4Dmi0BOZiPkxgCza4Sp3IrsvOUmdaYZ0sEpy8o/UzK zQ5uG9N5W8Cvx6tIe77x1l9DmfND1myRgly3SsZtyon7x7I3b1zNPRYfPx7YXpqR07PONkOaQt njacv4P/K3as6CO0IqSlNGsTU8zrZo1t6MBtgSCJ7WTFtczn7LT7TgmDTFzut+Tn16b2+EpAk0 M/2iM6k60mQBhu+iIKKw4RlwoI0lbe/z8m9uFJouIgicnd3SchrFMx3xzYsZdxFyRJIvFoazv+ qI6kLGZRP61WK4rPZFcGTP2j
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On Tue, Feb 01, 2022 at 12:37:27PM +0100, Jan Beulich wrote:
> On 01.02.2022 12:04, Roger Pau Monné wrote:
> > On Thu, Jan 27, 2022 at 04:13:47PM +0100, Jan Beulich wrote:
> >> While we don't want to skip calling update_idle_stats(), arrange for it
> >> to not increment the overall time spent in the state we didn't really
> >> enter.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> RFC: If we wanted to also move the tracing, then I think the part ahead
> >> of the if() also would need moving. At that point we could as well
> >> move update_last_cx_stat(), too, which afaict would allow skipping
> >> update_idle_stats() on the "else" path (which therefore would go
> >> away). Yet then, with the setting of power->safe_state moved up a
> >> little (which imo it should have been anyway) the two
> >> cpu_is_haltable() invocations would only have the lapic_timer_off()
> >> invocation left in between. This would then seem to call for simply
> >> ditching the 2nd one - acpi-idle also doesn't have a 2nd instance.
> >
> > It's possible for lapic_timer_off to take a non-trivial amount of time
> > when virtualized, but it's likely we won't be using mwait in that
> > case, so not sure it matter much to have the two cpu_is_haltable calls
> > if there's just a lapic_timer_off between them.
> >
> >> TBD: For the tracing I wonder if that really needs to come ahead of the
> >> local_irq_enable(). Maybe trace_exit_reason() needs to, but quite
> >> certainly TRACE_6D() doesn't.
> >
> > Would be good if it could be moved after the local_irq_enable call, as
> > it's not as trivial as I've expected, and will just add latency to any
> > pending interrupt waiting to be serviced. FWIW, I haven't spotted a
> > need to call it with interrupt disabled.
>
> Okay, I guess I'll to the larger rework then.
>
> >> --- a/xen/arch/x86/cpu/mwait-idle.c
> >> +++ b/xen/arch/x86/cpu/mwait-idle.c
> >> @@ -854,17 +854,23 @@ static void mwait_idle(void)
> >> mwait_idle_with_hints(cx->address, MWAIT_ECX_INTERRUPT_BREAK);
> >>
> >> local_irq_disable();
> >> - }
> >>
> >> - after = alternative_call(cpuidle_get_tick);
> >> + after = alternative_call(cpuidle_get_tick);
> >> +
> >> + cstate_restore_tsc();
> >> +
> >> + /* Now back in C0. */
> >> + update_idle_stats(power, cx, before, after);
> >> + } else {
> >> + /* Never left C0. */
> >> + after = alternative_call(cpuidle_get_tick);
> >> + update_idle_stats(power, cx, after, after);
> >
> > While adjusting this, could you also modify update_idle_stats to avoid
> > increasing cx->usage if before == after (or !sleep_ticks). I don't
> > think it's fine to increase the state counter if we never actually
> > entered it.
>
> I did consider it but then decided against. Even leaving this aspect
> aside the counter only counts _attempts_ to enter a certain state;
> the CPU may find reasons to never actually enter it. And what we have
> when before == after is still an attempt, albeit an unsuccessful one.
Right, in which case:
Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Not sure whether you would like to commit this now and do the lager
rework as a followup patch. That would be fine by me.
Thanks, Roger.
|