need some debugging help
Kenneth D. Merry
ken at kdm.org
Sun Aug 31 23:48:48 PDT 2003
On Sun, Aug 31, 2003 at 12:52:47 +0200, Poul-Henning Kamp wrote:
> In message <20030831070854.GA54748 at panzer.kdm.org>, "Kenneth D. Merry" writes:
>
> >Anyway, I got some debugging output, and I've attached dmesg output. Let
> >me know whether anything in there looks suspicious or points to a possible
> >problem.
>
> There's nothing which jumps out at me, and I guess the best strategy is
> hunting down the devbuf thing by changing all users of M_DEVBUF until
> something trips...
Thanks. That did the trick.
As it turns out, it was a one-line problem in the da(4) patches that was
causing the problem.
Anyway, that's fixed, and things seem to work fine. I've attached a new
version of the patches. I'll try to come up with a -stable version that'll
fix things there as well.
If anyone wants to take a look at the way I'm using mutexes here,
especially in the new taskqueue thread, I'd appreciate it.
In particular, I went through some interesting permutations in
taskqueue_kthread() to make things work right:
- I tried holding Giant when calling tsleep, but it complained that I
didn't own Giant.
- I tried not holding a mutex at all when calling tsleep, but ran into
this assert in msleep():
KASSERT(timo != 0 || mtx_owned(&Giant) || mtx != NULL,
("sleeping without a mutex"));
- I tried just holding a mutex all the time, but obviously you can't
malloc while holding a mutex (except Giant), and the sysctl code does a
number of mallocs. (The original cause of this problem -- M_WAITOK
mallocs.)
So in the end, I just acquire a mutex, drop it for taskqueue_run(),
re-acquire it and and pass it into the msleep call so that it can drop it
and re-acquire it for me. There's no other reason for it. The taskqueue
stuff already has its own mutex that isn't exposed to taskqueue_run(), and
it shouldn't be held anyway when the task's function is called.
I also put code in the sysctl functions in the cd(4) and da(4) drivers to
acquire Giant, since I'm assuming that the sysctl code needs it.
Comments are welcome.
Thanks,
Ken
--
Kenneth Merry
ken at kdm.org
-------------- next part --------------
==== //depot/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c#39 - /usr/home/ken/perforce2/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c ====
*** /tmp/tmp.319.0 Mon Sep 1 00:33:39 2003
--- /usr/home/ken/perforce2/FreeBSD-ken/src/sys/cam/scsi/scsi_cd.c Mon Sep 1 00:21:23 2003
***************
*** 62,67 ****
--- 62,68 ----
#include <sys/dvdio.h>
#include <sys/devicestat.h>
#include <sys/sysctl.h>
+ #include <sys/taskqueue.h>
#include <cam/cam.h>
#include <cam/cam_ccb.h>
***************
*** 154,159 ****
--- 155,161 ----
eventhandler_tag clonetag;
int minimum_command_size;
int outstanding_cmds;
+ struct task sysctl_task;
struct sysctl_ctx_list sysctl_ctx;
struct sysctl_oid *sysctl_tree;
STAILQ_HEAD(, cd_mode_params) mode_queue;
***************
*** 598,603 ****
--- 600,642 ----
}
}
+ static void
+ cdsysctlinit(void *context, int pending)
+ {
+ struct cam_periph *periph;
+ struct cd_softc *softc;
+ char tmpstr[80], tmpstr2[80];
+
+ periph = (struct cam_periph *)context;
+ softc = (struct cd_softc *)periph->softc;
+
+ snprintf(tmpstr, sizeof(tmpstr), "CAM CD unit %d", periph->unit_number);
+ snprintf(tmpstr2, sizeof(tmpstr2), "%d", periph->unit_number);
+
+ mtx_lock(&Giant);
+
+ sysctl_ctx_init(&softc->sysctl_ctx);
+ softc->sysctl_tree = SYSCTL_ADD_NODE(&softc->sysctl_ctx,
+ SYSCTL_STATIC_CHILDREN(_kern_cam_cd), OID_AUTO,
+ tmpstr2, CTLFLAG_RD, 0, tmpstr);
+
+ if (softc->sysctl_tree == NULL) {
+ printf("cdsysctlinit: unable to allocate sysctl tree\n");
+ return;
+ }
+
+ /*
+ * Now register the sysctl handler, so the user can the value on
+ * the fly.
+ */
+ SYSCTL_ADD_PROC(&softc->sysctl_ctx,SYSCTL_CHILDREN(softc->sysctl_tree),
+ OID_AUTO, "minimum_cmd_size", CTLTYPE_INT | CTLFLAG_RW,
+ &softc->minimum_command_size, 0, cdcmdsizesysctl, "I",
+ "Minimum CDB size");
+
+ mtx_unlock(&Giant);
+ }
+
/*
* We have a handler function for this so we can check the values when the
* user sets them, instead of every time we look at them.
***************
*** 642,648 ****
struct ccb_setasync csa;
struct ccb_pathinq cpi;
struct ccb_getdev *cgd;
! char tmpstr[80], tmpstr2[80];
caddr_t match;
cgd = (struct ccb_getdev *)arg;
--- 681,687 ----
struct ccb_setasync csa;
struct ccb_pathinq cpi;
struct ccb_getdev *cgd;
! char tmpstr[80];
caddr_t match;
cgd = (struct ccb_getdev *)arg;
***************
*** 696,712 ****
if (cpi.ccb_h.status == CAM_REQ_CMP && (cpi.hba_misc & PIM_NO_6_BYTE))
softc->quirks |= CD_Q_10_BYTE_ONLY;
! snprintf(tmpstr, sizeof(tmpstr), "CAM CD unit %d", periph->unit_number);
! snprintf(tmpstr2, sizeof(tmpstr2), "%d", periph->unit_number);
! sysctl_ctx_init(&softc->sysctl_ctx);
! softc->sysctl_tree = SYSCTL_ADD_NODE(&softc->sysctl_ctx,
! SYSCTL_STATIC_CHILDREN(_kern_cam_cd), OID_AUTO,
! tmpstr2, CTLFLAG_RD, 0, tmpstr);
! if (softc->sysctl_tree == NULL) {
! printf("cdregister: unable to allocate sysctl tree\n");
! free(softc, M_DEVBUF);
! return (CAM_REQ_CMP_ERR);
! }
/* The default is 6 byte commands, unless quirked otherwise */
if (softc->quirks & CD_Q_10_BYTE_ONLY)
--- 735,741 ----
if (cpi.ccb_h.status == CAM_REQ_CMP && (cpi.hba_misc & PIM_NO_6_BYTE))
softc->quirks |= CD_Q_10_BYTE_ONLY;
! TASK_INIT(&softc->sysctl_task, 0, cdsysctlinit, periph);
/* The default is 6 byte commands, unless quirked otherwise */
if (softc->quirks & CD_Q_10_BYTE_ONLY)
***************
*** 728,742 ****
softc->minimum_command_size = 10;
/*
- * Now register the sysctl handler, so the user can the value on
- * the fly.
- */
- SYSCTL_ADD_PROC(&softc->sysctl_ctx,SYSCTL_CHILDREN(softc->sysctl_tree),
- OID_AUTO, "minimum_cmd_size", CTLTYPE_INT | CTLFLAG_RW,
- &softc->minimum_command_size, 0, cdcmdsizesysctl, "I",
- "Minimum CDB size");
-
- /*
* We need to register the statistics structure for this device,
* but we don't have the blocksize yet for it. So, we register
* the structure and indicate that we don't have the blocksize
--- 757,762 ----
***************
*** 1847,1852 ****
--- 1867,1877 ----
xpt_announce_periph(periph, announce_buf);
if (softc->flags & CD_FLAG_CHANGER)
cdchangerschedule(softc);
+ /*
+ * Create our sysctl variables, now that we know
+ * we have successfully attached.
+ */
+ taskqueue_enqueue(taskqueue_thread,&softc->sysctl_task);
}
softc->state = CD_STATE_NORMAL;
/*
==== //depot/FreeBSD-ken/src/sys/cam/scsi/scsi_da.c#59 - /usr/home/ken/perforce2/FreeBSD-ken/src/sys/cam/scsi/scsi_da.c ====
*** /tmp/tmp.319.1 Mon Sep 1 00:33:39 2003
--- /usr/home/ken/perforce2/FreeBSD-ken/src/sys/cam/scsi/scsi_da.c Mon Sep 1 00:21:39 2003
***************
*** 48,53 ****
--- 48,54 ----
#include <sys/eventhandler.h>
#include <sys/malloc.h>
#include <sys/cons.h>
+ #include <sys/taskqueue.h>
#include <machine/md_var.h>
***************
*** 133,138 ****
--- 134,140 ----
struct disk_params params;
struct disk disk;
union ccb saved_ccb;
+ struct task sysctl_task;
struct sysctl_ctx_list sysctl_ctx;
struct sysctl_oid *sysctl_tree;
};
***************
*** 388,393 ****
--- 390,396 ----
static periph_init_t dainit;
static void daasync(void *callback_arg, u_int32_t code,
struct cam_path *path, void *arg);
+ static void dasysctlinit(void *context, int pending);
static int dacmdsizesysctl(SYSCTL_HANDLER_ARGS);
static periph_ctor_t daregister;
static periph_dtor_t dacleanup;
***************
*** 915,920 ****
--- 918,958 ----
}
}
+ static void
+ dasysctlinit(void *context, int pending)
+ {
+ struct cam_periph *periph;
+ struct da_softc *softc;
+ char tmpstr[80], tmpstr2[80];
+
+ periph = (struct cam_periph *)context;
+ softc = (struct da_softc *)periph->softc;
+
+ snprintf(tmpstr, sizeof(tmpstr), "CAM DA unit %d", periph->unit_number);
+ snprintf(tmpstr2, sizeof(tmpstr2), "%d", periph->unit_number);
+
+ mtx_lock(&Giant);
+ sysctl_ctx_init(&softc->sysctl_ctx);
+ softc->sysctl_tree = SYSCTL_ADD_NODE(&softc->sysctl_ctx,
+ SYSCTL_STATIC_CHILDREN(_kern_cam_da), OID_AUTO, tmpstr2,
+ CTLFLAG_RD, 0, tmpstr);
+ if (softc->sysctl_tree == NULL) {
+ printf("dasysctlinit: unable to allocate sysctl tree\n");
+ return;
+ }
+
+ /*
+ * Now register the sysctl handler, so the user can the value on
+ * the fly.
+ */
+ SYSCTL_ADD_PROC(&softc->sysctl_ctx,SYSCTL_CHILDREN(softc->sysctl_tree),
+ OID_AUTO, "minimum_cmd_size", CTLTYPE_INT | CTLFLAG_RW,
+ &softc->minimum_cmd_size, 0, dacmdsizesysctl, "I",
+ "Minimum CDB size");
+
+ mtx_unlock(&Giant);
+ }
+
static int
dacmdsizesysctl(SYSCTL_HANDLER_ARGS)
{
***************
*** 955,961 ****
struct ccb_setasync csa;
struct ccb_pathinq cpi;
struct ccb_getdev *cgd;
! char tmpstr[80], tmpstr2[80];
caddr_t match;
cgd = (struct ccb_getdev *)arg;
--- 993,999 ----
struct ccb_setasync csa;
struct ccb_pathinq cpi;
struct ccb_getdev *cgd;
! char tmpstr[80];
caddr_t match;
cgd = (struct ccb_getdev *)arg;
***************
*** 1008,1024 ****
if (cpi.ccb_h.status == CAM_REQ_CMP && (cpi.hba_misc & PIM_NO_6_BYTE))
softc->quirks |= DA_Q_NO_6_BYTE;
! snprintf(tmpstr, sizeof(tmpstr), "CAM DA unit %d", periph->unit_number);
! snprintf(tmpstr2, sizeof(tmpstr2), "%d", periph->unit_number);
! sysctl_ctx_init(&softc->sysctl_ctx);
! softc->sysctl_tree = SYSCTL_ADD_NODE(&softc->sysctl_ctx,
! SYSCTL_STATIC_CHILDREN(_kern_cam_da), OID_AUTO, tmpstr2,
! CTLFLAG_RD, 0, tmpstr);
! if (softc->sysctl_tree == NULL) {
! printf("daregister: unable to allocate sysctl tree\n");
! free(softc, M_DEVBUF);
! return (CAM_REQ_CMP_ERR);
! }
/*
* RBC devices don't have to support READ(6), only READ(10).
--- 1046,1052 ----
if (cpi.ccb_h.status == CAM_REQ_CMP && (cpi.hba_misc & PIM_NO_6_BYTE))
softc->quirks |= DA_Q_NO_6_BYTE;
! TASK_INIT(&softc->sysctl_task, 0, dasysctlinit, periph);
/*
* RBC devices don't have to support READ(6), only READ(10).
***************
*** 1050,1064 ****
softc->minimum_cmd_size = 16;
/*
- * Now register the sysctl handler, so the user can the value on
- * the fly.
- */
- SYSCTL_ADD_PROC(&softc->sysctl_ctx,SYSCTL_CHILDREN(softc->sysctl_tree),
- OID_AUTO, "minimum_cmd_size", CTLTYPE_INT | CTLFLAG_RW,
- &softc->minimum_cmd_size, 0, dacmdsizesysctl, "I",
- "Minimum CDB size");
-
- /*
* Block our timeout handler while we
* add this softc to the dev list.
*/
--- 1078,1083 ----
***************
*** 1539,1546 ****
}
}
free(csio->data_ptr, M_TEMP);
! if (announce_buf[0] != '\0')
xpt_announce_periph(periph, announce_buf);
softc->state = DA_STATE_NORMAL;
/*
* Since our peripheral may be invalidated by an error
--- 1558,1571 ----
}
}
free(csio->data_ptr, M_TEMP);
! if (announce_buf[0] != '\0') {
xpt_announce_periph(periph, announce_buf);
+ /*
+ * Create our sysctl variables, now that we know
+ * we have successfully attached.
+ */
+ taskqueue_enqueue(taskqueue_thread,&softc->sysctl_task);
+ }
softc->state = DA_STATE_NORMAL;
/*
* Since our peripheral may be invalidated by an error
==== //depot/FreeBSD-ken/src/sys/kern/subr_taskqueue.c#12 - /usr/home/ken/perforce2/FreeBSD-ken/src/sys/kern/subr_taskqueue.c ====
*** /tmp/tmp.319.2 Mon Sep 1 00:33:39 2003
--- /usr/home/ken/perforce2/FreeBSD-ken/src/sys/kern/subr_taskqueue.c Fri Aug 29 18:47:53 2003
***************
*** 36,41 ****
--- 36,43 ----
#include <sys/malloc.h>
#include <sys/mutex.h>
#include <sys/taskqueue.h>
+ #include <sys/kthread.h>
+ #include <sys/unistd.h>
static MALLOC_DEFINE(M_TASKQUEUE, "taskqueue", "Task Queues");
***************
*** 44,49 ****
--- 46,52 ----
static void *taskqueue_ih;
static void *taskqueue_giant_ih;
static struct mtx taskqueue_queues_mutex;
+ static struct proc *taskqueue_thread_proc;
struct taskqueue {
STAILQ_ENTRY(taskqueue) tq_link;
***************
*** 233,238 ****
--- 236,266 ----
taskqueue_run(taskqueue_swi_giant);
}
+ static void
+ taskqueue_kthread(void *arg)
+ {
+ struct mtx kthread_mutex;
+
+ bzero(&kthread_mutex, sizeof(kthread_mutex));
+
+ mtx_init(&kthread_mutex, "taskqueue kthread", NULL, MTX_DEF);
+
+ mtx_lock(&kthread_mutex);
+
+ for (;;) {
+ mtx_unlock(&kthread_mutex);
+ taskqueue_run(taskqueue_thread);
+ mtx_lock(&kthread_mutex);
+ msleep(&taskqueue_thread, &kthread_mutex, PWAIT, "tqthr", 0);
+ }
+ }
+
+ static void
+ taskqueue_thread_enqueue(void *context)
+ {
+ wakeup(&taskqueue_thread);
+ }
+
TASKQUEUE_DEFINE(swi, taskqueue_swi_enqueue, 0,
swi_add(NULL, "task queue", taskqueue_swi_run, NULL, SWI_TQ,
INTR_MPSAFE, &taskqueue_ih));
***************
*** 240,242 ****
--- 268,274 ----
TASKQUEUE_DEFINE(swi_giant, taskqueue_swi_giant_enqueue, 0,
swi_add(NULL, "Giant task queue", taskqueue_swi_giant_run,
NULL, SWI_TQ_GIANT, 0, &taskqueue_giant_ih));
+
+ TASKQUEUE_DEFINE(thread, taskqueue_thread_enqueue, 0,
+ kthread_create(taskqueue_kthread, NULL,
+ &taskqueue_thread_proc, RFNOWAIT, 0, "taskqueue"));
==== //depot/FreeBSD-ken/src/sys/sys/taskqueue.h#4 - /usr/home/ken/perforce2/FreeBSD-ken/src/sys/sys/taskqueue.h ====
*** /tmp/tmp.319.3 Mon Sep 1 00:33:39 2003
--- /usr/home/ken/perforce2/FreeBSD-ken/src/sys/sys/taskqueue.h Wed Aug 27 22:06:06 2003
***************
*** 112,116 ****
--- 112,117 ----
*/
TASKQUEUE_DECLARE(swi_giant);
TASKQUEUE_DECLARE(swi);
+ TASKQUEUE_DECLARE(thread);
#endif /* !_SYS_TASKQUEUE_H_ */
More information about the freebsd-current
mailing list