IDE TCQ #4

PATCH In review Jens Axboe 7 participants 27 messages Apr 15, 2002 – Apr 30, 2002 Block layer

lore

Share & tools

lore

Jens Axboe

author

2002-04-15 12:56 UTC #0 lore raw

PATCH 1/1 IDE TCQ #4

Hi,

Updated the patch to 2.5.8 (painful). Changes:

- Expand configure help options a bit
- Fix xconfig bug (thanks to
- Decrease queue depth if a command takes too long to complete
- Test master/slave stuff. It works, but one device can heavily starve another. This is the simple approach right now, means that one device will wait until the other is completely idle before starting any commands This is not necessary since we can have queued commands on both devices at the same time. TODO.
- Add proc output for oldest command, just for testing.
- pci_dev compile fixes.
- Make sure ide-disk doesn't BUG if TCQ is not used, basically this was fixed by off-loading the using_tcq setting to ide-tcq.
- Remove warning about 'queued feature set not supported'
- Abstract ide_tcq_wait_dataphase() into a function

Small enough to include here, since 2.5.8 already had some of the tcq stuff.

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c +7 -5

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
@@ -899,10 +899,10 @@
 	spin_lock_prefetch(q->queue_lock);
 
 	generic_unplug_device(q);
-	add_wait_queue(&rl->wait, &wait);
+	add_wait_queue_exclusive(&rl->wait, &wait);
 	do {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (rl->count < batch_requests)
+		if (!rl->count)
 			schedule();
 		spin_lock_irq(q->queue_lock);
 		rq = get_request(q, rw);

@@ -1720,9 +1720,11 @@
 	 * Free request slots per queue.
 	 * (Half for reads, half for writes)
 	 */
-	queue_nr_requests = 64;
-	if (total_ram > MB(32))
-		queue_nr_requests = 256;
+	queue_nr_requests = (total_ram >> 8) & ~15;	/* One per quarter-megabyte */
+	if (queue_nr_requests < 32)
+		queue_nr_requests = 32;
+	if (queue_nr_requests > 512)
+		queue_nr_requests = 512;
 
 	/*
 	 * Batch frees according to queue length

drivers/ide/Config.help renamed from opt/kernel/linux-2.5.8/drivers/ide/Config.help +23 -3

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/Config.help linux/drivers/ide/Config.help
@@ -753,9 +753,17 @@
 
   If you have such a drive, say Y here.
 
-CONFIG_BLK_DEV_IDE_TCQ_DEFAULT
-  Enabled tagged command queueing unconditionally on drives that report
-  support for it.
+CONFIG_BLK_DEV_IDE_TCQ_FULL
+  When a command completes from the drive, the SERVICE bit is checked to
+  see if other queued commands are ready to be started. Doing this
+  immediately after a command completes has a tendency to 'starve' the
+  device hardware queue, since we risk emptying the queue completely
+  before starting any new commands. This shows up during stressing the
+  drive as a /\/\/\/\ queue size balance, where we could instead try and
+  maintain a minimum queue size and get a /---------\ graph instead.
+
+  Saying Y here will attempt to always keep the queue full when possible
+  at the cost of possibly increasing command turn-around latency.
 
   Generally say Y here.

@@ -766,6 +774,18 @@
   You probably just want the default of 32 here. If you enter an invalid
   number, the default value will be used.
 
+CONFIG_BLK_DEV_IDE_TCQ_DEFAULT
+  Enabled tagged command queueing unconditionally on drives that report
+  support for it. Regardless of the chosen value here, tagging can be
+  controlled at run time:
+
+  echo "using_tcq:32" > /proc/ide/hdX/settings
+
+  where any value between 1-32 selects chosen queue depth and enables
+  TCQ, and 0 disables it.
+
+  Generally say Y here.
+
 CONFIG_BLK_DEV_IT8172
   Say Y here to support the on-board IDE controller on the Integrated
   Technology Express, Inc. ITE8172 SBC.  Vendor page at

drivers/ide/Config.in renamed from opt/kernel/linux-2.5.8/drivers/ide/Config.in +5 -4

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/Config.in linux/drivers/ide/Config.in
@@ -48,10 +48,11 @@
          dep_bool '    Enable DMA only for disks ' CONFIG_IDEDMA_ONLYDISK $CONFIG_IDEDMA_PCI_AUTO
 	 define_bool CONFIG_BLK_DEV_IDEDMA $CONFIG_BLK_DEV_IDEDMA_PCI
 	 dep_bool '    ATA tagged command queueing' CONFIG_BLK_DEV_IDE_TCQ $CONFIG_BLK_DEV_IDEDMA_PCI
-	   dep_bool '      TCQ on by default' CONFIG_BLK_DEV_IDE_TCQ_DEFAULT $CONFIG_BLK_DEV_IDE_TCQ
-	   if [ $CONFIG_BLK_DEV_IDE_TCQ_DEFAULT != "n" ]; then
-		int '      Default queue depth' CONFIG_BLK_DEV_IDE_TCQ_DEPTH 32
-	   fi
+	 dep_bool '      Attempt to keep queue full' CONFIG_BLK_DEV_IDE_TCQ_FULL $CONFIG_BLK_DEV_IDE_TCQ
+	 dep_bool '      TCQ on by default' CONFIG_BLK_DEV_IDE_TCQ_DEFAULT $CONFIG_BLK_DEV_IDE_TCQ
+	 if [ "$CONFIG_BLK_DEV_IDE_TCQ" != "n" ]; then
+	    int '      Default queue depth' CONFIG_BLK_DEV_IDE_TCQ_DEPTH 8
+	 fi
 	 dep_bool '    ATA Work(s) In Progress (EXPERIMENTAL)' CONFIG_IDEDMA_PCI_WIP $CONFIG_BLK_DEV_IDEDMA_PCI $CONFIG_EXPERIMENTAL
 	 dep_bool '    Good-Bad DMA Model-Firmware (WIP)' CONFIG_IDEDMA_NEW_DRIVE_LISTINGS $CONFIG_IDEDMA_PCI_WIP
 	 dep_bool '    AEC62XX chipset support' CONFIG_BLK_DEV_AEC62XX $CONFIG_BLK_DEV_IDEDMA_PCI

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +238 ~~-145~~

490 lines not on this page · raw message

drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +36 ~~-35~~

109 lines not on this page · raw message

drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +4 ~~-15~~

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
@@ -610,9 +610,6 @@
 		case ide_dma_check:
 			return config_drive_for_dma (drive);
 		case ide_dma_begin:
-#ifdef DEBUG
-			printk("ide_dma_begin: from %p\n", __builtin_return_address(0));
-#endif
 			if (test_and_set_bit(IDE_DMA, &HWGROUP(drive)->flags))
 				BUG();
 			/* Note that this is done *after* the cmd has

@@ -629,19 +626,18 @@
 		case ide_dma_write_queued:
 		case ide_dma_queued_start:
 			return ide_tcq_dmaproc(func, drive);
-#endif
+#endif /* CONFIG_BLK_DEV_IDE_TCQ */
 
 		case ide_dma_read:
 			reading = 1 << 3;
 		case ide_dma_write:
-			ar = HWGROUP(drive)->rq->special;
+			ar = IDE_CUR_AR(drive);
 
 			if (ide_start_dma(hwif, drive, func))
 				return 1;
 
 			if (drive->type != ATA_DISK)
 				return 0;
-
 			BUG_ON(HWGROUP(drive)->handler);
 			ide_set_handler(drive, &ide_dma_intr, WAIT_CMD, dma_timer_expiry);	/* issue cmd to drive */
 			if ((ar->ar_rq->flags & REQ_DRIVE_TASKFILE) &&

@@ -655,20 +651,13 @@
 			}
 			return hwif->dmaproc(ide_dma_begin, drive);
 		case ide_dma_end: /* returns 1 on error, 0 otherwise */
-#ifdef DEBUG
-			printk("ide_dma_end: from %p\n", __builtin_return_address(0));
-#endif
-			if (!test_and_clear_bit(IDE_DMA, &HWGROUP(drive)->flags)) {
-				printk("ide_dma_end: dma not going? %p\n", __builtin_return_address(0));
-				return 1;
-			}
+			if (!test_and_clear_bit(IDE_DMA, &HWGROUP(drive)->flags))
+				BUG();
 			drive->waiting_for_dma = 0;
 			outb(inb(dma_base)&~1, dma_base);	/* stop DMA */
 			dma_stat = inb(dma_base+2);	/* get DMA status */
 			outb(dma_stat|6, dma_base+2);	/* clear the INTR & ERROR bits */
 			ide_destroy_dmatable(drive);	/* purge DMA mappings */
-			if (drive->tcq)
-				IDE_SET_CUR_TAG(drive, -1);
 			return (dma_stat & 7) != 4 ? (0x10 | dma_stat) : 0;	/* verify good DMA status */
 		case ide_dma_test_irq: /* returns 1 if dma irq issued, 0 otherwise */
 			dma_stat = inb(dma_base+2);

drivers/ide/ide-probe.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-probe.c +5 ~~-10~~

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-probe.c linux/drivers/ide/ide-probe.c
@@ -192,19 +192,16 @@
 	/*
 	 * it's an ata drive, build command list
 	 */
-#ifndef CONFIG_BLK_DEV_IDE_TCQ
 	drive->queue_depth = 1;
+#ifdef CONFIG_BLK_DEV_IDE_TCQ_DEPTH
+	drive->queue_depth = CONFIG_BLK_DEV_IDE_TCQ_DEPTH;
 #else
-# ifndef CONFIG_BLK_DEV_IDE_TCQ_DEPTH
-#  define CONFIG_BLK_DEV_IDE_TCQ_DEPTH 1
-# endif
 	drive->queue_depth = drive->id->queue_depth + 1;
-	if (drive->queue_depth > CONFIG_BLK_DEV_IDE_TCQ_DEPTH)
-		drive->queue_depth = CONFIG_BLK_DEV_IDE_TCQ_DEPTH;
+#endif
 	if (drive->queue_depth < 1 || drive->queue_depth > IDE_MAX_TAG)
 		drive->queue_depth = IDE_MAX_TAG;
-#endif
-	if (ide_build_commandlist(drive))
+
+	if (ide_init_commandlist(drive))
 		goto err_misc;
 
 	return;

@@ -482,11 +479,9 @@
 	sprintf(hwif->dev.bus_id, "%04x", hwif->io_ports[IDE_DATA_OFFSET]);
 	sprintf(hwif->dev.name, "ide");
 	hwif->dev.driver_data = hwif;
-#ifdef CONFIG_BLK_DEV_IDEPCI
 	if (hwif->pci_dev)
 		hwif->dev.parent = &hwif->pci_dev->dev;
 	else
-#endif
 		hwif->dev.parent = NULL; /* Would like to do = &device_legacy */
 	device_register(&hwif->dev);

drivers/ide/ide-tcq.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-tcq.c +112 ~~-65~~

359 lines not on this page · raw message

include/linux/ide.h renamed from opt/kernel/linux-2.5.8/include/linux/ide.h +44 ~~-14~~

136 lines not on this page · raw message

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-04-15 13:26 UTC #1 lore raw

Quoted 3 lines · Jens Axboe

Hi,
Updated the patch to 2.5.8 (painful). Changes:

S*it - I did just offer you assistance... well you where faster. Anyway thank you for going into this trouble.

Please allow me some notes anyway:

drivers/block/ll_rw_blk.c

Quoted 4 lines

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c +0 -0

I recognize the following makes sense but since it's affecting every single type of device out there it should go separetely to Linus I think...

drivers/block/ll_rw_blk.c

Quoted 2 lines

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c +2 -0
+  drive as a /\/\/\/\ queue size balance, where we could instead try and
+  maintain a minimum queue size and get a /---------\ graph instead.

Nice drawings :-). But the help text shouldn't be
that encouraging right now IMHO... (At least
we should mark the options as experimental. (OK I will do that...)

Quoted 6 lines

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -0
@@ -381,8 +381,23 @@
 		add_blkdev_randomness(major(rq->rq_dev));
 

drivers/ide/ide.c line 392

Quoted 10 lines

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +9 -0

 		spin_lock_irqsave(&ide_lock, flags);
+
+		if ((jiffies - ar->ar_time > ATA_AR_MAX_TURNAROUND) && drive->queue_depth > 1) {
+			printk("%s: exceeded max command turn-around time (%d seconds)\n", drive->name, ATA_AR_MAX_TURNAROUND / HZ);
+			drive->queue_depth >>= 1;
+		}
+
+		if (jiffies - ar->ar_time > drive->tcq->oldest_command)
+			drive->tcq->oldest_command = jiffies - ar->ar_time;
+

time_before() and timer_after() should be used here.

drivers/ide/ide.c line 1115

Quoted 1 line

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -0
 	while ((read_timer() - hwif->last_time) < DISK_RECOVERY_TIME);

Same here.

I don't like that the following get's even more complicated. But of course I will just have too look closer and understand it actually.

drivers/ide/ide.c line 1314

Quoted 2 lines

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +2 -0
+		if (0 < (signed long)(jiffies + WAIT_MIN_SLEEP - sleep))
+			sleep = jiffies + WAIT_MIN_SLEEP;

The susual... timer_after() time_before()...

drivers/ide/ide.c line 1690

Quoted 3 lines

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -2
-#ifdef CONFIG_BLK_DEV_IDEPCI
 		if (hwif->pci_dev && !hwif->pci_dev->vendor)
-#endif

Here I'm still not convinced whatever this should be implemented for all architectures...

drivers/ide/ide.c line 2824

Quoted 2 lines

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +1 -1
-		memset(ar, 0, sizeof(*ar));
+		memset(ar, 0, sizeof(ar));

Please look closer - I'm quite convinced that sizeof(ar) == sizeof(void *) which gives not the desired effect. I have fixed this during the last merge...

drivers/ide/ide-disk.c line 110

Quoted 13 lines

static int ide_check_media_change (kdev_t i_rdev)
diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +1 -4
@@ -107,10 +107,7 @@
 	 * 48-bit commands are pretty sanely laid out
 	 */
 	if (lba48bit) {
-		if (cmd == READ)
-			command = WIN_READ_EXT;
-		else
-			command = WIN_WRITE_EXT;
+		command = cmd == READ ? WIN_READ_EXT : WIN_WRITE_EXT;

Checking with GCC will reveal that the former version doesn't generate worser code...

drivers/ide/ide-disk.c line 898

Quoted 9 lines

drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +2 -2

 	return command;

@@ -895,24 +894,24 @@
 
 		__set_bit(i, &tag_mask);
 		len += sprintf(out+len, "%d, ", i);
-		if (ar->ar_time > max_jif)
-			max_jif = ar->ar_time;
+		if (cur_jif - ar->ar_time > max_jif)
+			max_jif = cur_jif - ar->ar_time;

I disgust timer calculartions...- will have to look at a way how to nap in a similar way eth drivers do.

drivers/ide/ide-dma.c line 634

Quoted 11 lines

diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +2 -1
 
+#endif /* CONFIG_BLK_DEV_IDE_TCQ */
 
 		case ide_dma_read:
 			reading = 1 << 3;
 		case ide_dma_write:
-			ar = HWGROUP(drive)->rq->special;
+			ar = IDE_CUR_AR(drive);

Ahhh!!! I'm gald to see this.

drivers/ide/ide-dma.c

Quoted 6 lines

static inline void drive_ctl_nien(ide_drive_t *drive, int clear) {
#ifdef IDE_TCQ_NIEN
drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -1
-	int mask = clear ? 0 : 2;
+	int mask = clear ? 0 : 1 << 1;

????

drivers/ide/ide-dma.c

Quoted 2 lines

drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -1
-			BUG_ON(drive->tcq->active_tag == -1);
+			BUG_ON(drive->tcq->active_tag == IDE_INACTIVE_TAG);

IDE_IDLE_TAG would sound better I think... but no argument here.

drivers/ide/ide-dma.c

Quoted 3 lines

drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -2
-#define IDE_CUR_AR(drive)	\
-	((drive)->using_tcq ? IDE_CUR_TAG((drive)) : HWGROUP((drive))->rq->special)
+#define IDE_CUR_AR(drive)	(HWGROUP((drive))->rq->special)

Ahh that's nice :-). Let's look further down how this coexists with ide-cd.c...

drivers/ide/ide-dma.c

Quoted 2 lines

drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -1
-extern inline int ide_get_tag(ide_drive_t *drive)
+static inline int ide_get_tag(ide_drive_t *drive)

OK. I have missed this one apparently.

ar_timer will make it at least esier to finish the ide-cd.c transition to this data transport base.

Should I just quickly remerge this, so we can work further from the same code base to ease the merging pain?

Jens Axboe

author

2002-04-15 13:33 UTC #2 lore raw

In reply to Jens Axboe

Quoted 7 lines · Martin Dalecki

Quoted 3 lines · Jens Axboe
Hi,
Updated the patch to 2.5.8 (painful). Changes:
S*it - I did just offer you assistance... well you where faster. Anyway thank you for going into this trouble.

:-)

Quoted 11 lines

Quoted 7 lines
diff -urN -X /home/axboe/cdrom/exclude
/opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c
linux/drivers/block/ll_rw_blk.c
--- /opt/kernel/linux-2.5.8/drivers/block/ll_rw_blk.c 2002-04-15
07:59:53.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c 2002-04-15 08:42:23.000000000 +0200
I recognize the following makes sense but since it's affecting every single type of device out there it should go separetely to Linus I think...

They have.

Quoted 6 lines

drivers/ide/Config.help line 763
Quoted 2 lines
drivers/ide/Config.help renamed from opt/kernel/linux-2.5.8/drivers/ide/Config.help +2 -0
+  drive as a /\/\/\/\ queue size balance, where we could instead try and
+  maintain a minimum queue size and get a /---------\ graph instead.
Nice drawings :-). But the help text shouldn't be
that encouraging right now IMHO... (At least
we should mark the options as experimental. (OK I will do that...)

It's no more experiemental than the rest of it, in fact all my testing has been with _FULL option enabled right now. So I'd basically say it's the recommended setting :-)

Quoted 8 lines

Quoted 8 lines
diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide.c linux/drivers/ide/ide.c
--- /opt/kernel/linux-2.5.8/drivers/ide/ide.c 2002-04-15
10:05:03.000000000 +0200
+++ linux/drivers/ide/ide.c 2002-04-15 14:31:08.000000000 +0200
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -0
@@ -381,8 +381,23 @@
 		add_blkdev_randomness(major(rq->rq_dev));
 

Quoted 14 lines

drivers/ide/ide.c line 392

Quoted 12 lines

drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +9 -0

 		spin_lock_irqsave(&ide_lock, flags);
+
+		if ((jiffies - ar->ar_time > ATA_AR_MAX_TURNAROUND) && 
 drive->queue_depth > 1) {
+			printk("%s: exceeded max command turn-around time 
 (%d seconds)\n", drive->name, ATA_AR_MAX_TURNAROUND / HZ);
+			drive->queue_depth >>= 1;
+		}
+
+		if (jiffies - ar->ar_time > drive->tcq->oldest_command)
+			drive->tcq->oldest_command = jiffies - ar->ar_time;
+

time_before() and timer_after() should be used here.

Not necessarily, the above is wrap safe.

Quoted 3 lines

drivers/ide/ide.c line 1115
Quoted 1 line
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -0
 	while ((read_timer() - hwif->last_time) < DISK_RECOVERY_TIME);
Same here.

Ditto, at least for this one I agree that readibility would be better with the macros.

Quoted 6 lines

drivers/ide/ide.c line 1690
Quoted 3 lines
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +0 -2
-#ifdef CONFIG_BLK_DEV_IDEPCI
 		if (hwif->pci_dev && !hwif->pci_dev->vendor)
-#endif
Here I'm still not convinced whatever this should be implemented for all architectures...

Agree, we can leave this the way it was for now and just do the nasty ifdefs.

Quoted 6 lines

drivers/ide/ide.c line 2824
Quoted 2 lines
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +1 -1
-		memset(ar, 0, sizeof(*ar));
+		memset(ar, 0, sizeof(ar));
Please look closer - I'm quite convinced that sizeof(ar) == sizeof(void *) which gives not the desired effect. I have fixed this during the last merge...

Irk, where did that come from?! Had you done the incremental patches this would not have slipped through! My mistake.

Quoted 10 lines

Quoted 10 lines
static int ide_check_media_change (kdev_t i_rdev)
diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
--- /opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c 2002-04-15
07:59:53.000000000 +0200
+++ linux/drivers/ide/ide-disk.c 2002-04-15 10:20:33.000000000 +0200
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +0 -0
@@ -107,10 +107,7 @@
 	 * 48-bit commands are pretty sanely laid out
 	 */
 	if (lba48bit) {

Quoted 8 lines

drivers/ide/ide-disk.c line 110
Quoted 5 lines
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +1 -4
-		if (cmd == READ)
-			command = WIN_READ_EXT;
-		else
-			command = WIN_WRITE_EXT;
+		command = cmd == READ ? WIN_READ_EXT : WIN_WRITE_EXT;
Checking with GCC will reveal that the former version doesn't generate worser code...

I had already rewritten it by the time your changed version was out, so it was simply unchanged.

Quoted 12 lines

drivers/ide/ide-disk.c line 898
Quoted 9 lines
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +2 -2
 	return command;
@@ -895,24 +894,24 @@
 
 		__set_bit(i, &tag_mask);
 		len += sprintf(out+len, "%d, ", i);
-		if (ar->ar_time > max_jif)
-			max_jif = ar->ar_time;
+		if (cur_jif - ar->ar_time > max_jif)
+			max_jif = cur_jif - ar->ar_time;
I disgust timer calculartions...- will have to look at a way how to nap in a similar way eth drivers do.

? This is just statistics. There absolutely nothing wrong with the above.

Quoted 8 lines

Quoted 8 lines
diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
--- /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c 2002-04-15
07:59:53.000000000 +0200
+++ linux/drivers/ide/ide-dma.c 2002-04-15 09:17:55.000000000 +0200
+#endif /* CONFIG_BLK_DEV_IDE_TCQ */

Quoted 7 lines

drivers/ide/ide-dma.c line 634
Quoted 5 lines
		case ide_dma_read:
			reading = 1 << 3;
		case ide_dma_write:
drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -1
-			ar = HWGROUP(drive)->rq->special;
+			ar = IDE_CUR_AR(drive);
Ahhh!!! I'm gald to see this.

:-)

Quoted 6 lines

drivers/ide/ide-tcq.c line 65
Quoted 5 lines
static inline void drive_ctl_nien(ide_drive_t *drive, int clear) {
#ifdef IDE_TCQ_NIEN
drivers/ide/ide-tcq.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-tcq.c +1 -1
-	int mask = clear ? 0 : 2;
+	int mask = clear ? 0 : 1 << 1;
????

Just more readable that we are setting bit 2.

Quoted 14 lines

Quoted 4 lines
(quoted diff) +0 -2
-#define IDE_CUR_AR(drive)	\
-	((drive)->using_tcq ? IDE_CUR_TAG((drive)) : 
HWGROUP((drive))->rq->special)
+#define IDE_CUR_AR(drive) (HWGROUP((drive))->rq->special)
Ahh that's nice :-). Let's look further down how this coexists with ide-cd.c...
include/linux/ide.h line 1043
Quoted 2 lines
include/linux/ide.h renamed from opt/kernel/linux-2.5.8/include/linux/ide.h +1 -1
-extern inline int ide_get_tag(ide_drive_t *drive)
+static inline int ide_get_tag(ide_drive_t *drive)
OK. I have missed this one apparently.

Quoted 5 lines

ar_timer will make it at least esier to finish the ide-cd.c transition to this data transport base.
Should I just quickly remerge this, so we can work further from the same code base to ease the merging pain?

Sure, go ahead. And do the changes they way we discussed earlier today, ok? :)

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-04-15 13:41 UTC #3 lore raw

In reply to Jens Axboe

Quoted 6 lines · Jens Axboe

Quoted 6 lines
drivers/ide/ide.c line 2824
Quoted 2 lines
drivers/ide/ide.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide.c +1 -1
-		memset(ar, 0, sizeof(*ar));
+		memset(ar, 0, sizeof(ar));
Please look closer - I'm quite convinced that sizeof(ar) == sizeof(void *) which gives not the desired effect. I have fixed this during the last merge...

No problem here - you see I'm quite good at catching cr*ap like this ;-).

Quoted 16 lines

Irk, where did that come from?! Had you done the incremental patches this would not have slipped through! My mistake.
Quoted 10 lines
drivers/ide/ide-disk.c line 898
Quoted 7 lines
		__set_bit(i, &tag_mask);
		len += sprintf(out+len, "%d, ", i);
drivers/ide/ide-disk.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-disk.c +2 -2
-		if (ar->ar_time > max_jif)
-			max_jif = ar->ar_time;
+		if (cur_jif - ar->ar_time > max_jif)
+			max_jif = cur_jif - ar->ar_time;
I disgust timer calculartions...- will have to look at a way how to nap in a similar way eth drivers do.

Quoted 2 lines

? This is just statistics. There absolutely nothing wrong with the above.

Please don't take me wrong - I don't think that there is something wrong with it... It was more out of the stommach about how it looks...

Quoted 27 lines

Quoted 7 lines
Quoted 7 lines
diff -urN -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
--- /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c 2002-04-15
07:59:53.000000000 +0200
+++ linux/drivers/ide/ide-dma.c 2002-04-15 09:17:55.000000000 +0200
+#endif /* CONFIG_BLK_DEV_IDE_TCQ */
Quoted 7 lines
drivers/ide/ide-dma.c line 634
Quoted 5 lines
		case ide_dma_read:
			reading = 1 << 3;
		case ide_dma_write:
drivers/ide/ide-dma.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c +1 -1
-			ar = HWGROUP(drive)->rq->special;
+			ar = IDE_CUR_AR(drive);
Ahhh!!! I'm gald to see this.
:-)
Quoted 7 lines
drivers/ide/ide-tcq.c line 65
Quoted 5 lines
static inline void drive_ctl_nien(ide_drive_t *drive, int clear) {
#ifdef IDE_TCQ_NIEN
drivers/ide/ide-tcq.c renamed from opt/kernel/linux-2.5.8/drivers/ide/ide-tcq.c +1 -1
-	int mask = clear ? 0 : 2;
+	int mask = clear ? 0 : 1 << 1;
????

Quoted 1 line

Just more readable that we are setting bit 2.

0x02 is for me always a hint that one should read it binary... You memmorazied the bit patterns corresopnding to hex digits propably as well already a long time ago. Didn't you? :-).

Quoted 19 lines

Quoted 19 lines
Quoted 6 lines
(quoted diff) +0 -2
-#define IDE_CUR_AR(drive)	\
-	((drive)->using_tcq ? IDE_CUR_TAG((drive)) : 
HWGROUP((drive))->rq->special)
+#define IDE_CUR_AR(drive) (HWGROUP((drive))->rq->special)
Ahh that's nice :-). Let's look further down how this coexists with ide-cd.c...
include/linux/ide.h line 1043
Quoted 2 lines
include/linux/ide.h renamed from opt/kernel/linux-2.5.8/include/linux/ide.h +1 -1
-extern inline int ide_get_tag(ide_drive_t *drive)
+static inline int ide_get_tag(ide_drive_t *drive)
OK. I have missed this one apparently.
ar_timer will make it at least esier to finish the ide-cd.c transition to this data transport base.

Quoted 6 lines

Quoted 2 lines
Should I just quickly remerge this, so we can work further from the same code base to ease the merging pain?
Sure, go ahead. And do the changes they way we discussed earlier today, ok? :)

OK... agreed First - "raw" merge (and fixes already discuesed here) and then go on...

Aaron Tiensivu

2002-04-15 16:13 UTC #4 lore raw

In reply to Jens Axboe

Simple question but hopefully it has a simple answer.. is there a command you can issue or flag you can look for from the output of hdparm to tell if your hard drive is capable of TCQ before installing the patch? I have a few IBM drives that I'm sure have TCQ abilities but I don't trust them as far as I can throw them (being Hungarian and cursed) but I'd like to give TCQ a whirl on my WD 120GB drives that should work OK, if they support TCQ..

Sorry if it's already been asked.. :)

Jens Axboe

author

2002-04-18 12:58 UTC #5 lore raw

Quoted 2 lines · Martin Dalecki

BTW> Jens: Do you have any idea what the "sector chaing" in ide-cd is good for?! I would love to just get rid of it alltogether!

Sector chaining? Are you talking about the cdrom_read_intr() comments?

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-04-18 13:01 UTC #6 lore raw

In reply to Aaron Tiensivu

Quoted 7 lines · Jens Axboe

Quoted 2 lines · Martin Dalecki
BTW> Jens: Do you have any idea what the "sector chaing" in ide-cd is good for?! I would love to just get rid of it alltogether!
Sector chaining? Are you talking about the cdrom_read_intr() comments?

Sorry I did mean sector caching.

Jens Axboe

author

2002-04-15 16:18 UTC #7 lore raw

In reply to Jens Axboe

Quoted 8 lines · Aaron Tiensivu

Simple question but hopefully it has a simple answer.. is there a command you can issue or flag you can look for from the output of hdparm to tell if your hard drive is capable of TCQ before installing the patch? I have a few IBM drives that I'm sure have TCQ abilities but I don't trust them as far as I can throw them (being Hungarian and cursed) but I'd like to give TCQ a whirl on my WD 120GB drives that should work OK, if they support TCQ..
Sorry if it's already been asked.. :)

It has not been asked :-)

You can run a IDENTIFY_DEVICE from user space with the task ioctls and look at word 83 -- bit 1 and 14 must be set for TCQ to be supported. If you give me the model identifier from the IBM drive, I can tell you if it has tcq or not...

I'll write a small util to detect this tomorrow and send it to you + the list.

I thought only the IBM's had TCQ, but I got one report today indicating that some WD do too.

Signature · 1 line

Jens Axboe

Jens Axboe

author

2002-04-15 16:44 UTC #8 lore raw

In reply to Jens Axboe

Quoted 12 lines · Jens Axboe

Quoted 8 lines · Aaron Tiensivu
Simple question but hopefully it has a simple answer.. is there a command you can issue or flag you can look for from the output of hdparm to tell if your hard drive is capable of TCQ before installing the patch? I have a few IBM drives that I'm sure have TCQ abilities but I don't trust them as far as I can throw them (being Hungarian and cursed) but I'd like to give TCQ a whirl on my WD 120GB drives that should work OK, if they support TCQ..
Sorry if it's already been asked.. :)
It has not been asked :-)

Quoted 7 lines

You can run a IDENTIFY_DEVICE from user space with the task ioctls and look at word 83 -- bit 1 and 14 must be set for TCQ to be supported. If you give me the model identifier from the IBM drive, I can tell you if it has tcq or not...
I'll write a small util to detect this tomorrow and send it to you + the list.

Duh, you can of course just look at /proc/ide/ideX/hdY/identify and parse that. The info above is still valid for that, of course :-)

