Date: Wed, 5 Jan 2022 10:26:13 -0700 From: Warner Losh <imp@bsdimp.com> To: Alexander Motin <mav@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 9e007a88d65b - main - atkbd: Reduce polling rate from 10Hz to ~1Hz. Message-ID: <CANCZdfrHtui-zGkhq-QEm0ANQD28n=39V3gy_3SDKcKCnZkXgg@mail.gmail.com> In-Reply-To: <202201051641.205GfWZV041757@gitrepo.freebsd.org> References: <202201051641.205GfWZV041757@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--0000000000009c5c2905d4d90db6 Content-Type: text/plain; charset="UTF-8" Maybe we default hw.atkbd.hz to 0. That will give us more info if this is even a thing still. And it would give users hit by this a no recompile fix and give us feedback as to how often this happens. We used to miss ISA interrupts in the early SMPNG days, and that's the time this change was introduced. The PIC does a good job of latching the state and we have no other drivers that have this workaround absent issues with the device itself. Warner On Wed, Jan 5, 2022, 9:41 AM Alexander Motin <mav@freebsd.org> wrote: > The branch main has been updated by mav: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=9e007a88d65ba0d23e73c3c052d474a78260d503 > > commit 9e007a88d65ba0d23e73c3c052d474a78260d503 > Author: Alexander Motin <mav@FreeBSD.org> > AuthorDate: 2022-01-05 16:32:44 +0000 > Commit: Alexander Motin <mav@FreeBSD.org> > CommitDate: 2022-01-05 16:41:26 +0000 > > atkbd: Reduce polling rate from 10Hz to ~1Hz. > > In my understanding this is only needed to workaround lost interrupts. > I was thinking to remove it completely, but the comment about edge- > triggered interrupt may be true and needs deeper investigation. ~1Hz > should be often enough to handle the supposedly rare loss cases, but > rare enough to not appear in top. Add sysctl hw.atkbd.hz to tune it. > > MFC after: 1 month > --- > sys/dev/atkbdc/atkbd.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/sys/dev/atkbdc/atkbd.c b/sys/dev/atkbdc/atkbd.c > index 40dd698984e3..cee1207df973 100644 > --- a/sys/dev/atkbdc/atkbd.c > +++ b/sys/dev/atkbdc/atkbd.c > @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$"); > #include <sys/proc.h> > #include <sys/limits.h> > #include <sys/malloc.h> > +#include <sys/sysctl.h> > > #include <machine/bus.h> > #include <machine/resource.h> > @@ -73,6 +74,13 @@ typedef struct atkbd_state { > #endif > } atkbd_state_t; > > +static SYSCTL_NODE(_hw, OID_AUTO, atkbd, CTLFLAG_RD | CTLFLAG_MPSAFE, 0, > + "AT keyboard"); > + > +static int atkbdhz = 1; > +SYSCTL_INT(_hw_atkbd, OID_AUTO, hz, CTLFLAG_RWTUN, &atkbdhz, 0, > + "Polling frequency (in hz)"); > + > static void atkbd_timeout(void *arg); > static int atkbd_reset(KBDC kbdc, int flags, int c); > > @@ -198,8 +206,11 @@ atkbd_timeout(void *arg) > kbdd_intr(kbd, NULL); > } > splx(s); > - state = (atkbd_state_t *)kbd->kb_data; > - callout_reset(&state->ks_timer, hz / 10, atkbd_timeout, arg); > + if (atkbdhz > 0) { > + state = (atkbd_state_t *)kbd->kb_data; > + callout_reset_sbt(&state->ks_timer, SBT_1S / atkbdhz, 0, > + atkbd_timeout, arg, C_PREL(1)); > + } > } > > /* LOW-LEVEL */ > --0000000000009c5c2905d4d90db6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"auto"><div>Maybe we default hw.atkbd.hz to 0. That will give us= more info if this is even a thing still. And it would give users hit by th= is a no recompile fix and give us feedback as to how often this happens.</d= iv><div dir=3D"auto"><br></div><div dir=3D"auto">We used to miss ISA interr= upts in the early SMPNG days, and that's the time this change was intro= duced. The PIC does a good job of latching the state and we have no other d= rivers that have this workaround absent issues with the device itself.</div= ><div dir=3D"auto"><br></div><div dir=3D"auto">Warner<br><br><div class=3D"= gmail_quote" dir=3D"auto"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jan= 5, 2022, 9:41 AM Alexander Motin <<a href=3D"mailto:mav@freebsd.org">ma= v@freebsd.org</a>> wrote:<br></div><blockquote class=3D"gmail_quote" sty= le=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The br= anch main has been updated by mav:<br> <br> URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D9e007a88d65ba0d23= e73c3c052d474a78260d503" rel=3D"noreferrer noreferrer" target=3D"_blank">ht= tps://cgit.FreeBSD.org/src/commit/?id=3D9e007a88d65ba0d23e73c3c052d474a7826= 0d503</a><br> <br> commit 9e007a88d65ba0d23e73c3c052d474a78260d503<br> Author:=C2=A0 =C2=A0 =C2=A0Alexander Motin <mav@FreeBSD.org><br> AuthorDate: 2022-01-05 16:32:44 +0000<br> Commit:=C2=A0 =C2=A0 =C2=A0Alexander Motin <mav@FreeBSD.org><br> CommitDate: 2022-01-05 16:41:26 +0000<br> <br> =C2=A0 =C2=A0 atkbd: Reduce polling rate from 10Hz to ~1Hz.<br> <br> =C2=A0 =C2=A0 In my understanding this is only needed to workaround lost in= terrupts.<br> =C2=A0 =C2=A0 I was thinking to remove it completely, but the comment about= edge-<br> =C2=A0 =C2=A0 triggered interrupt may be true and needs deeper investigatio= n.=C2=A0 ~1Hz<br> =C2=A0 =C2=A0 should be often enough to handle the supposedly rare loss cas= es, but<br> =C2=A0 =C2=A0 rare enough to not appear in top.=C2=A0 Add sysctl hw.atkbd.h= z to tune it.<br> <br> =C2=A0 =C2=A0 MFC after:=C2=A0 =C2=A0 =C2=A0 1 month<br> ---<br> =C2=A0sys/dev/atkbdc/atkbd.c | 15 +++++++++++++--<br> =C2=A01 file changed, 13 insertions(+), 2 deletions(-)<br> <br> diff --git a/sys/dev/atkbdc/atkbd.c b/sys/dev/atkbdc/atkbd.c<br> index 40dd698984e3..cee1207df973 100644<br> --- a/sys/dev/atkbdc/atkbd.c<br> +++ b/sys/dev/atkbdc/atkbd.c<br> @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");<br> =C2=A0#include <sys/proc.h><br> =C2=A0#include <sys/limits.h><br> =C2=A0#include <sys/malloc.h><br> +#include <sys/sysctl.h><br> <br> =C2=A0#include <machine/bus.h><br> =C2=A0#include <machine/resource.h><br> @@ -73,6 +74,13 @@ typedef struct atkbd_state {<br> =C2=A0#endif<br> =C2=A0} atkbd_state_t;<br> <br> +static SYSCTL_NODE(_hw, OID_AUTO, atkbd, CTLFLAG_RD | CTLFLAG_MPSAFE, 0,<b= r> +=C2=A0 =C2=A0 "AT keyboard");<br> +<br> +static int atkbdhz =3D 1;<br> +SYSCTL_INT(_hw_atkbd, OID_AUTO, hz, CTLFLAG_RWTUN, &atkbdhz, 0,<br> +=C2=A0 =C2=A0 "Polling frequency (in hz)");<br> +<br> =C2=A0static void=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 atkbd_timeout(vo= id *arg);<br> =C2=A0static int=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0atkbd_reset= (KBDC kbdc, int flags, int c);<br> <br> @@ -198,8 +206,11 @@ atkbd_timeout(void *arg)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 kbdd_intr(kbd, NULL);<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 splx(s);<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0state =3D (atkbd_state_t *)kbd->kb_data;<br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0callout_reset(&state->ks_timer, hz / 10,= atkbd_timeout, arg);<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (atkbdhz > 0) {<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0state =3D (atkbd_st= ate_t *)kbd->kb_data;<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0callout_reset_sbt(&= amp;state->ks_timer, SBT_1S / atkbdhz, 0,<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0atkbd= _timeout, arg, C_PREL(1));<br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}<br> =C2=A0}<br> <br> =C2=A0/* LOW-LEVEL */<br> </blockquote></div></div></div> --0000000000009c5c2905d4d90db6--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrHtui-zGkhq-QEm0ANQD28n=39V3gy_3SDKcKCnZkXgg>