Re: ipmi(4): don't block interrupts/systq long time
hi patrick keshishian,
From: patrick keshishian
Subject: Re: ipmi(4): don't block interrupts/systq long time
Date: Fri, 7 Jun 2019 19:24:15 -0700
>> Index: sys/dev/ipmi.c
>> ===
>> RCS file: /cvs/src/sys/dev/ipmi.c,v
>> retrieving revision 1.102
>> diff -u -p -r1.102 ipmi.c
>> --- sys/dev/ipmi.c 15 Jun 2018 12:21:41 - 1.102
>> +++ sys/dev/ipmi.c 7 Jun 2019 10:00:10 -
>> @@ -1037,14 +1037,10 @@ ipmi_cmd(struct ipmi_cmd *c)
>> void
>> ipmi_cmd_poll(struct ipmi_cmd *c)
>> {
>> -mtx_enter(&c->c_sc->sc_cmd_mtx);
>> -
>> if ((c->c_ccode = ipmi_sendcmd(c)))
>> printf("%s: sendcmd fails\n", DEVNAME(c->c_sc));
>> else
>> c->c_ccode = ipmi_recvcmd(c);
>> -
>> -mtx_leave(&c->c_sc->sc_cmd_mtx);
>> }
>>
>> void
>> @@ -1653,7 +1649,6 @@ ipmi_match(struct device *parent, void *
>>
>> /* XXX local softc is wrong wrong wrong */
>> sc = malloc(sizeof(*sc), M_TEMP, M_WAITOK | M_ZERO);
>> -mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
>> strlcpy(sc->sc_dev.dv_xname, "ipmi0", sizeof(sc->sc_dev.dv_xname));
>>
>> /* Map registers */
>> @@ -1693,15 +1688,31 @@ ipmi_attach(struct device *parent, struc
>> /* Map registers */
>> ipmi_map_regs(sc, ia);
>>
>> +/* Setup taskqs */
>> +sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
>> +if (sc->sc_cmd_taskq == NULL) {
>> +printf(": unable to allocate cmd taskq\n");
>> +return;
>> +}
>> +sc->sc_wdog_taskq = taskq_create("ipmiwdog", 1, IPL_SOFTCLOCK, 0);
>> +if (sc->sc_cmd_taskq == NULL) {
>
> paste-o?
ah, you are right, thank you very much!
here is revised patch.
Index: sys/dev/ipmi.c
===
RCS file: /cvs/src/sys/dev/ipmi.c,v
retrieving revision 1.102
diff -u -p -r1.102 ipmi.c
--- sys/dev/ipmi.c 15 Jun 2018 12:21:41 - 1.102
+++ sys/dev/ipmi.c 9 Jun 2019 10:39:21 -
@@ -1037,14 +1037,10 @@ ipmi_cmd(struct ipmi_cmd *c)
void
ipmi_cmd_poll(struct ipmi_cmd *c)
{
- mtx_enter(&c->c_sc->sc_cmd_mtx);
-
if ((c->c_ccode = ipmi_sendcmd(c)))
printf("%s: sendcmd fails\n", DEVNAME(c->c_sc));
else
c->c_ccode = ipmi_recvcmd(c);
-
- mtx_leave(&c->c_sc->sc_cmd_mtx);
}
void
@@ -1653,7 +1649,6 @@ ipmi_match(struct device *parent, void *
/* XXX local softc is wrong wrong wrong */
sc = malloc(sizeof(*sc), M_TEMP, M_WAITOK | M_ZERO);
- mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
strlcpy(sc->sc_dev.dv_xname, "ipmi0", sizeof(sc->sc_dev.dv_xname));
/* Map registers */
@@ -1693,15 +1688,31 @@ ipmi_attach(struct device *parent, struc
/* Map registers */
ipmi_map_regs(sc, ia);
+ /* Setup taskqs */
+ sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
+ if (sc->sc_cmd_taskq == NULL) {
+ printf(": unable to allocate cmd taskq\n");
+ return;
+ }
+ sc->sc_wdog_taskq = taskq_create("ipmiwdog", 1, IPL_SOFTCLOCK, 0);
+ if (sc->sc_wdog_taskq == NULL) {
+ taskq_destroy(sc->sc_cmd_taskq);
+ printf(": unable to allocate wdog taskq\n");
+ return;
+ }
+ task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
+
+ /* Setup thread */
sc->sc_thread = malloc(sizeof(struct ipmi_thread), M_DEVBUF, M_NOWAIT);
if (sc->sc_thread == NULL) {
+ taskq_destroy(sc->sc_cmd_taskq);
+ taskq_destroy(sc->sc_wdog_taskq);
printf(": unable to allocate thread\n");
return;
}
sc->sc_thread->sc = sc;
sc->sc_thread->running = 1;
- /* Setup threads */
kthread_create_deferred(ipmi_create_thread, sc);
printf(": version %d.%d interface %s %sbase 0x%x/%x spacing %d",
@@ -1717,16 +1728,12 @@ ipmi_attach(struct device *parent, struc
/* Setup Watchdog timer */
sc->sc_wdog_period = 0;
- task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
wdog_register(ipmi_watchdog, sc);
rw_init(&sc->sc_ioctl.lock, DEVNAME(sc));
sc->sc_ioctl.req.msgid = -1;
c->c_sc = sc;
c->c_cc
Re: ipmi(4): don't block interrupts/systq long time
On Fri, Jun 07, 2019 at 07:29:19PM +0900, Naoki Fukaumi wrote:
> hi tech@,
>
> here is another patch for another issue for ipmi(4).
>
> ipmi_sendcmd() and ipmi_recvcmd() are always called in order/pairs as a
> single task of a single-threaded taskq, remove mutex from there. this
> avoids interrupts blocked long time.
>
> task for watchdog can be long lived, use own taskq instead of systq.
>
> --
> FUKAUMI Naoki
>
> Index: sys/dev/ipmi.c
> ===
> RCS file: /cvs/src/sys/dev/ipmi.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 ipmi.c
> --- sys/dev/ipmi.c15 Jun 2018 12:21:41 - 1.102
> +++ sys/dev/ipmi.c7 Jun 2019 10:00:10 -
> @@ -1037,14 +1037,10 @@ ipmi_cmd(struct ipmi_cmd *c)
> void
> ipmi_cmd_poll(struct ipmi_cmd *c)
> {
> - mtx_enter(&c->c_sc->sc_cmd_mtx);
> -
> if ((c->c_ccode = ipmi_sendcmd(c)))
> printf("%s: sendcmd fails\n", DEVNAME(c->c_sc));
> else
> c->c_ccode = ipmi_recvcmd(c);
> -
> - mtx_leave(&c->c_sc->sc_cmd_mtx);
> }
>
> void
> @@ -1653,7 +1649,6 @@ ipmi_match(struct device *parent, void *
>
> /* XXX local softc is wrong wrong wrong */
> sc = malloc(sizeof(*sc), M_TEMP, M_WAITOK | M_ZERO);
> - mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
> strlcpy(sc->sc_dev.dv_xname, "ipmi0", sizeof(sc->sc_dev.dv_xname));
>
> /* Map registers */
> @@ -1693,15 +1688,31 @@ ipmi_attach(struct device *parent, struc
> /* Map registers */
> ipmi_map_regs(sc, ia);
>
> + /* Setup taskqs */
> + sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
> + if (sc->sc_cmd_taskq == NULL) {
> + printf(": unable to allocate cmd taskq\n");
> + return;
> + }
> + sc->sc_wdog_taskq = taskq_create("ipmiwdog", 1, IPL_SOFTCLOCK, 0);
> + if (sc->sc_cmd_taskq == NULL) {
paste-o?
> + taskq_destroy(sc->sc_cmd_taskq);
> + printf(": unable to allocate wdog taskq\n");
> + return;
> + }
> + task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
> +
> + /* Setup thread */
> sc->sc_thread = malloc(sizeof(struct ipmi_thread), M_DEVBUF, M_NOWAIT);
> if (sc->sc_thread == NULL) {
> + taskq_destroy(sc->sc_cmd_taskq);
> + taskq_destroy(sc->sc_wdog_taskq);
> printf(": unable to allocate thread\n");
> return;
> }
> sc->sc_thread->sc = sc;
> sc->sc_thread->running = 1;
>
> - /* Setup threads */
> kthread_create_deferred(ipmi_create_thread, sc);
>
> printf(": version %d.%d interface %s %sbase 0x%x/%x spacing %d",
> @@ -1717,16 +1728,12 @@ ipmi_attach(struct device *parent, struc
>
> /* Setup Watchdog timer */
> sc->sc_wdog_period = 0;
> - task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
> wdog_register(ipmi_watchdog, sc);
>
> rw_init(&sc->sc_ioctl.lock, DEVNAME(sc));
> sc->sc_ioctl.req.msgid = -1;
> c->c_sc = sc;
> c->c_ccode = -1;
> -
> - sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
> - mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
> }
>
> int
> @@ -1896,8 +1903,8 @@ ipmi_watchdog(void *arg, int period)
> int res;
>
> t = &sc->sc_wdog_tickle_task;
> - (void)task_del(systq, t);
> - res = task_add(systq, t);
> + (void)task_del(sc->sc_wdog_taskq, t);
> + res = task_add(sc->sc_wdog_taskq, t);
> KASSERT(res == 1);
> }
> return (period);
> Index: sys/dev/ipmivar.h
> ===
> RCS file: /cvs/src/sys/dev/ipmivar.h,v
> retrieving revision 1.28
> diff -u -p -r1.28 ipmivar.h
> --- sys/dev/ipmivar.h 5 Feb 2016 06:29:01 - 1.28
> +++ sys/dev/ipmivar.h 7 Jun 2019 10:00:10 -
> @@ -112,7 +112,6 @@ struct ipmi_softc {
> int sc_btseq;
> u_int8_tsc_buf[IPMI_MAX_RX + 16];
> struct taskq*sc_cmd_taskq;
> - struct mutexsc_cmd_mtx;
>
> struct ipmi_ioctl {
> struct rwlock lock;
> @@ -122,6 +121,7 @@ struct ipmi_softc {
> } sc_ioctl;
>
> int sc_wdog_period;
> + struct taskq*sc_wdog_taskq;
> struct task sc_wdog_tickle_task;
>
> struct ipmi_thread *sc_thread;
>
ipmi(4): don't block interrupts/systq long time
hi tech@,
here is another patch for another issue for ipmi(4).
ipmi_sendcmd() and ipmi_recvcmd() are always called in order/pairs as a
single task of a single-threaded taskq, remove mutex from there. this
avoids interrupts blocked long time.
task for watchdog can be long lived, use own taskq instead of systq.
--
FUKAUMI Naoki
Index: sys/dev/ipmi.c
===
RCS file: /cvs/src/sys/dev/ipmi.c,v
retrieving revision 1.102
diff -u -p -r1.102 ipmi.c
--- sys/dev/ipmi.c 15 Jun 2018 12:21:41 - 1.102
+++ sys/dev/ipmi.c 7 Jun 2019 10:00:10 -
@@ -1037,14 +1037,10 @@ ipmi_cmd(struct ipmi_cmd *c)
void
ipmi_cmd_poll(struct ipmi_cmd *c)
{
- mtx_enter(&c->c_sc->sc_cmd_mtx);
-
if ((c->c_ccode = ipmi_sendcmd(c)))
printf("%s: sendcmd fails\n", DEVNAME(c->c_sc));
else
c->c_ccode = ipmi_recvcmd(c);
-
- mtx_leave(&c->c_sc->sc_cmd_mtx);
}
void
@@ -1653,7 +1649,6 @@ ipmi_match(struct device *parent, void *
/* XXX local softc is wrong wrong wrong */
sc = malloc(sizeof(*sc), M_TEMP, M_WAITOK | M_ZERO);
- mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
strlcpy(sc->sc_dev.dv_xname, "ipmi0", sizeof(sc->sc_dev.dv_xname));
/* Map registers */
@@ -1693,15 +1688,31 @@ ipmi_attach(struct device *parent, struc
/* Map registers */
ipmi_map_regs(sc, ia);
+ /* Setup taskqs */
+ sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
+ if (sc->sc_cmd_taskq == NULL) {
+ printf(": unable to allocate cmd taskq\n");
+ return;
+ }
+ sc->sc_wdog_taskq = taskq_create("ipmiwdog", 1, IPL_SOFTCLOCK, 0);
+ if (sc->sc_cmd_taskq == NULL) {
+ taskq_destroy(sc->sc_cmd_taskq);
+ printf(": unable to allocate wdog taskq\n");
+ return;
+ }
+ task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
+
+ /* Setup thread */
sc->sc_thread = malloc(sizeof(struct ipmi_thread), M_DEVBUF, M_NOWAIT);
if (sc->sc_thread == NULL) {
+ taskq_destroy(sc->sc_cmd_taskq);
+ taskq_destroy(sc->sc_wdog_taskq);
printf(": unable to allocate thread\n");
return;
}
sc->sc_thread->sc = sc;
sc->sc_thread->running = 1;
- /* Setup threads */
kthread_create_deferred(ipmi_create_thread, sc);
printf(": version %d.%d interface %s %sbase 0x%x/%x spacing %d",
@@ -1717,16 +1728,12 @@ ipmi_attach(struct device *parent, struc
/* Setup Watchdog timer */
sc->sc_wdog_period = 0;
- task_set(&sc->sc_wdog_tickle_task, ipmi_watchdog_tickle, sc);
wdog_register(ipmi_watchdog, sc);
rw_init(&sc->sc_ioctl.lock, DEVNAME(sc));
sc->sc_ioctl.req.msgid = -1;
c->c_sc = sc;
c->c_ccode = -1;
-
- sc->sc_cmd_taskq = taskq_create("ipmicmd", 1, IPL_NONE, TASKQ_MPSAFE);
- mtx_init(&sc->sc_cmd_mtx, IPL_MPFLOOR);
}
int
@@ -1896,8 +1903,8 @@ ipmi_watchdog(void *arg, int period)
int res;
t = &sc->sc_wdog_tickle_task;
- (void)task_del(systq, t);
- res = task_add(systq, t);
+ (void)task_del(sc->sc_wdog_taskq, t);
+ res = task_add(sc->sc_wdog_taskq, t);
KASSERT(res == 1);
}
return (period);
Index: sys/dev/ipmivar.h
===
RCS file: /cvs/src/sys/dev/ipmivar.h,v
retrieving revision 1.28
diff -u -p -r1.28 ipmivar.h
--- sys/dev/ipmivar.h 5 Feb 2016 06:29:01 - 1.28
+++ sys/dev/ipmivar.h 7 Jun 2019 10:00:10 -
@@ -112,7 +112,6 @@ struct ipmi_softc {
int sc_btseq;
u_int8_tsc_buf[IPMI_MAX_RX + 16];
struct taskq*sc_cmd_taskq;
- struct mutexsc_cmd_mtx;
struct ipmi_ioctl {
struct rwlock lock;
@@ -122,6 +121,7 @@ struct ipmi_softc {
} sc_ioctl;
int sc_wdog_period;
+ struct taskq*sc_wdog_taskq;
struct task sc_wdog_tickle_task;
struct ipmi_thread *sc_thread;