Signature · 1 line

Jens Axboe

Jens Axboe

author

2002-04-16 10:30 UTC #9 lore raw

In reply to Jens Axboe

Quoted 8 lines · Aaron Tiensivu

Simple question but hopefully it has a simple answer.. is there a command you can issue or flag you can look for from the output of hdparm to tell if your hard drive is capable of TCQ before installing the patch? I have a few IBM drives that I'm sure have TCQ abilities but I don't trust them as far as I can throw them (being Hungarian and cursed) but I'd like to give TCQ a whirl on my WD 120GB drives that should work OK, if they support TCQ..
Sorry if it's already been asked.. :)

Mark Hahn wrote this little script to detect support for TCQ, modified by me to not use the hdX symlinks.

Signature · 1 line

Jens Axboe

#!/usr/bin/perl

# bit 1 (TCQ) and 14 (word is valid) must be set to indicate tcq support
$mask = (1 << 1) | (1 << 14);

# bit 15 must be cleared too
$bits = $mask | (1 << 15);

# mail me the results!
$addr = "linux-tcq\@kernel.dk";

foreach $i (</proc/ide/ide*>) {
	foreach $d (<$i/hd*>) {
		@words = split(/\s/,`cat $d/identify`);
		$w83 = hex($words[83]);
		if (!(($w83 & $bits) ^ $mask)) {
			$model = `cat $d/model`;
			push(@goodies, $model);
			chomp($model);
			print "$d ($model) supports TCQ\n";
		}
	}
}

