On Sun, Jan 06, 2019 at 01:43:19PM -0600, Amit Kulkarni wrote:
> Hi,
>
> Referring to the end of mpi's message, and also mlarkin@ later comment
> https://marc.info/?l=openbsd-tech&m=154577028830964&w=2
>
> I am trying to replace some easy timeout_add() calls with timeout_add_msec().
>
> My current understanding with the occurences of timeout_add() in the tree is
> that: if there is a hardcoded call like timeout_add(struct timeout, 1), then
> replace with timeout_add_msec(struct timeout, 10). That is, 1 tick = 10 msec.
>
> So if there's a hardcoded call like timeout_add(struct timeout, 5), then
> replace with timeout_add_msec(struct timeout, 50).
>
> If there are hz calculations which I don't understand like for example in
> /sys/arch/alpha/tc/ioasic.c, then I am skipping these for now.
> if (alpha_led_blink != 0) {
> timeout_set(&led_blink_state.tmo, ioasic_led_blink, NULL);
> timeout_add(&led_blink_state.tmo,
> (((averunnable.ldavg[0] + FSCALE) * hz) >> (FSHIFT + 3)));
> }
>
> A call like timeout_add(struct timeout, 0) is replaced by an equivalent call
> to timeout_add_msec(struct timeout, 0).
>
> Both the above scenarios are in the following diff and un-tested (not
> compiled also, for now), no way I can test some of these, as I don't have
> access to hardware. Mainly looking for critical review and feedback to get
> this going in the right direction.
>
> Thanks for your time!
You started to convert the wrong timeout_add() calls. Doing a blind
timeout_add() to timeout_add_msec() conversion especially on calls with 0
or 1 tick sleep time is the wrong approach.
The right approach is to identify calls that do sleep for a well defined time
(1sec, 50ms or whatever) and convert those. Most of the timeouts you
changes are not in that category. First I would look at timeouts that have
a multiplication with hz in them since those wait for a specific time.
> diff --git arch/alpha/alpha/promcons.c arch/alpha/alpha/promcons.c
> index 9efabd3bf1c..b872f6e3931 100644
> --- arch/alpha/alpha/promcons.c
> +++ arch/alpha/alpha/promcons.c
> @@ -100,7 +100,7 @@ promopen(dev, flag, mode, p)
> error = (*linesw[tp->t_line].l_open)(dev, tp, p);
> if (error == 0 && setuptimeout) {
> timeout_set(&prom_to, promtimeout, tp);
> - timeout_add(&prom_to, 1);
> + timeout_add_msec(&prom_to, 10);
> }
> return error;
> }
> @@ -220,7 +220,7 @@ promtimeout(v)
> if (tp->t_state & TS_ISOPEN)
> (*linesw[tp->t_line].l_rint)(c, tp);
> }
> - timeout_add(&prom_to, 1);
> + timeout_add_msec(&prom_to, 10);
> }
>
> struct tty *
> diff --git arch/amd64/isa/clock.c arch/amd64/isa/clock.c
> index db516d9ecde..9d5934e6817 100644
> --- arch/amd64/isa/clock.c
> +++ arch/amd64/isa/clock.c
> @@ -326,7 +326,7 @@ rtcstart(void)
> mc146818_write(NULL, MC_REGB, MC_REGB_24HR | MC_REGB_PIE);
>
> /*
> - * On a number of i386 systems, the rtc will fail to start when booting
> + * On a number of amd64 systems, the rtc will fail to start when booting
> * the system. This is due to us missing to acknowledge an interrupt
> * during early stages of the boot process. If we do not acknowledge
> * the interrupt, the rtc clock will not generate further interrupts.
> @@ -334,7 +334,7 @@ rtcstart(void)
> * to drain any un-acknowledged rtc interrupt(s).
> */
> timeout_set(&rtcdrain_timeout, rtcdrain, (void *)&rtcdrain_timeout);
> - timeout_add(&rtcdrain_timeout, 1);
> + timeout_add_msec(&rtcdrain_timeout, 10);
> }
>
> void
> diff --git arch/amd64/pci/pchb.c arch/amd64/pci/pchb.c
> index 6e599d7be4a..80b5ada1cb4 100644
> --- arch/amd64/pci/pchb.c
> +++ arch/amd64/pci/pchb.c
> @@ -332,7 +332,7 @@ pchb_rnd(void *v)
> }
> }
>
> - timeout_add(&sc->sc_rng_to, 1);
> + timeout_add(&sc->sc_rng_to, 10);
> }
>
> void
> diff --git dev/ic/vga.c dev/ic/vga.c
> index 74cc3e07bf8..2cebb65d2d5 100644
> --- dev/ic/vga.c
> +++ dev/ic/vga.c
> @@ -765,7 +765,7 @@ vga_show_screen(void *v, void *cookie, int waitok, void
> (*cb)(void *, int, int),
> if (cb) {
> timeout_set(&vc->vc_switch_timeout,
> (void(*)(void *))vga_doswitch, vc);
> - timeout_add(&vc->vc_switch_timeout, 0);
> + timeout_add(&vc->vc_switch_timeout, 10);
> return (EAGAIN);
> }
>
> diff --git net/if_pfsync.c net/if_pfsync.c
> index 8d842e48466..dc8d2f41466 100644
> --- net/if_pfsync.c
> +++ net/if_pfsync.c
> @@ -2318,7 +2318,7 @@ pfsync_bulk_start(void)
> sc->sc_bulk_last = sc->sc_bulk_next;
>
> pfsync_bulk_status(PFSYNC_BUS_START);
> - timeout_add(&sc->sc_bulk_tmo, 0);
> + timeout_add_msec(&sc->sc_bulk_tmo, 0);
> }
> }
>
>
>
>
--
:wq Claudio
