From owner-freebsd-current@FreeBSD.ORG Sun Aug 31 23:48:48 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6069816A4BF for ; Sun, 31 Aug 2003 23:48:48 -0700 (PDT) Received: from panzer.kdm.org (panzer.kdm.org [216.160.178.169]) by mx1.FreeBSD.org (Postfix) with ESMTP id E8EB243FDD for ; Sun, 31 Aug 2003 23:48:46 -0700 (PDT) (envelope-from ken@panzer.kdm.org) Received: from panzer.kdm.org (localhost [127.0.0.1]) by panzer.kdm.org (8.12.9/8.12.5) with ESMTP id h816mfYU061350; Mon, 1 Sep 2003 00:48:42 -0600 (MDT) (envelope-from ken@panzer.kdm.org) Received: (from ken@localhost) by panzer.kdm.org (8.12.9/8.12.5/Submit) id h816mf4d061349; Mon, 1 Sep 2003 00:48:41 -0600 (MDT) (envelope-from ken) Date: Mon, 1 Sep 2003 00:48:41 -0600 From: "Kenneth D. Merry" To: Poul-Henning Kamp Message-ID: <20030901064840.GA61253@panzer.kdm.org> References: <20030831070854.GA54748@panzer.kdm.org> <16343.1062327167@critter.freebsd.dk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="jRHKVT23PllUwdXP" Content-Disposition: inline In-Reply-To: <16343.1062327167@critter.freebsd.dk> User-Agent: Mutt/1.4.1i cc: current@FreeBSD.org Subject: Re: need some debugging help X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Sep 2003 06:48:48 -0000 --jRHKVT23PllUwdXP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Aug 31, 2003 at 12:52:47 +0200, Poul-Henning Kamp wrote: > In message <20030831070854.GA54748@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@kdm.org --jRHKVT23PllUwdXP Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="cam.sysctl.20030831" ==== //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 #include #include + #include #include #include *************** *** 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 #include #include + #include #include *************** *** 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 #include #include + #include + #include 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_ */ --jRHKVT23PllUwdXP--