if ($addr && $#goodies) {
	open(M, "| mail -s TCQ-report $addr");
	print M @goodies;
	close(M);
}

Jens Axboe

author

2002-04-18 12:57 UTC #10 lore raw

Quoted 52 lines · Sebastian Droege

Quoted 49 lines · On Thu, 18 Apr 2002 13:20:15 +0200 Martin Dalecki…
Quoted 48 lines · Sebastian Droege
Quoted 10 lines · On Wed, 17 Apr 2002 12:42:39 +0200 Martin Dalecki…
Quoted 5 lines
2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant)
but the problem shows up only with hdc
Duh oh... This is actually a good hint. I will look after this.
Quoted 12 lines
Quoted 3 lines
and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
This is harmless it just shows that there was no DMA transfer in flight the time you have cat-ed this file.
Quoted 3 lines
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings
I'll do some more testings later the day
Hi again,
after some playing with hdparm I found something bad :(
ide_tcq_intr_timeout: timeout waiting for interrupt
ide_tcq_intr_timeout: hwgroup not busy
hda: invalidating pending queue
ide_tcq_invalidate_queue: done
and then... nothing works anymore -> total lockup of the IDE system
This happens only (?) when I put hdparm -qa1 -qA1 -qc1 -qd1 -qm16 -qu1 -qW1 /dev/hda (the same with hdb) in my bootscripts When I start hdparm later everything works fine first, but after a while (and when I'm under X) the IDE system is crashed
I have enabled following options:
CONFIG_BLK_DEV_IDE_TCQ=y
CONFIG_BLK_DEV_IDE_TCQ_FULL=y
CONFIG_BLK_DEV_IDE_TCQ_DEFAULT=y
CONFIG_BLK_DEV_IDE_TCQ_DEPTH=32
I hope this helps somehow but I don't know what further informations I can provide
BTW: I think we should create devfs entries for ide-scsi devices (because hdparm doesn't take scdX ;) )

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Bye
I don't know whatever you have already tryed the recend patches for 2.5.8. The problem in place is presumably fixed there (tough there are still problems with ide-cd remaining which I'm working on right now.)
This problem exists also with your newest ide patches (I think 39 is the newest)
And again: I don't see how ide-cd can cause this problem... I access my cdroms via scsi-emulation and ide-cd isn't compiled into the kernel

Try disabling the CONFIG_BLK_DEV_IDE_TCQ_FULL option.

Signature · 1 line

Jens Axboe

Helge Hafting

2002-04-17 07:33 UTC #11 lore raw

In reply to Jens Axboe

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Aaron Tiensivu
Simple question but hopefully it has a simple answer.. is there a command you can issue or flag you can look for from the output of hdparm to tell if your hard drive is capable of TCQ before installing the patch? I have a few IBM drives that I'm sure have TCQ abilities but I don't trust them as far as I can throw them (being Hungarian and cursed) but I'd like to give TCQ a whirl on my WD 120GB drives that should work OK, if they support TCQ..

Quoted 4 lines

Quoted 1 line
Sorry if it's already been asked.. :)
Mark Hahn wrote this little script to detect support for TCQ, modified by me to not use the hdX symlinks.

I tried it. It had to run as root to get permission to read. Then it hung the machine - caps & scroll lock blinking.

I can retry it without X (and fs'es remounted r/o) _if_ the resulting crash may help with debugging.

IDE interface: Intel Corp. 82371AB PIIX4 IDE (rev 01)

Helge Hafting

Martin Dalecki

2002-04-17 08:50 UTC #12 lore raw

In reply to Jens Axboe

Quoted 13 lines · Sebastian Droege

Quoted 9 lines · On Tue, 16 Apr 2002 20:09:14 +0200 Jens Axboe <[email protected]> wrote:
Quoted 4 lines · Sebastian Droege
Hi,
just one short question:
My hda supports TCQ but my hdb doesn't
Is it safe to enable TCQ in kernel config?
yes, should be safe.

Quoted 7 lines

Quoted 3 lines
Signature · 1 line
Jens Axboe
Ok it really works ;)
But there's another problem in 2.5.8 with ide patches until 37 applied (they don't appear with 2.5.8 and ide patches until 35), the unexpected interrupts (look at the relevant dmesg output at the bottom). They appear with and without TCQ enabled. If you need more informations, just ask :)

