* Paul Irofti <p...@irofti.net> le [14-08-2019 15:43:19 +0300]:
> On Wed, Aug 14, 2019 at 03:19:28PM +0200, prx wrote:
> > * Theo Buehler <t...@theobuehler.org> le [14-08-2019 07:23:01 +0200]:
> > > On Mon, Aug 12, 2019 at 02:34:55PM +0200, p...@ybad.name wrote:
> > > > >Synopsis: key input repeat speed too fast.
> > > > >Category: cwm
> > > > >Environment:
> > > > System : OpenBSD 6.6
> > > > Details : OpenBSD 6.6-beta (GENERIC.MP) #201: Sun Aug 11
> > > > 23:13:55 MDT 2019
> > > >
> > > > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > >
> > > > Architecture: OpenBSD.amd64
> > > > Machine : amd64
> > > > >Description:
> > > > Since last snapshot, when opening an X session with cwm, a
> > > > pressed key is
> > > > repeated multiple times.
> > > > Example : try to write "ls" in xterm produce "lllsss"
> > > >
> > > > Also, videos are played very fast (not sure it's related).
> > > > >How-To-Repeat:
> > > > Open a new X session under cwm and write anything.
> > > > >Fix:
> > >
> > > Thanks for the report.
> > >
> > > It would be helpful to know which commit triggered this issue. One of
> > > the suspicious commits is the TSC change. Could you apply the revert
> > > below on top of a -current tree, compile and install a new kernel
> > > (see release(8), step 2) and test if the problem persists?
> > >
> > > Index: sys/arch/amd64/amd64/cpu.c
> > > ===================================================================
> > > RCS file: /var/cvs/src/sys/arch/amd64/amd64/cpu.c,v
> > > retrieving revision 1.139
> > > diff -u -p -r1.139 cpu.c
> > > --- sys/arch/amd64/amd64/cpu.c 9 Aug 2019 15:20:04 -0000 1.139
> > > +++ sys/arch/amd64/amd64/cpu.c 12 Aug 2019 18:44:55 -0000
> > > @@ -1,4 +1,4 @@
> > > -/* $OpenBSD: cpu.c,v 1.139 2019/08/09 15:20:04 pirofti Exp $
> > > */
> > > +/* $OpenBSD: cpu.c,v 1.138 2019/08/07 18:53:28 guenther Exp $
> > > */
> > > /* $NetBSD: cpu.c,v 1.1 2003/04/26 18:39:26 fvdl Exp $ */
> > >
> > > /*-
> > > @@ -800,10 +800,6 @@ cpu_init(struct cpu_info *ci)
> > > cr4 = rcr4();
> > > lcr4(cr4 & ~CR4_PGE);
> > > lcr4(cr4);
> > > -
> > > - /* Synchronize TSC */
> > > - if (cold && !CPU_IS_PRIMARY(ci))
> > > - tsc_sync_ap(ci);
> > > #endif
> > > }
> > >
> > > @@ -858,7 +854,6 @@ void
> > > cpu_start_secondary(struct cpu_info *ci)
> > > {
> > > int i;
> > > - u_long s;
> > >
> > > ci->ci_flags |= CPUF_AP;
> > >
> > > @@ -879,18 +874,6 @@ cpu_start_secondary(struct cpu_info *ci)
> > > printf("dropping into debugger; continue from here to resume
> > > boot\n");
> > > db_enter();
> > > #endif
> > > - } else {
> > > - /*
> > > - * Synchronize time stamp counters. Invalidate cache and
> > > - * synchronize twice (in tsc_sync_bp) to minimize possible
> > > - * cache effects. Disable interrupts to try and rule out any
> > > - * external interference.
> > > - */
> > > - s = intr_disable();
> > > - wbinvd();
> > > - tsc_sync_bp(ci);
> > > - intr_restore(s);
> > > - printf("TSC skew=%lld\n", (long long)ci->ci_tsc_skew);
> > > }
> > >
> > > if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) {
> > > @@ -915,8 +898,6 @@ void
> > > cpu_boot_secondary(struct cpu_info *ci)
> > > {
> > > int i;
> > > - int64_t drift;
> > > - u_long s;
> > >
> > > atomic_setbits_int(&ci->ci_flags, CPUF_GO);
> > >
> > > @@ -929,17 +910,6 @@ cpu_boot_secondary(struct cpu_info *ci)
> > > printf("dropping into debugger; continue from here to resume
> > > boot\n");
> > > db_enter();
> > > #endif
> > > - } else if (cold) {
> > > - /* Synchronize TSC again, check for drift. */
> > > - drift = ci->ci_tsc_skew;
> > > - s = intr_disable();
> > > - wbinvd();
> > > - tsc_sync_bp(ci);
> > > - intr_restore(s);
> > > - drift -= ci->ci_tsc_skew;
> > > - printf("TSC skew=%lld drift=%lld\n",
> > > - (long long)ci->ci_tsc_skew, (long long)drift);
> > > - tsc_sync_drift(drift);
> > > }
> > > }
> > >
> > > @@ -964,14 +934,7 @@ cpu_hatch(void *v)
> > > panic("%s: already running!?", ci->ci_dev->dv_xname);
> > > #endif
> > >
> > > - /*
> > > - * Synchronize the TSC for the first time. Note that interrupts are
> > > - * off at this point.
> > > - */
> > > - wbinvd();
> > > ci->ci_flags |= CPUF_PRESENT;
> > > - ci->ci_tsc_skew = 0; /* reset on resume */
> > > - tsc_sync_ap(ci);
> > >
> > > lapic_enable();
> > > lapic_startclock();
> > > Index: sys/arch/amd64/amd64/tsc.c
> > > ===================================================================
> > > RCS file: /var/cvs/src/sys/arch/amd64/amd64/tsc.c,v
> > > retrieving revision 1.13
> > > diff -u -p -r1.13 tsc.c
> > > --- sys/arch/amd64/amd64/tsc.c 9 Aug 2019 15:20:05 -0000 1.13
> > > +++ sys/arch/amd64/amd64/tsc.c 12 Aug 2019 18:44:55 -0000
> > > @@ -1,10 +1,8 @@
> > > -/* $OpenBSD: tsc.c,v 1.13 2019/08/09 15:20:05 pirofti Exp $
> > > */
> > > +/* $OpenBSD: tsc.c,v 1.12 2019/08/03 14:57:51 jcs Exp $ */
> > > /*
> > > - * Copyright (c) 2008 The NetBSD Foundation, Inc.
> > > * Copyright (c) 2016,2017 Reyk Floeter <r...@openbsd.org>
> > > * Copyright (c) 2017 Adam Steen <a...@adamsteen.com.au>
> > > * Copyright (c) 2017 Mike Belopuhov <m...@openbsd.org>
> > > - * Copyright (c) 2019 Paul Irofti <piro...@openbsd.org>
> > > *
> > > * Permission to use, copy, modify, and distribute this software for any
> > > * purpose with or without fee is hereby granted, provided that the above
> > > @@ -22,7 +20,6 @@
> > > #include <sys/param.h>
> > > #include <sys/systm.h>
> > > #include <sys/timetc.h>
> > > -#include <sys/atomic.h>
> > >
> > > #include <machine/cpu.h>
> > > #include <machine/cpufunc.h>
> > > @@ -36,12 +33,6 @@ int tsc_recalibrate;
> > > uint64_t tsc_frequency;
> > > int tsc_is_invariant;
> > >
> > > -#define TSC_DRIFT_MAX 250
> > > -int64_t tsc_drift_observed;
> > > -
> > > -volatile int64_t tsc_sync_val;
> > > -volatile struct cpu_info *tsc_sync_cpu;
> > > -
> > > uint tsc_get_timecount(struct timecounter *tc);
> > >
> > > #include "lapic.h"
> > > @@ -190,8 +181,10 @@ calibrate_tsc_freq(void)
> > > return;
> > > tsc_frequency = freq;
> > > tsc_timecounter.tc_frequency = freq;
> > > +#ifndef MULTIPROCESSOR
> > > if (tsc_is_invariant)
> > > tsc_timecounter.tc_quality = 2000;
> > > +#endif
> > > }
> > >
> > > void
> > > @@ -210,13 +203,14 @@ cpu_recalibrate_tsc(struct timecounter *
> > > uint
> > > tsc_get_timecount(struct timecounter *tc)
> > > {
> > > - return rdtsc() + curcpu()->ci_tsc_skew;
> > > + return rdtsc();
> > > }
> > >
> > > void
> > > tsc_timecounter_init(struct cpu_info *ci, uint64_t cpufreq)
> > > {
> > > - if (!(ci->ci_flags & CPUF_CONST_TSC) ||
> > > + if (!(ci->ci_flags & CPUF_PRIMARY) ||
> > > + !(ci->ci_flags & CPUF_CONST_TSC) ||
> > > !(ci->ci_flags & CPUF_INVAR_TSC))
> > > return;
> > >
> > > @@ -226,7 +220,9 @@ tsc_timecounter_init(struct cpu_info *ci
> > > /* Newer CPUs don't require recalibration */
> > > if (tsc_frequency > 0) {
> > > tsc_timecounter.tc_frequency = tsc_frequency;
> > > +#ifndef MULTIPROCESSOR
> > > tsc_timecounter.tc_quality = 2000;
> > > +#endif
> > > } else {
> > > tsc_recalibrate = 1;
> > > tsc_frequency = cpufreq;
> > > @@ -234,103 +230,5 @@ tsc_timecounter_init(struct cpu_info *ci
> > > calibrate_tsc_freq();
> > > }
> > >
> > > - if (tsc_drift_observed > TSC_DRIFT_MAX) {
> > > - printf("ERROR: %lld cycle TSC drift observed\n",
> > > - (long long)tsc_drift_observed);
> > > - tsc_timecounter.tc_quality = -1000;
> > > - tsc_is_invariant = 0;
> > > - }
> > > -
> > > - printf("%s: TSC skew=%lld observed drift=%lld\n", __func__,
> > > - (long long)ci->ci_tsc_skew, (long long)tsc_drift_observed);
> > > -
> > > - if (ci->ci_flags & CPUF_PRIMARY)
> > > - tc_init(&tsc_timecounter);
> > > -}
> > > -
> > > -/*
> > > - * Record drift (in clock cycles). Called during AP startup.
> > > - */
> > > -void
> > > -tsc_sync_drift(int64_t drift)
> > > -{
> > > - if (drift < 0)
> > > - drift = -drift;
> > > - if (drift > tsc_drift_observed)
> > > - tsc_drift_observed = drift;
> > > -}
> > > -
> > > -/*
> > > - * Called during startup of APs, by the boot processor. Interrupts
> > > - * are disabled on entry.
> > > - */
> > > -void
> > > -tsc_read_bp(struct cpu_info *ci, uint64_t *bptscp, uint64_t *aptscp)
> > > -{
> > > - uint64_t bptsc;
> > > -
> > > - if (atomic_swap_ptr(&tsc_sync_cpu, ci) != NULL)
> > > - panic("tsc_sync_bp: 1");
> > > -
> > > - /* Flag it and read our TSC. */
> > > - atomic_setbits_int(&ci->ci_flags, CPUF_SYNCTSC);
> > > - bptsc = (rdtsc() >> 1);
> > > -
> > > - /* Wait for remote to complete, and read ours again. */
> > > - while ((ci->ci_flags & CPUF_SYNCTSC) != 0)
> > > - membar_consumer();
> > > - bptsc += (rdtsc() >> 1);
> > > -
> > > - /* Wait for the results to come in. */
> > > - while (tsc_sync_cpu == ci)
> > > - CPU_BUSY_CYCLE();
> > > - if (tsc_sync_cpu != NULL)
> > > - panic("tsc_sync_bp: 2");
> > > -
> > > - *bptscp = bptsc;
> > > - *aptscp = tsc_sync_val;
> > > -}
> > > -
> > > -void
> > > -tsc_sync_bp(struct cpu_info *ci)
> > > -{
> > > - uint64_t bptsc, aptsc;
> > > -
> > > - tsc_read_bp(ci, &bptsc, &aptsc); /* discarded - cache effects */
> > > - tsc_read_bp(ci, &bptsc, &aptsc);
> > > -
> > > - /* Compute final value to adjust for skew. */
> > > - ci->ci_tsc_skew = bptsc - aptsc;
> > > -}
> > > -
> > > -/*
> > > - * Called during startup of AP, by the AP itself. Interrupts are
> > > - * disabled on entry.
> > > - */
> > > -void
> > > -tsc_post_ap(struct cpu_info *ci)
> > > -{
> > > - uint64_t tsc;
> > > -
> > > - /* Wait for go-ahead from primary. */
> > > - while ((ci->ci_flags & CPUF_SYNCTSC) == 0)
> > > - membar_consumer();
> > > - tsc = (rdtsc() >> 1);
> > > -
> > > - /* Instruct primary to read its counter. */
> > > - atomic_clearbits_int(&ci->ci_flags, CPUF_SYNCTSC);
> > > - tsc += (rdtsc() >> 1);
> > > -
> > > - /* Post result. Ensure the whole value goes out atomically. */
> > > - (void)atomic_swap_64(&tsc_sync_val, tsc);
> > > -
> > > - if (atomic_swap_ptr(&tsc_sync_cpu, NULL) != ci)
> > > - panic("tsc_sync_ap");
> > > -}
> > > -
> > > -void
> > > -tsc_sync_ap(struct cpu_info *ci)
> > > -{
> > > - tsc_post_ap(ci);
> > > - tsc_post_ap(ci);
> > > + tc_init(&tsc_timecounter);
> > > }
> > > Index: sys/arch/amd64/include/cpu.h
> > > ===================================================================
> > > RCS file: /var/cvs/src/sys/arch/amd64/include/cpu.h,v
> > > retrieving revision 1.132
> > > diff -u -p -r1.132 cpu.h
> > > --- sys/arch/amd64/include/cpu.h 9 Aug 2019 15:20:05 -0000 1.132
> > > +++ sys/arch/amd64/include/cpu.h 12 Aug 2019 18:44:55 -0000
> > > @@ -1,4 +1,4 @@
> > > -/* $OpenBSD: cpu.h,v 1.132 2019/08/09 15:20:05 pirofti Exp $
> > > */
> > > +/* $OpenBSD: cpu.h,v 1.131 2019/05/17 19:07:16 guenther Exp $
> > > */
> > > /* $NetBSD: cpu.h,v 1.1 2003/04/26 18:39:39 fvdl Exp $ */
> > >
> > > /*-
> > > @@ -206,8 +206,6 @@ struct cpu_info {
> > > union vmm_cpu_cap ci_vmm_cap;
> > > paddr_t ci_vmxon_region_pa;
> > > struct vmxon_region *ci_vmxon_region;
> > > -
> > > - int64_t ci_tsc_skew; /* counter skew vs cpu0 */
> > > };
> > >
> > > #define CPUF_BSP 0x0001 /* CPU is the original BSP */
> > > @@ -223,7 +221,6 @@ struct cpu_info {
> > > #define CPUF_INVAR_TSC 0x0100 /* CPU has invariant TSC */
> > > #define CPUF_USERXSTATE 0x0200 /* CPU has curproc's xsave
> > > state */
> > >
> > > -#define CPUF_SYNCTSC 0x0800 /* Synchronize TSC */
> > > #define CPUF_PRESENT 0x1000 /* CPU is present */
> > > #define CPUF_RUNNING 0x2000 /* CPU is running */
> > > #define CPUF_PAUSE 0x4000 /* CPU is paused in DDB */
> > > Index: sys/arch/amd64/include/cpuvar.h
> > > ===================================================================
> > > RCS file: /var/cvs/src/sys/arch/amd64/include/cpuvar.h,v
> > > retrieving revision 1.10
> > > diff -u -p -r1.10 cpuvar.h
> > > --- sys/arch/amd64/include/cpuvar.h 9 Aug 2019 15:20:05 -0000
> > > 1.10
> > > +++ sys/arch/amd64/include/cpuvar.h 12 Aug 2019 18:44:55 -0000
> > > @@ -1,4 +1,4 @@
> > > -/* $OpenBSD: cpuvar.h,v 1.10 2019/08/09 15:20:05 pirofti Exp $
> > > */
> > > +/* $OpenBSD: cpuvar.h,v 1.9 2017/10/06 13:33:53 mikeb Exp $
> > > */
> > > /* $NetBSD: cpuvar.h,v 1.1 2003/03/01 18:29:28 fvdl Exp $ */
> > >
> > > /*-
> > > @@ -97,9 +97,5 @@ void identifycpu(struct cpu_info *);
> > > void cpu_init(struct cpu_info *);
> > > void cpu_init_first(void);
> > > void cpu_adjust_tsc_freq(uint64_t (*)());
> > > -
> > > -void tsc_sync_drift(int64_t);
> > > -void tsc_sync_bp(struct cpu_info *);
> > > -void tsc_sync_ap(struct cpu_info *);
> > >
> > > #endif
> > >
> >
> > Patch applied, no more issue now.
> >
> > However, even if /etc/sysctl.conf is empty, I notice :
> >
> > # sysctl |grep timecoun.*hard
> > kern.timecounter.hardware=acpihpet0
> >
> > So, by default, @phessler solution seems to be applied.
> >
> > Of course, telle me if I can provide any further data.
>
> See my message from 5 minutes ago :)
>
> With the patch applied, try and force the timecounter to tsc.
>
Just read it.
patch applied, timecounter to tsc in sysctl.conf, rebooted, and so
far, everything seems fine.
--
prx
