Re: [PATCH] IDE TCQ #4

Martin Dalecki
2002-04-15T13:26:56Z

Jens Axboe wrote:
> 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:

>
> 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...

> + 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...)

> 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
> @@ -381,8 +381,23 @@
> add_blkdev_randomness(major(rq->rq_dev));
>
> 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.

> 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.

> + if (0 < (signed long)(jiffies + WAIT_MIN_SLEEP - sleep))
> + sleep = jiffies + WAIT_MIN_SLEEP;

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

> -#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...

> - 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...

> 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
> @@ -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...

> 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.

> 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 */
>
> 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.

>
> static inline void drive_ctl_nien(ide_drive_t *drive, int clear)
> {
> #ifdef IDE_TCQ_NIEN
> - int mask = clear ? 0 : 2;
> + int mask = clear ? 0 : 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.

> -#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...

> -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?