They are not a problem. They are just diagnostics for us and will go away at some point in time.

Sebastian Droege

2002-04-17 11:29 UTC #13 lore raw

Quoted 18 lines · On Wed, 17 Apr 2002 09:48:33 +0200 Martin Dalecki…

Quoted 17 lines · Sebastian Droege
Quoted 12 lines · On Tue, 16 Apr 2002 20:09:14 +0200 Jens Axboe <[email protected]> wrote:
Quoted 4 lines · Sebastian Droege
Hi,
just one short question:
My hda supports TCQ but my hdb doesn't
Is it safe to enable TCQ in kernel config?
yes, should be safe.
Signature · 1 line
Jens Axboe

Quoted 6 lines

Quoted 3 lines
Ok it really works ;)
But there's another problem in 2.5.8 with ide patches until 37 applied (they don't appear with 2.5.8 and ide patches until 35), the unexpected interrupts (look at the relevant dmesg output at the bottom). They appear with and without TCQ enabled. If you need more informations, just ask :)
They are not a problem. They are just diagnostics for us and will go away at some point in time.

Ok but there are actually 2 real problems then...
1.
TCQ on hda is enabled with queue depth 32 and CONFIG_BLK_DEV_IDE_TCQ_FULL enabled
When I do many transfers on the hard disk I get a "ide_dma_queued_start: abort (stat=d0)" after some time and the IDE system doesn't respond anymore :( 2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts
hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant) but the problem shows up only with hdc

