On Sat, Jan 12, 2019 at 10:40:35PM -0600, Amit Kulkarni wrote:
> > 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.
>
> Thanks for the feedback Claudio and Mark!
>
> Here is a new diff based on your suggestions, looking for feedback, no tests
> requested yet. I assumed in below diff that 1hz == 1sec (confirmed in
> timeout_add_sec() in /sys/kern/kern_timeout.c), and converted some
> multiplication and some divisions, all related to hz. Did some conversions in
> comments also, because in future they might be used.
> -- amit
>
>
> diff --git sys/arch/armv7/exynos/exuart.c sys/arch/armv7/exynos/exuart.c
> index 4b0588750ea..15086bc5976 100644
> --- sys/arch/armv7/exynos/exuart.c
> +++ sys/arch/armv7/exynos/exuart.c
> @@ -283,7 +283,7 @@ exuart_intr(void *arg)
> if (p >= sc->sc_ibufend) {
> sc->sc_floods++;
> if (sc->sc_errors++ == 0)
> - timeout_add(&sc->sc_diag_tmo, 60 * hz);
> + timeout_add_sec(&sc->sc_diag_tmo, 60);
> } else {
> *p++ = c;
> if (p == sc->sc_ibufhigh && ISSET(tp->t_cflag, CRTSCTS))
> @@ -710,7 +710,7 @@ exuartclose(dev_t dev, int flag, int mode, struct proc *p)
> /* tty device is waiting for carrier; drop dtr then re-raise */
> //CLR(sc->sc_ucr3, EXUART_CR3_DSR);
> //bus_space_write_2(iot, ioh, EXUART_UCR3, sc->sc_ucr3);
> - timeout_add(&sc->sc_dtr_tmo, hz * 2);
> + timeout_add_sec(&sc->sc_dtr_tmo, 2);
> } else {
> /* no one else waiting; turn off the uart */
> exuart_pwroff(sc);
This one is OK.
> diff --git sys/arch/sparc64/dev/fd.c sys/arch/sparc64/dev/fd.c
> index 8d548062f83..654d8c95524 100644
> --- sys/arch/sparc64/dev/fd.c
> +++ sys/arch/sparc64/dev/fd.c
> @@ -1632,7 +1632,7 @@ loop:
> fdc->sc_state = RECALCOMPLETE;
> if (fdc->sc_flags & FDC_NEEDHEADSETTLE) {
> /* allow 1/30 second for heads to settle */
> - timeout_add(&fdc->fdcpseudointr_to, hz / 30);
> + timeout_add_msec(&fdc->fdcpseudointr_to, 33);
> return (1); /* will return later */
> }
>
Wonder if this should be 30 or 40 since 33 is rather odd.
> diff --git sys/dev/fdt/imxuart.c sys/dev/fdt/imxuart.c
> index 84c7eb5aee6..c2fd7e4a6d3 100644
> --- sys/dev/fdt/imxuart.c
> +++ sys/dev/fdt/imxuart.c
> @@ -228,7 +228,7 @@ imxuart_intr(void *arg)
> if (p >= sc->sc_ibufend) {
> sc->sc_floods++;
> if (sc->sc_errors++ == 0)
> - timeout_add(&sc->sc_diag_tmo, 60 * hz);
> + timeout_add_sec(&sc->sc_diag_tmo, 60);
> } else {
> *p++ = c;
> if (p == sc->sc_ibufhigh &&
> @@ -457,7 +457,7 @@ imxuart_softint(void *arg)
> if (ISSET(c, IMXUART_RX_OVERRUN)) {
> sc->sc_overflows++;
> if (sc->sc_errors++ == 0)
> - timeout_add(&sc->sc_diag_tmo, 60 * hz);
> + timeout_add_sec(&sc->sc_diag_tmo, 60);
> }
> /* This is ugly, but fast. */
>
> @@ -629,7 +629,7 @@ imxuartclose(dev_t dev, int flag, int mode, struct proc
> *p)
> /* tty device is waiting for carrier; drop dtr then re-raise */
> CLR(sc->sc_ucr3, IMXUART_CR3_DSR);
> bus_space_write_2(iot, ioh, IMXUART_UCR3, sc->sc_ucr3);
> - timeout_add(&sc->sc_dtr_tmo, hz * 2);
> + timeout_add_sec(&sc->sc_dtr_tmo, 2);
> } else {
> /* no one else waiting; turn off the uart */
> imxuart_pwroff(sc);
OK.
> diff --git sys/dev/ic/if_wi_hostap.c sys/dev/ic/if_wi_hostap.c
> index 64e3c10f3f5..155a391e7f9 100644
> --- sys/dev/ic/if_wi_hostap.c
> +++ sys/dev/ic/if_wi_hostap.c
> @@ -410,7 +410,7 @@ wihap_sta_timeout(void *v)
>
> /* Add wihap timeout if we have not already done so. */
> if (!timeout_pending(&whi->tmo))
> - timeout_add(&whi->tmo, hz / 10);
> + timeout_add_msec(&whi->tmo, 100);
>
> splx(s);
> }
OK.
> diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c
> index 0f024c0ad34..19bbb76f4a6 100644
> --- sys/dev/ic/pluart.c
> +++ sys/dev/ic/pluart.c
> @@ -225,7 +225,7 @@ pluart_intr(void *arg)
> if (p >= sc->sc_ibufend) {
> sc->sc_floods++;
> if (sc->sc_errors++ == 0)
> - timeout_add(&sc->sc_diag_tmo, 60 * hz);
> + timeout_add_sec(&sc->sc_diag_tmo, 60);
> } else {
> *p++ = c;
> if (p == sc->sc_ibufhigh && ISSET(tp->t_cflag,
> CRTSCTS)) {
> @@ -441,7 +441,7 @@ pluart_softint(void *arg)
> if (ISSET(c, IMXUART_RX_OVERRUN)) {
> sc->sc_overflows++;
> if (sc->sc_errors++ == 0)
> - timeout_add(&sc->sc_diag_tmo, 60 * hz);
> + timeout_add_sec(&sc->sc_diag_tmo, 60);
> }
> */
> /* This is ugly, but fast. */
> @@ -618,7 +618,7 @@ pluartclose(dev_t dev, int flag, int mode, struct proc *p)
> /* tty device is waiting for carrier; drop dtr then re-raise */
> //CLR(sc->sc_ucr3, IMXUART_CR3_DSR);
> //bus_space_write_4(iot, ioh, IMXUART_UCR3, sc->sc_ucr3);
> - timeout_add(&sc->sc_dtr_tmo, hz * 2);
> + timeout_add_sec(&sc->sc_dtr_tmo, 2);
> } else {
> /* no one else waiting; turn off the uart */
> pluart_pwroff(sc);
OK
> diff --git sys/dev/ic/w83l518d_sdmmc.c sys/dev/ic/w83l518d_sdmmc.c
> index 76c7a10c8d6..7c83510bed3 100644
> --- sys/dev/ic/w83l518d_sdmmc.c
> +++ sys/dev/ic/w83l518d_sdmmc.c
> @@ -578,7 +578,7 @@ wb_sdmmc_intr(struct wb_softc *wb)
> "\5CRC\6FIFO\7CARD\010PENDING");
>
> if (val & WB_INT_CARD)
> - timeout_add(&wb->wb_sdmmc_to, hz / 4);
> + timeout_add_msec(&wb->wb_sdmmc_to, 250);
>
> return 1;
> }
OK.
> diff --git sys/dev/pci/pccbb.c sys/dev/pci/pccbb.c
> index 642e7879b7f..b4980347f0b 100644
> --- sys/dev/pci/pccbb.c
> +++ sys/dev/pci/pccbb.c
> @@ -1234,7 +1234,7 @@ cb_pcmcia_poll(void *arg)
> u_int32_t spsr; /* socket present-state reg */
>
> timeout_set(&cb_poll_timeout, cb_pcmcia_poll, arg);
> - timeout_add(&cb_poll_timeout, hz / 10);
> + timeout_add_msec(&cb_poll_timeout, 100);
> switch (poll->level) {
> case IPL_NET:
> s = splnet();
OK.
> diff --git sys/net/if_pppoe.c sys/net/if_pppoe.c
> index e55316351fd..9e03310714b 100644
> --- sys/net/if_pppoe.c
> +++ sys/net/if_pppoe.c
> @@ -1346,7 +1346,7 @@ pppoe_tlf(struct sppp *sp)
> * function and defer disconnecting to the timeout handler.
> */
> sc->sc_state = PPPOE_STATE_CLOSING;
> - timeout_add(&sc->sc_timeout, hz / 50);
> + timeout_add_msec(&sc->sc_timeout, 20);
> }
>
> static void
I would wait with this one, since there are many more timeout_add() in the
file that also should be converted.
Anyone else wants to comment on this or give me an OK?
--
:wq Claudio
