Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/jdolecek-ncqfixes]: src/sys/dev separate ata_xfer slot allocation and th...
details: https://anonhg.NetBSD.org/src/rev/7aa64a040de5
branches: jdolecek-ncqfixes
changeset: 850579:7aa64a040de5
user: jdolecek <jdolecek%NetBSD.org@localhost>
date: Sat Sep 22 09:22:59 2018 +0000
description:
separate ata_xfer slot allocation and the memory allocation, so that
there can be more queued xfers than number of supported slots by controller,
and use a pool instead of custom pre-allocation
primarily to help PR kern/52614
remove no longer needed custom wd(4) logic for flush cache
switch also wd(4) trim/suspend/setcache/wdioctlstrategy to sleep waiting
for the memory, they are all called from process context and this
avoids spurious failures
diffstat:
sys/dev/ata/TODO.ncq | 13 +-
sys/dev/ata/ata.c | 138 +++++++++++++++++++++++++-----
sys/dev/ata/ata_subr.c | 202 ++++++++++----------------------------------
sys/dev/ata/atavar.h | 11 +-
sys/dev/ata/satapmp_subr.c | 6 +-
sys/dev/ata/wd.c | 72 ++--------------
sys/dev/ata/wdvar.h | 6 +-
sys/dev/ic/ahcisata_core.c | 60 +++++++-----
sys/dev/ic/mvsata.c | 6 +-
sys/dev/ic/siisata.c | 67 ++++++++------
sys/dev/scsipi/atapi_wdc.c | 8 +-
sys/dev/usb/umass_isdata.c | 6 +-
12 files changed, 267 insertions(+), 328 deletions(-)
diffs (truncated from 1257 to 300 lines):
diff -r a7eebea6d01d -r 7aa64a040de5 sys/dev/ata/TODO.ncq
--- a/sys/dev/ata/TODO.ncq Mon Sep 17 20:54:41 2018 +0000
+++ b/sys/dev/ata/TODO.ncq Sat Sep 22 09:22:59 2018 +0000
@@ -1,16 +1,13 @@
jdolecek-ncqfixes goals:
-- make ata_xfer dynamically allocated using a pool
- - will fix: queue is allocated regardless if there are any drives, fix?
- - malloc() -> kmem_zalloc() in ata_queue_alloc() once this is done
-- remove limit of queued ata_xfers, allow any number of pending xfers;
- this should fix kern/52614 AKA wdc-attached ATAPI cd(4)
-- remove the wd(4) flush condition, just allocate a dynamic ata_xfer
-- change wd(4) dump code to use on-stack ata_xfer to not rely on pool having
- memory
+- add to wd(4) a callout to restart buf queue processing when ata_get_xfer()
+ call fails and remove ata_channel_start()
+- change wd(4) dump code to use preallocated or on-stack ata_xfer to not rely
+ on pool having memory
- re-fix QEMU ahci(4) bug workaround (no READ LOG EXT support) - now it
triggers KASSERT()
- fix ahci(4) error handling under paralles - invalid bio via WD_CHAOS_MONKEY
ends up being handled as NOERROR, triggering KASSERT() in wd(4)
+- weed out remaining KM_NOSLEEP
Bugs
----
diff -r a7eebea6d01d -r 7aa64a040de5 sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Mon Sep 17 20:54:41 2018 +0000
+++ b/sys/dev/ata/ata.c Sat Sep 22 09:22:59 2018 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ata.c,v 1.141.6.5 2018/09/17 20:54:41 jdolecek Exp $ */
+/* $NetBSD: ata.c,v 1.141.6.6 2018/09/22 09:22:59 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.141.6.5 2018/09/17 20:54:41 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.141.6.6 2018/09/22 09:22:59 jdolecek Exp $");
#include "opt_ata.h"
@@ -84,6 +84,7 @@
#endif
static ONCE_DECL(ata_init_ctrl);
+static struct pool ata_xfer_pool;
/*
* A queue of atabus instances, used to ensure the same bus probe order
@@ -142,6 +143,8 @@
atabus_init(void)
{
+ pool_init(&ata_xfer_pool, sizeof(struct ata_xfer), 0, 0, 0,
+ "ataspl", NULL, IPL_BIO);
TAILQ_INIT(&atabus_initq_head);
mutex_init(&atabus_qlock, MUTEX_DEFAULT, IPL_NONE);
cv_init(&atabus_qcv, "atainitq");
@@ -779,7 +782,7 @@
ATADEBUG_PRINT(("%s\n", __func__), DEBUG_FUNCS);
- xfer = ata_get_xfer(chp);
+ xfer = ata_get_xfer(chp, false);
if (xfer == NULL) {
ATADEBUG_PRINT(("%s: no xfer\n", __func__),
DEBUG_FUNCS|DEBUG_PROBE);
@@ -884,7 +887,7 @@
ATADEBUG_PRINT(("ata_set_mode=0x%x\n", mode), DEBUG_FUNCS);
- xfer = ata_get_xfer(chp);
+ xfer = ata_get_xfer(chp, false);
if (xfer == NULL) {
ATADEBUG_PRINT(("%s: no xfer\n", __func__),
DEBUG_FUNCS|DEBUG_PROBE);
@@ -919,7 +922,7 @@
ata_read_log_ext_ncq(struct ata_drive_datas *drvp, uint8_t flags,
uint8_t *slot, uint8_t *status, uint8_t *err)
{
- struct ata_xfer *xfer;
+ struct ata_xfer *xfer = &drvp->recovery_xfer;
int rv;
struct ata_channel *chp = drvp->chnl_softc;
struct atac_softc *atac = chp->ch_atac;
@@ -932,7 +935,7 @@
(drvp->drive_flags & ATA_DRIVE_NCQ) == 0)
return EOPNOTSUPP;
- xfer = ata_get_xfer_ext(chp, C_RECOVERY, 0);
+ memset(xfer, 0, sizeof(*xfer));
tb = drvp->recovery_blk;
memset(tb, 0, sizeof(drvp->recovery_blk));
@@ -994,7 +997,6 @@
rv = 0;
out:
- ata_free_xfer(chp, xfer);
return rv;
}
@@ -1128,17 +1130,22 @@
ata_channel_lock(chp);
again:
- KASSERT(chq->queue_active <= chq->queue_openings);
- if (chq->queue_active == chq->queue_openings) {
- ATADEBUG_PRINT(("%s(chp=%p): channel %d completely busy\n",
+ /* is there a xfer ? */
+ if ((xfer = SIMPLEQ_FIRST(&chp->ch_queue->queue_xfer)) == NULL) {
+ ATADEBUG_PRINT(("%s(chp=%p): channel %d queue_xfer is empty\n",
__func__, chp, chp->ch_channel), DEBUG_XFERS);
goto out;
}
- /* is there a xfer ? */
- if ((xfer = SIMPLEQ_FIRST(&chp->ch_queue->queue_xfer)) == NULL) {
- ATADEBUG_PRINT(("%s(chp=%p): channel %d queue_xfer is empty\n",
- __func__, chp, chp->ch_channel), DEBUG_XFERS);
+ /*
+ * if someone is waiting for the command to be active, wake it up
+ * and let it process the command
+ */
+ if (__predict_false(xfer->c_flags & C_WAITACT)) {
+ ATADEBUG_PRINT(("atastart: xfer %p channel %d drive %d "
+ "wait active\n", xfer, chp->ch_channel, xfer->c_drive),
+ DEBUG_XFERS);
+ cv_broadcast(&chp->ch_queue->c_active);
goto out;
}
@@ -1179,20 +1186,37 @@
struct ata_drive_datas * const drvp = &chp->ch_drive[xfer->c_drive];
/*
- * if someone is waiting for the command to be active, wake it up
- * and let it process the command
+ * Are we on limit of active xfers ?
+ * For recovery, we must leave one slot available at all times.
*/
- if (xfer->c_flags & C_WAITACT) {
- ATADEBUG_PRINT(("atastart: xfer %p channel %d drive %d "
- "wait active\n", xfer, chp->ch_channel, xfer->c_drive),
- DEBUG_XFERS);
- cv_broadcast(&chp->ch_queue->c_active);
+ KASSERT(chq->queue_active <= chq->queue_openings);
+ const uint8_t chq_openings = (!recovery && chq->queue_openings > 1)
+ ? (chq->queue_openings - 1) : chq->queue_openings;
+ const uint8_t drv_openings = ISSET(xfer->c_flags, C_NCQ)
+ ? drvp->drv_openings : ATA_MAX_OPENINGS;
+ if (chq->queue_active >= MIN(chq_openings, drv_openings)) {
+ if (recovery) {
+ panic("%s: channel %d busy, recovery not possible",
+ __func__, chp->ch_channel);
+ }
+
+ ATADEBUG_PRINT(("%s(chp=%p): channel %d completely busy\n",
+ __func__, chp, chp->ch_channel), DEBUG_XFERS);
goto out;
}
- if (atac->atac_claim_hw)
- if (!atac->atac_claim_hw(chp, 0))
+ /* Slot allocation can fail if drv_openings < ch_openings */
+ if (!ata_queue_alloc_slot(chp, &xfer->c_slot, drv_openings))
+ goto out;
+
+ if (__predict_false(atac->atac_claim_hw)) {
+ if (!atac->atac_claim_hw(chp, 0)) {
+ ata_queue_free_slot(chp, xfer->c_slot);
goto out;
+ }
+ }
+
+ /* Now committed to start the xfer */
ATADEBUG_PRINT(("%s(chp=%p): xfer %p channel %d drive %d\n",
__func__, chp, xfer, chp->ch_channel, xfer->c_drive), DEBUG_XFERS);
@@ -1273,7 +1297,13 @@
KASSERT(mutex_owned(&chp->ch_lock));
- KASSERT(chq->queue_active < chq->queue_openings);
+ /*
+ * When openings is just 1, can't reserve anything for
+ * recovery. KASSERT() here is to catch code which naively
+ * relies on C_RECOVERY to work under this condition.
+ */
+ KASSERT((xfer->c_flags & C_RECOVERY) == 0 || chq->queue_openings > 1);
+
KASSERT((chq->active_xfers_used & __BIT(xfer->c_slot)) == 0);
if ((xfer->c_flags & C_RECOVERY) == 0)
@@ -1290,6 +1320,64 @@
chq->queue_active++;
}
+/*
+ * Does it's own locking, does not require splbio().
+ * flags - whether to block waiting for free xfer
+ */
+struct ata_xfer *
+ata_get_xfer(struct ata_channel *chp, bool waitok)
+{
+ struct ata_xfer *xfer;
+
+ xfer = pool_get(&ata_xfer_pool, waitok ? PR_WAITOK : PR_NOWAIT);
+ KASSERT(!waitok || xfer != NULL);
+
+ if (xfer != NULL) {
+ /* zero everything */
+ memset(xfer, 0, sizeof(*xfer));
+ }
+
+ return xfer;
+}
+
+/*
+ * ata_deactivate_xfer() must be always called prior to ata_free_xfer()
+ */
+void
+ata_free_xfer(struct ata_channel *chp, struct ata_xfer *xfer)
+{
+ struct ata_queue *chq = chp->ch_queue;
+
+ ata_channel_lock(chp);
+
+ if (xfer->c_flags & (C_WAITACT|C_WAITTIMO)) {
+ /* Someone is waiting for this xfer, so we can't free now */
+ xfer->c_flags |= C_FREE;
+ cv_broadcast(&chq->c_active);
+ ata_channel_unlock(chp);
+ return;
+ }
+
+ /* XXX move PIOBM and free_gw to deactivate? */
+#if NATA_PIOBM /* XXX wdc dependent code */
+ if (xfer->c_flags & C_PIOBM) {
+ struct wdc_softc *wdc = CHAN_TO_WDC(chp);
+
+ /* finish the busmastering PIO */
+ (*wdc->piobm_done)(wdc->dma_arg,
+ chp->ch_channel, xfer->c_drive);
+ chp->ch_flags &= ~(ATACH_DMA_WAIT | ATACH_PIOBM_WAIT | ATACH_IRQ_WAIT);
+ }
+#endif
+
+ if (__predict_false(chp->ch_atac->atac_free_hw))
+ chp->ch_atac->atac_free_hw(chp);
+
+ ata_channel_unlock(chp);
+
+ pool_put(&ata_xfer_pool, xfer);
+}
+
void
ata_deactivate_xfer(struct ata_channel *chp, struct ata_xfer *xfer)
{
@@ -1311,6 +1399,8 @@
chq->active_xfers_used &= ~__BIT(xfer->c_slot);
chq->queue_active--;
+ ata_queue_free_slot(chp, xfer->c_slot);
+
if (xfer->c_flags & C_WAIT)
cv_broadcast(&chq->c_cmd_finish);
diff -r a7eebea6d01d -r 7aa64a040de5 sys/dev/ata/ata_subr.c
--- a/sys/dev/ata/ata_subr.c Mon Sep 17 20:54:41 2018 +0000
+++ b/sys/dev/ata/ata_subr.c Sat Sep 22 09:22:59 2018 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ata_subr.c,v 1.6.2.4 2018/09/17 20:54:41 jdolecek Exp $ */
+/* $NetBSD: ata_subr.c,v 1.6.2.5 2018/09/22 09:22:59 jdolecek Exp $ */
/*
* Copyright (c) 1998, 2001 Manuel Bouyer. All rights reserved.
@@ -25,14 +25,13 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata_subr.c,v 1.6.2.4 2018/09/17 20:54:41 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_subr.c,v 1.6.2.5 2018/09/22 09:22:59 jdolecek Exp $");
#include "opt_ata.h"
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
-#include <sys/malloc.h>
#include <sys/device.h>
#include <sys/conf.h>
#include <sys/fcntl.h>
@@ -167,37 +166,30 @@
if (openings > ATA_MAX_OPENINGS)
openings = ATA_MAX_OPENINGS;
- /* XXX convert to kmem_zalloc() once ata_xfer is moved to pool */
Home |
Main Index |
Thread Index |
Old Index