and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings

I'll do some more testings later the day

Bye

Jens Axboe

author

2002-04-18 13:26 UTC #14 lore raw

Quoted 40 lines · Martin Dalecki

Quoted 39 lines · Jens Axboe
Quoted 16 lines · Martin Dalecki
Quoted 14 lines · Jens Axboe
Quoted 11 lines · Martin Dalecki
Quoted 8 lines · Jens Axboe
Quoted 2 lines · Martin Dalecki
BTW> Jens: Do you have any idea what the "sector chaing" in ide-cd is good for?! I would love to just get rid of it alltogether!
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Sector chaining? Are you talking about the cdrom_read_intr() comments?
Sorry I did mean sector caching.
That's for padding/caching sub-frame sized reads.
I tought the BIO layer did this alredy... Well it's a pain
Nope, it does not.
Quoted 1 line
in the a** to deal with it. IDE-FLOPPY is passing packet commands
It sure is... sr doesn't do it and lots of others don't as well, so I suppose we could rip it out. We already require reblocking with loop in those cases anyway.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
through the request buffer but IDE-CD is passing them through request special field... argh!
So kill ->special usage in ide-cd and use ->buffer?
That's the idea, but the caching code mentioned above is abusing it already in a way I can't grasp wholly immediately.

It's most definitely _not_ abusing it, in fact it's a pretty regular usage of ->buffer. ide-cd never does highmem I/O, so ->buffer always points to the transfer address for a block request. cdrom_read_from_buffer() is simply copying data from the internal 2kb cache to rq->buffer, eod.

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-04-18 13:42 UTC #15 lore raw

