aic94xx: Increase can_queue and cmds_per_lun

This is unnecessary unless you alter it before host alloc (which is where it takes the shost values from the template).

Also, I think if you look at the rest of the driver, it's careful to account for the need for required scbs in its internal queueing algorithms, so the ASD_FREE_SCBS should be unnecessary.

Review

Quoted 1 line

(quoted diff) +1 -0

+	shost->can_queue = aic94xx_sht.can_queue;

This is unnecessary unless you alter it before host alloc (which is where it takes the shost values from the template).

Also, I think if you look at the rest of the driver, it's careful to account for the need for required scbs in its internal queueing algorithms, so the ASD_FREE_SCBS should be unnecessary.

Review

Quoted 1 line

(quoted diff) +1 -0

+	shost->can_queue = aic94xx_sht.can_queue;

Below is a patch that sets cmd_per_lun and can_queue in the aic94xx driver's scsi_host_template to better performing values than what's there currently. The cmd_per_lun setting is stolen straight out of the adp94xx source, and can_queue is derived from the max_scb value that we calculate in asd_init_hw. To the best of my (admittedly limited) knowledge, this method provides the correct values (can_queue = 443 in both adp94xx and aic94xx on my 9405W) but if anybody knows better, please enlighten me. :)

That said, the effect of leaving these values set to 1 is terrible performance in the case of either

Just for grins, I ran bogodisk (an O_DIRECT-enabled read speed test) against three different scenarios:

1) adp94xx 1.0.8-6, pounded into 2.6.18-rc4 [green]
2) aic94xx 1.0.2, without this patch [red]
3) aic94xx 1.0.2, with this [blue]

...with these results:

http://sweaglesw.net/~djwong/programs/bogodisk/bd_graphs/bad_sas.0.png

As you can see, the read performance is cut in half by the aic94xx driver not getting a chance to have multiple I/Os in flight at any given time. With the patch, the two drivers are fairly close bandwidth-wise. Also thanks to Mike Anderson for helping me figure this out.

drivers/scsi/aic94xx/aic94xx_init.c static struct scsi_host_template aic94xx · line 75

Quoted 8 lines

drivers/scsi/aic94xx/aic94xx_init.c +1 -1

@@ -71,7 +72,7 @@ static struct scsi_host_template aic94xx
      .change_queue_type      = sas_change_queue_type,
      .bios_param             = sas_bios_param,
      .can_queue              = 1,
-     .cmd_per_lun            = 1,
+     .cmd_per_lun            = 2,

Why 2? Why not 3? If you can set this to 3, then why not 4? But if you can set it to 4, why not 5?

This value should also be dynamically set, it should depend on the type of LU and it shouldn't be present in the host template. (But that's an architectural argument which leads nowhere on lsml.)

Quoted 12 lines · Luben Tuikov

drivers/scsi/aic94xx/aic94xx_init.c static struct scsi_host_template aic94xx · line 75

Quoted 8 lines

drivers/scsi/aic94xx/aic94xx_init.c +1 -1

@@ -71,7 +72,7 @@ static struct scsi_host_template aic94xx
      .change_queue_type      = sas_change_queue_type,
      .bios_param             = sas_bios_param,
      .can_queue              = 1,
-     .cmd_per_lun            = 1,
+     .cmd_per_lun            = 2,

Why 2? Why not 3? If you can set this to 3, then why not 4? But if you can set it to 4, why not 5?

Quoted 3 lines

This value should also be dynamically set, it should depend on the type of LU and it shouldn't be present in the host template. (But that's an architectural argument which leads nowhere on lsml.)

actually, cmd_per_lun is pretty much a vestigial variable. It's used for the initial queue depth before the driver decides to turn on TCQ. Thus, since the non-tagged depth should always only be 1, the only useful value to set it to is 1.

Hi all,

Below is a patch that sets can_queue in the aic94xx driver's scsi_host to better performing values than what's there currently. It seems that asd_ha->seq.can_queue reflects the number of requests that can be queued per controller; so long as there's one scsi_host per controller, it seems logical that the scsi_host ought to have the same can_queue value. To the best of my (still limited) knowledge, this method provides the correct value.

The effect of leaving this value set to 1 is terrible performance in the case of either (a) certain Maxtor SAS drives flying solo or (b) flooding several disks with I/O simultaneously (md-raid). There may be more scenarios where we see similar problems that I haven't uncovered.