On Sun, Aug 31, 2003 at 12:52:47 +0200, Poul-Henning Kamp wrote:
> In message <[EMAIL PROTECTED]>, "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
[EMAIL PROTECTED]
==== //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_ */
_______________________________________________
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[EMAIL PROTECTED]"

Reply via email to