In reply to Sebastian Droege

Quoted 10 lines · Jens Axboe

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
through the request buffer but IDE-CD is passing them through request special field... argh!
So kill ->special usage in ide-cd and use ->buffer?
That's the idea, but the caching code mentioned above is abusing it already in a way I can't grasp wholly immediately.

Quoted 5 lines

It's most definitely _not_ abusing it, in fact it's a pretty regular usage of ->buffer. ide-cd never does highmem I/O, so ->buffer always points to the transfer address for a block request. cdrom_read_from_buffer() is simply copying data from the internal 2kb cache to rq->buffer, eod.

Either way it's ide-floppy doing the abuse then :-(.
I think the best sollution to this is anyway just to remove the struct packet_command altogeter and just add the required fields to struct ata_request - makes ata_request bigger but provides much less pointer tossing as a benefit.

Quoted 1 line

Signature · 3 lines

- phone: +49 214 8656 283
- job:   eVision-Ventures AG, LEV .de (MY OPINIONS ARE MY OWN!)
- langs: de_DE.ISO8859-1, en_US, pl_PL.ISO8859-2, last ressort: ru_RU.KOI8-R

Martin Dalecki

2002-04-18 13:48 UTC #16 lore raw

In reply to Sebastian Droege

Quoted 5 lines · Martin Dalecki

Either way it's ide-floppy doing the abuse then :-(.
I think the best sollution to this is anyway just to remove the struct packet_command altogeter and just add the required fields to struct ata_request - makes ata_request bigger but provides much less pointer tossing as a benefit.

I just did it. Advantage is: DMA works again. However on one of the two read paths in ide-cd (FS vers block read) I get irq underruns. This should be now fixable...

Martin Dalecki

2002-04-17 11:34 UTC #17 lore raw

In reply to Jens Axboe

Quoted 40 lines · Sebastian Droege

