Date: Fri, 24 Feb 2017 13:45:41 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Jia-Shiun Li <jiashiun@gmail.com> Cc: John Baldwin <jhb@freebsd.org>, freebsd-current <freebsd-current@freebsd.org>, Konstantin Belousov <kib@freebsd.org> Subject: Re: TSC as timecounter makes system lag Message-ID: <20170224114541.GU2092@kib.kiev.ua> In-Reply-To: <CAHNYxxNB2QT4_h6RtMz9-sAc5br_VBWj6-NafSXuf88W56BmBQ@mail.gmail.com> References: <20170113120534.GC2349@kib.kiev.ua> <20170223100829.GR2092@kib.kiev.ua> <CAHNYxxPDQtu5oJw2FEibYPaxZb==8CiSYyX6i2CXoi5DDB7PEw@mail.gmail.com> <2204246.QKzIRnxiUQ@ralph.baldwin.cx> <CAHNYxxNB2QT4_h6RtMz9-sAc5br_VBWj6-NafSXuf88W56BmBQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Feb 24, 2017 at 12:15:26PM +0800, Jia-Shiun Li wrote:
> On Fri, Feb 24, 2017 at 12:55 AM, John Baldwin <jhb@freebsd.org> wrote:
>
> > On Thursday, February 23, 2017 11:04:58 PM Jia-Shiun Li wrote:
> > >
> > >
> > > This does not work.
> > >
> > > I added a printf before the outer if clause, and it says
> > >
> > > init_TSC_tc:546: deepest 00000000 vendor 00008086 amd_pminfo 00000000
> > >
> > > full boot dmesg attached. Looks init_TSC_tc() is called too early before
> > > acpi_cpu_attach() initializing cpu_deepest_sleep. Maybe it should be put
> > > after
> > > driver initialization, since it depends on probed ACPI C states?
> >
> > We don't actually need cpu_deepest_sleep. We could just set C2STOP always.
> > It doesn't hurt to have the flag set if the system only supports C1 except
> > that you get the printf in a verbose boot.
> >
> > Try this slight variation of Konstantin's patch. If this works we can
> > remove
> > cpu_deepest_sleep entirely as a followup since it will no longer be used:
> >
> > Index: tsc.c
> > ===================================================================
> > --- tsc.c (revision 314113)
> > +++ tsc.c (working copy)
> > @@ -542,9 +542,11 @@ init_TSC_tc(void)
> > * result incorrect runtimes for kernel idle threads (but not
> > * for any non-idle threads).
> > */
> > - if (cpu_deepest_sleep >= 2 && cpu_vendor_id == CPU_VENDOR_INTEL &&
> > + if (cpu_vendor_id == CPU_VENDOR_INTEL &&
> > (amd_pminfo & AMDPM_TSC_INVARIANT) == 0) {
> > tsc_timecounter.tc_flags |= TC_FLAGS_C2STOP;
> > + if (timecounter == &tsc_timecounter)
> > + cpu_disable_c2_sleep++;
> > if (bootverbose)
> > printf("TSC timecounter disables C2 and C3.\n");
> > }
> >
>
> Tested working on E7400 against r313909. And changing timecounter from/to
> TSC
> correctly enables/disables C2.
>
> The latter part cpu_disable_c2_sleep++ is not needed. When
> init_TSC_tc() got called timecounter is not yet tsc_timecounter.
> inittimecounter() later will do the work calling tc_windup().
>
You mean, just this
- if (cpu_deepest_sleep >= 2 && cpu_vendor_id == CPU_VENDOR_INTEL &&
+ if (cpu_vendor_id == CPU_VENDOR_INTEL &&
is enough to fix the issue ? If yes, we can remove the cpu_deepest_sleep
variable. This is John' observation, I think he would prefer to prepare
the patch.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170224114541.GU2092>