[src/jdolecek-ncq]: src/sys/dev/ata hold channel lock for ata_exec_xfer() and...

[jdolecek <jdolecek%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/2202ffc67252
branches:  jdolecek-ncq
changeset: 352686:2202ffc67252
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Wed Jun 21 22:40:43 2017 +0000

description:
hold channel lock for ata_exec_xfer() and most of atastart(), convert
C_WAITACT handling to use condvar

add comment for the code block with C_FREE and ata_free_xfer() explaining
why it's not abstraction layer violation

diffstat:

 sys/dev/ata/TODO.ncq |   3 --
 sys/dev/ata/ata.c    |  56 +++++++++++++++++++++++++++++++++++++--------------
 sys/dev/ata/atavar.h |   4 +-
 3 files changed, 42 insertions(+), 21 deletions(-)

diffs (220 lines):

diff -r 430141f15eed -r 2202ffc67252 sys/dev/ata/TODO.ncq
--- a/sys/dev/ata/TODO.ncq      Wed Jun 21 22:34:46 2017 +0000
+++ b/sys/dev/ata/TODO.ncq      Wed Jun 21 22:40:43 2017 +0000
@@ -12,9 +12,6 @@
 test wd* at umass?, confirm the ata_channel kludge works
 + add detach code (channel detach, free queue)
 
-is ata_exec_xfer() + POLL safe wrt. more outstanding I/Os? why is it waiting
-until xfer is head of queue? also layer violation with the ata_xfer_free() call
-
 test device error handling (currently appears to not work well, at least in NCQ case)
 
 do proper NCQ error recovery (currently not even really attempted)
diff -r 430141f15eed -r 2202ffc67252 sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Wed Jun 21 22:34:46 2017 +0000
+++ b/sys/dev/ata/ata.c Wed Jun 21 22:40:43 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.132.8.14 2017/06/21 22:34:46 jdolecek Exp $  */
+/*     $NetBSD: ata.c,v 1.132.8.15 2017/06/21 22:40:43 jdolecek Exp $  */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.14 2017/06/21 22:34:46 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.15 2017/06/21 22:40:43 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -130,6 +130,7 @@
 static void atabusconfig_thread(void *);
 
 static void ata_channel_idle(struct ata_channel *);
+static void ata_activate_xfer_locked(struct ata_channel *, struct ata_xfer *);
 
 /*
  * atabus_init:
@@ -246,6 +247,7 @@
        if (zero)
                memset(xfer, 0, sizeof(*xfer));
 
+       cv_init(&xfer->c_active, "ataact");
        callout_init(&xfer->c_timo_callout, 0);         /* XXX MPSAFE */
 }
 
@@ -254,6 +256,7 @@
 {
        callout_halt(&xfer->c_timo_callout, NULL);      /* XXX MPSAFE */
        callout_destroy(&xfer->c_timo_callout);
+       cv_destroy(&xfer->c_active);
 }
 
 struct ata_queue *
@@ -1058,6 +1061,8 @@
        /* complete xfer setup */
        xfer->c_chp = chp;
 
+       mutex_enter(&chp->ch_lock);
+
        /* insert at the end of command list */
        TAILQ_INSERT_TAIL(&chp->ch_queue->queue_xfer, xfer, c_xferchain);
        ATADEBUG_PRINT(("atastart from ata_exec_xfer, flags 0x%x\n",
@@ -1070,14 +1075,22 @@
                while (chp->ch_queue->queue_active > 0 ||
                    TAILQ_FIRST(&chp->ch_queue->queue_xfer) != xfer) {
                        xfer->c_flags |= C_WAITACT;
-                       tsleep(xfer, PRIBIO, "ataact", 0);
+                       cv_wait(&xfer->c_active, &chp->ch_lock);
                        xfer->c_flags &= ~C_WAITACT;
+
+                       /*
+                        * Free xfer now if it there was attempt to free it
+                        * while we were waiting.
+                        */
                        if (xfer->c_flags & C_FREE) {
                                ata_free_xfer(chp, xfer);
                                return;
                        }
                }
        }
+
+       mutex_exit(&chp->ch_lock);
+
        atastart(chp);
 }
 
@@ -1108,8 +1121,10 @@
        splx(spl1);
 #endif /* ATA_DEBUG */
 
+       mutex_enter(&chp->ch_lock);
+
        if (chq->queue_active == chq->queue_openings) {
-               return; /* channel completely busy */
+               goto out; /* channel completely busy */
        }
 
        /* is the queue frozen? */