Quoted 21 lines · On Wed, 17 Apr 2002 09:48:33 +0200 Martin Dalecki…
Quoted 19 lines · Sebastian Droege
Quoted 13 lines · On Tue, 16 Apr 2002 20:09:14 +0200 Jens Axboe <[email protected]> wrote:
Quoted 4 lines · Sebastian Droege
Hi,
just one short question:
My hda supports TCQ but my hdb doesn't
Is it safe to enable TCQ in kernel config?
yes, should be safe.
Signature · 1 line
Jens Axboe
Quoted 6 lines
Quoted 3 lines
Ok it really works ;)
But there's another problem in 2.5.8 with ide patches until 37 applied (they don't appear with 2.5.8 and ide patches until 35), the unexpected interrupts (look at the relevant dmesg output at the bottom). They appear with and without TCQ enabled. If you need more informations, just ask :)
They are not a problem. They are just diagnostics for us and will go away at some point in time.
Ok but there are actually 2 real problems then...
1.
TCQ on hda is enabled with queue depth 32 and CONFIG_BLK_DEV_IDE_TCQ_FULL enabled
When I do many transfers on the hard disk I get a "ide_dma_queued_start: abort (stat=d0)" after some time and the IDE system doesn't respond anymore :( 2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts
hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant) but the problem shows up only with hdc

Quoted 4 lines

and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings

Basically right now DMA on CD-ROM devices is "disabled" due to the fact that ide-cd.c is pasing struct packet_command through the rq->special field. This is subject to change soon becouse I'm right now adpting ide-cd to use the recently introduced struct ata_request stuff.
So it doesn't make much sense if you test it - becouse the issues are just well known. However if you see an IDE xx which contains a "fix ide-cd blah blah" in the log - I would be really glad if you could have a look in to it.

Sebastian Droege

2002-04-17 11:40 UTC #18 lore raw

Quoted 46 lines · On Wed, 17 Apr 2002 12:32:15 +0200 Martin Dalecki…

Quoted 40 lines · Sebastian Droege
Quoted 21 lines · On Wed, 17 Apr 2002 09:48:33 +0200 Martin Dalecki…
Quoted 19 lines · Sebastian Droege
Quoted 13 lines · On Tue, 16 Apr 2002 20:09:14 +0200 Jens Axboe <[email protected]> wrote:
Quoted 4 lines · Sebastian Droege
Hi,
just one short question:
My hda supports TCQ but my hdb doesn't
Is it safe to enable TCQ in kernel config?
yes, should be safe.
Signature · 1 line
Jens Axboe
Quoted 6 lines
Quoted 3 lines
Ok it really works ;)
But there's another problem in 2.5.8 with ide patches until 37 applied (they don't appear with 2.5.8 and ide patches until 35), the unexpected interrupts (look at the relevant dmesg output at the bottom). They appear with and without TCQ enabled. If you need more informations, just ask :)
They are not a problem. They are just diagnostics for us and will go away at some point in time.
Ok but there are actually 2 real problems then...
1.
TCQ on hda is enabled with queue depth 32 and CONFIG_BLK_DEV_IDE_TCQ_FULL enabled
When I do many transfers on the hard disk I get a "ide_dma_queued_start: abort (stat=d0)" after some time and the IDE system doesn't respond anymore :( 2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts
hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant) but the problem shows up only with hdc
Quoted 4 lines
and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings

Quoted 8 lines

Basically right now DMA on CD-ROM devices is "disabled" due to the fact that ide-cd.c is pasing struct packet_command through the rq->special field. This is subject to change soon becouse I'm right now adpting ide-cd to use the recently introduced struct ata_request stuff.
So it doesn't make much sense if you test it - becouse the issues are just well known. However if you see an IDE xx which contains a "fix ide-cd blah blah" in the log - I would be really glad if you could have a look in to it.

Yeah sure but hda is a hard disk
The both cdrom drives aren't touched by hdparm
And I don't see such message (maybe because the both cdroms are accessed via scsi emulation?!) and atapi cdrom support isn't compiled into the kernel

Bye

Martin Dalecki

2002-04-17 11:44 UTC #19 lore raw

In reply to Jens Axboe

Quoted 4 lines

2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant) but the problem shows up only with hdc

Duh oh... This is actually a good hint. I will look after this.

Quoted 4 lines

and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running

This is harmless it just shows that there was no DMA transfer in flight the time you have cat-ed this file.

Quoted 3 lines

but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings
I'll do some more testings later the day

Sebastian Droege

2002-04-18 12:19 UTC #20 lore raw

Quoted 9 lines · On Wed, 17 Apr 2002 12:42:39 +0200 Martin Dalecki…

Quoted 4 lines
2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant) but the problem shows up only with hdc
Duh oh... This is actually a good hint. I will look after this.
Quoted 1 line

Quoted 10 lines

Quoted 3 lines
and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
This is harmless it just shows that there was no DMA transfer in flight the time you have cat-ed this file.
Quoted 3 lines
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings
I'll do some more testings later the day

Hi again,
after some playing with hdparm I found something bad :(

ide_tcq_intr_timeout: timeout waiting for interrupt
ide_tcq_intr_timeout: hwgroup not busy
hda: invalidating pending queue
ide_tcq_invalidate_queue: done

and then... nothing works anymore -> total lockup of the IDE system
This happens only (?) when I put hdparm -qa1 -qA1 -qc1 -qd1 -qm16 -qu1 -qW1 /dev/hda (the same with hdb) in my bootscripts When I start hdparm later everything works fine first, but after a while (and when I'm under X) the IDE system is crashed

I have enabled following options:
CONFIG_BLK_DEV_IDE_TCQ=y
CONFIG_BLK_DEV_IDE_TCQ_FULL=y
CONFIG_BLK_DEV_IDE_TCQ_DEFAULT=y
CONFIG_BLK_DEV_IDE_TCQ_DEPTH=32

I hope this helps somehow but I don't know what further informations I can provide

BTW: I think we should create devfs entries for ide-scsi devices (because hdparm doesn't take scdX ;) )

Bye

Dave Jones

2002-04-17 13:01 UTC #21 lore raw

In reply to Jens Axboe

Quoted 5 lines · Helge Hafting

I tried it. It had to run as root to get permission to read. Then it hung the machine - caps & scroll lock blinking.
I can retry it without X (and fs'es remounted r/o) _if_ the resulting crash may help with debugging.

The blinking LEDs indicate you oopsed. Try it without X and you should see an oops that when decoded may be useful for Jens / Martin.

Signature · 2 lines

| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Jens Axboe

author

2002-04-17 13:05 UTC #22 lore raw

In reply to Jens Axboe

Quoted 9 lines · Dave Jones

Quoted 5 lines · Helge Hafting
I tried it. It had to run as root to get permission to read. Then it hung the machine - caps & scroll lock blinking.
I can retry it without X (and fs'es remounted r/o) _if_ the resulting crash may help with debugging.
The blinking LEDs indicate you oopsed. Try it without X and you should see an oops that when decoded may be useful for Jens / Martin.

This bug has been fixed in the latest patches, it's the 'ar was on stack, don't return to pool' bug.

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-04-18 12:22 UTC #23 lore raw

In reply to Jens Axboe

Quoted 44 lines · Sebastian Droege

Quoted 10 lines · On Wed, 17 Apr 2002 12:42:39 +0200 Martin Dalecki…
Quoted 5 lines
2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant)
but the problem shows up only with hdc
Duh oh... This is actually a good hint. I will look after this.
Quoted 12 lines
Quoted 3 lines
and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
This is harmless it just shows that there was no DMA transfer in flight the time you have cat-ed this file.
Quoted 3 lines
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings
I'll do some more testings later the day
Hi again,
after some playing with hdparm I found something bad :(
ide_tcq_intr_timeout: timeout waiting for interrupt
ide_tcq_intr_timeout: hwgroup not busy
hda: invalidating pending queue
ide_tcq_invalidate_queue: done
and then... nothing works anymore -> total lockup of the IDE system
This happens only (?) when I put hdparm -qa1 -qA1 -qc1 -qd1 -qm16 -qu1 -qW1 /dev/hda (the same with hdb) in my bootscripts When I start hdparm later everything works fine first, but after a while (and when I'm under X) the IDE system is crashed
I have enabled following options:
CONFIG_BLK_DEV_IDE_TCQ=y
CONFIG_BLK_DEV_IDE_TCQ_FULL=y
CONFIG_BLK_DEV_IDE_TCQ_DEFAULT=y
CONFIG_BLK_DEV_IDE_TCQ_DEPTH=32

Quoted 5 lines

I hope this helps somehow but I don't know what further informations I can provide
BTW: I think we should create devfs entries for ide-scsi devices (because hdparm doesn't take scdX ;) )
Bye

I don't know whatever you have already tryed the recend patches for 2.5.8. The problem in place is presumably fixed there (tough there are still problems with ide-cd remaining which I'm working on right now.)

BTW> Jens: Do you have any idea what the "sector chaing" in ide-cd is good for?! I would love to just get rid of it alltogether!

Signature · 3 lines

- phone: +49 214 8656 283
- job:   eVision-Ventures AG, LEV .de (MY OPINIONS ARE MY OWN!)
- langs: de_DE.ISO8859-1, en_US, pl_PL.ISO8859-2, last ressort: ru_RU.KOI8-R

Sebastian Droege

2002-04-18 12:28 UTC #24 lore raw

Quoted 47 lines · On Thu, 18 Apr 2002 13:20:15 +0200 Martin Dalecki…

Quoted 46 lines · Sebastian Droege
Quoted 10 lines · On Wed, 17 Apr 2002 12:42:39 +0200 Martin Dalecki…
Quoted 5 lines
2.
when I cat /proc/ide/ide1/hdc/identify I get 2 unexpected interrupts hdc and hdd are both cdrom drives (accessed via scsi-emulation if relevant)
but the problem shows up only with hdc
Duh oh... This is actually a good hint. I will look after this.
Quoted 12 lines
Quoted 3 lines
and maybe a third problem ;)
in /proc/ide/ide0/hda/tcq there is written:
DMA status: not running
This is harmless it just shows that there was no DMA transfer in flight the time you have cat-ed this file.
Quoted 3 lines
but DMA is explicitly enabled by hdparm and shows up in /proc/ide/ide0/hda/settings
I'll do some more testings later the day
Hi again,
after some playing with hdparm I found something bad :(
ide_tcq_intr_timeout: timeout waiting for interrupt
ide_tcq_intr_timeout: hwgroup not busy
hda: invalidating pending queue
ide_tcq_invalidate_queue: done
and then... nothing works anymore -> total lockup of the IDE system
This happens only (?) when I put hdparm -qa1 -qA1 -qc1 -qd1 -qm16 -qu1 -qW1 /dev/hda (the same with hdb) in my bootscripts When I start hdparm later everything works fine first, but after a while (and when I'm under X) the IDE system is crashed
I have enabled following options:
CONFIG_BLK_DEV_IDE_TCQ=y
CONFIG_BLK_DEV_IDE_TCQ_FULL=y
CONFIG_BLK_DEV_IDE_TCQ_DEFAULT=y
CONFIG_BLK_DEV_IDE_TCQ_DEPTH=32
I hope this helps somehow but I don't know what further informations I can provide

Quoted 8 lines

Quoted 3 lines
BTW: I think we should create devfs entries for ide-scsi devices (because hdparm doesn't take scdX ;) )
Bye
I don't know whatever you have already tryed the recend patches for 2.5.8. The problem in place is presumably fixed there (tough there are still problems with ide-cd remaining which I'm working on right now.)

This problem exists also with your newest ide patches (I think 39 is the newest)
And again: I don't see how ide-cd can cause this problem... I access my cdroms via scsi-emulation and ide-cd isn't compiled into the kernel

Bye

Jens Axboe

author

2002-04-18 13:02 UTC #25 lore raw

In reply to Jens Axboe

Quoted 1 line · Sebastian Droege

This happens only (?) when I put hdparm -qa1 -qA1 -qc1 -qd1 -qm16 -qu1 -qW1 /dev/hda (the same with hdb) in my bootscripts

btw, using -mXX and -d1 at the same time makes no sense, -m is multi pio setting and has no impact when using dma. write caching is ready enabled with tcq currently, so you don't need to do that either.

Signature · 1 line

Jens Axboe

Martin Schewe

2002-04-30 19:58 UTC #26 lore raw

In reply to Jens Axboe

Hi,

Quoted 29 lines · Jens Axboe

Mark Hahn wrote this little script to detect support for TCQ, modified by me to not use the hdX symlinks.

[...]

#!/usr/bin/perl

# bit 1 (TCQ) and 14 (word is valid) must be set to indicate tcq support
$mask = (1 << 1) | (1 << 14);

# bit 15 must be cleared too
$bits = $mask | (1 << 15);

# mail me the results!
$addr = "linux-tcq\@kernel.dk";

foreach $i (</proc/ide/ide*>) {
	foreach $d (<$i/hd*>) {
		@words = split(/\s/,`cat $d/identify`);
		$w83 = hex($words[83]);
		if (!(($w83 & $bits) ^ $mask)) {
			$model = `cat $d/model`;
			push(@goodies, $model);
			chomp($model);
			print "$d ($model) supports TCQ\n";
		}
	}
}

Quoted 1 line

if ($addr && $#goodies) {

$#goodies refers to the last index of the array and scalar @goodies to the actual number of elements. So you probably got only mails from people having more than two drives supporting TCQ... :)

Quoted 4 lines

	open(M, "| mail -s TCQ-report $addr");
	print M @goodies;
	close(M);
}

Fixed version attached.

Martin

#!/usr/bin/perl

# bit 1 (TCQ) and 14 (word is valid) must be set to indicate tcq support
$mask = (1 << 1) | (1 << 14);

# bit 15 must be cleared too
$bits = $mask | (1 << 15);

# mail me the results!
$addr = "linux-tcq\@kernel.dk";

foreach $i (</proc/ide/ide*>) {
	foreach $d (<$i/hd*>) {
		@words = split(/\s/,`cat $d/identify`);
		$w83 = hex($words[83]);
		if (!(($w83 & $bits) ^ $mask)) {
			$model = `cat $d/model`;
			push(@goodies, $model);
			chomp($model);
			print "$d ($model) supports TCQ\n";
		}
	}
}

if ($addr && @goodies) {
	open(M, "| mail -s TCQ-report $addr");
	print M @goodies;
	close(M);
}