@@ -1118,12 +1133,12 @@
                        chq->queue_flags &= ~QF_IDLE_WAIT;
                        wakeup(&chq->queue_flags);
                }
-               return; /* queue frozen */
+               goto out; /* queue frozen */
        }
 
        /* is there a xfer ? */
        if ((xfer = TAILQ_FIRST(&chp->ch_queue->queue_xfer)) == NULL)
-               return;
+               goto out;
 
        /*
         * Can only take NCQ command if there are no current active
@@ -1132,7 +1147,7 @@
         */
        axfer = TAILQ_FIRST(&chp->ch_queue->active_xfers);
        if (axfer && (axfer->c_flags & C_NCQ) == 0)
-               return;
+               goto out;
 
        /* adjust chp, in case we have a shared queue */
        chp = xfer->c_chp;
@@ -1148,9 +1163,10 @@
                ATADEBUG_PRINT(("atastart: xfer %p channel %d drive %d "
                    "wait active\n", xfer, chp->ch_channel, xfer->c_drive),
                    DEBUG_XFERS);
-               wakeup(xfer);
-               return;
+               cv_signal(&xfer->c_active);
+               goto out;
        }
+
 #ifdef DIAGNOSTIC
        if ((chp->ch_flags & ATACH_IRQ_WAIT) != 0
            && chp->ch_queue->queue_openings == 1)
@@ -1163,12 +1179,22 @@
                drvp->state = 0;
        }
 
-       ata_activate_xfer(chp, xfer);
+       ata_activate_xfer_locked(chp, xfer);
 
        if (atac->atac_cap & ATAC_CAP_NOIRQ)
                KASSERT(xfer->c_flags & C_POLL);
 
+       mutex_exit(&chp->ch_lock);
+
+       /*
+        * XXX MPSAFE can't keep the lock, xfer->c_start() might call the done
+        * routine for polled commands.
+        */
        xfer->c_start(chp, xfer);
+       return;
+
+out:
+       mutex_exit(&chp->ch_lock);
 }
 
 /*
@@ -1233,7 +1259,7 @@
        if (xfer->c_flags & C_WAITACT) {
                /* Someone is waiting for this xfer, so we can't free now */
                xfer->c_flags |= C_FREE;
-               wakeup(xfer);
+               cv_signal(&xfer->c_active);
                goto out;
        }
 
@@ -1261,12 +1287,12 @@
        mutex_exit(&chp->ch_lock);
 }
 
-void
-ata_activate_xfer(struct ata_channel *chp, struct ata_xfer *xfer)
+static void
+ata_activate_xfer_locked(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        struct ata_queue * const chq = chp->ch_queue;
 
-       mutex_enter(&chp->ch_lock);
+       KASSERT(mutex_owned(&chp->ch_lock));
 
        KASSERT(chq->queue_active < chq->queue_openings);
        KASSERT((chq->active_xfers_used & __BIT(xfer->c_slot)) == 0);
@@ -1275,8 +1301,6 @@
        TAILQ_INSERT_TAIL(&chq->active_xfers, xfer, c_activechain);
        chq->active_xfers_used |= __BIT(xfer->c_slot);
        chq->queue_active++;
-
-       mutex_exit(&chp->ch_lock);
 }
 
 void
diff -r 430141f15eed -r 2202ffc67252 sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Wed Jun 21 22:34:46 2017 +0000
+++ b/sys/dev/ata/atavar.h      Wed Jun 21 22:40:43 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.92.8.13 2017/06/21 19:38:43 jdolecek Exp $        */
+/*     $NetBSD: atavar.h,v 1.92.8.14 2017/06/21 22:40:43 jdolecek Exp $        */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -134,6 +134,7 @@
  */
 struct ata_xfer {
        struct callout c_timo_callout;  /* timeout callout handle */
+       kcondvar_t c_active;            /* somebody actively waiting for xfer */
 
 #define c_startzero    c_chp
        /* Channel and drive that are to process the request. */
@@ -490,7 +491,6 @@
 #define ata_get_xfer(chp) ata_get_xfer_ext((chp), true, 0);
 void   ata_free_xfer(struct ata_channel *, struct ata_xfer *);
 
-void   ata_activate_xfer(struct ata_channel *, struct ata_xfer *);
 void   ata_deactivate_xfer(struct ata_channel *, struct ata_xfer *);
 
 void   ata_exec_xfer(struct ata_channel *, struct ata_xfer *);