PATCH [3/5] qla2xxx: TCQ fixes

PATCH [3/5] qla2xxx: TCQ fixes

[Andrew Vasquez]

ChangeSet
  1.1867 04/07/12 09:38:28 andrew.vasquez@apc.qlogic.com +2 -0
  Correct usage of tag-command-queueing methods:
  
  o Properly call scsi_activate_tcq() rather than
    scsi_adjust_queue_depth().
  o Properly retrieve tag message from command via
    scsi_populate_tag_msg().

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>

 drivers/scsi/qla2xxx/qla_iocb.c |   16 +++++++++++-----
 drivers/scsi/qla2xxx/qla_os.c   |    2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff -Nru a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
--- a/drivers/scsi/qla2xxx/qla_iocb.c	2004-07-12 09:54:49 -07:00
+++ b/drivers/scsi/qla2xxx/qla_iocb.c	2004-07-12 09:54:49 -07:00
@@ -22,6 +22,8 @@
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 
+#include <scsi/scsi_tcq.h>
+
 static inline uint16_t qla2x00_get_cmd_direction(struct scsi_cmnd *cmd);
 static inline cont_entry_t *qla2x00_prep_cont_type0_iocb(scsi_qla_host_t *);
 static inline cont_a64_entry_t *qla2x00_prep_cont_type1_iocb(scsi_qla_host_t *);
@@ -337,6 +339,7 @@
 	uint16_t	req_cnt;
 	uint16_t	tot_dsds;
 	device_reg_t	*reg;
+	char		tag[2];
 
 	/* Setup device pointers. */
 	ret = 0;
@@ -415,14 +418,17 @@
 	cmd_pkt->lun = cpu_to_le16(fclun->lun);
 
 	/* Update tagged queuing modifier */
-	cmd_pkt->control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG);
-	if (cmd->device->tagged_supported) {
-		switch (cmd->tag) {
-		case HEAD_OF_QUEUE_TAG:
+	if (scsi_populate_tag_msg(cmd, tag)) {
+		switch (tag[0]) {
+		case MSG_SIMPLE_TAG:
+			cmd_pkt->control_flags =
+			    __constant_cpu_to_le16(CF_SIMPLE_TAG);
+			break;
+		case MSG_HEAD_TAG:
 			cmd_pkt->control_flags =
 			    __constant_cpu_to_le16(CF_HEAD_TAG);
 			break;
-		case ORDERED_QUEUE_TAG:
+		case MSG_ORDERED_TAG:
 			cmd_pkt->control_flags =
 			    __constant_cpu_to_le16(CF_ORDERED_TAG);
 			break;
diff -Nru a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
--- a/drivers/scsi/qla2xxx/qla_os.c	2004-07-12 09:54:49 -07:00
+++ b/drivers/scsi/qla2xxx/qla_os.c	2004-07-12 09:54:49 -07:00
@@ -1784,7 +1784,7 @@
 
 		ql2xmaxqdepth = queue_depth;
 
-		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, queue_depth);
+		scsi_activate_tcq(sdev, queue_depth);
 
 		qla_printk(KERN_INFO, ha,
 		    "scsi(%d:%d:%d:%d): Enabled tagged queuing, queue "

Re: PATCH [3/5] qla2xxx: TCQ fixes

[James Bottomley]

On Mon, 2004-07-12 at 09:05, Andrew Vasquez wrote:
> ChangeSet
>   1.1867 04/07/12 09:38:28 andrew.vasquez@apc.qlogic.com +2 -0
>   Correct usage of tag-command-queueing methods:

I really don't think it is.

For the way the qla2xxx works, the correct tag usage would be to use per
HBA tags (which were introduced for aic7xxx but never used by them, so I
haven't actually created the mid-layer API), to populate your queue
handle field with the tag and to dispense with your outstanding_cmnds[]
array and use scsi_find_tag() to get back the command from the handle.

Since you'd be the first user of this API, I'd be happy to come up with
it for a willing victim^Wvolunteer.

James

Re: PATCH [3/5] qla2xxx: TCQ fixes

[Mike Christie]

James Bottomley wrote:
> On Mon, 2004-07-12 at 09:05, Andrew Vasquez wrote:
> 
>>ChangeSet
>>  1.1867 04/07/12 09:38:28 andrew.vasquez@apc.qlogic.com +2 -0
>>  Correct usage of tag-command-queueing methods:
> 
> 
> I really don't think it is.
> 
> For the way the qla2xxx works, the correct tag usage would be to use per
> HBA tags (which were introduced for aic7xxx but never used by them, so I
> haven't actually created the mid-layer API), to populate your queue
> handle field with the tag and to dispense with your outstanding_cmnds[]
> array and use scsi_find_tag() to get back the command from the handle.
> 
> Since you'd be the first user of this API, I'd be happy to come up with
> it for a willing victim^Wvolunteer.

Depending on the API, we might be able to use it for the linux-iscsi 
driver. Today the driver uses scsi_activate_tcq(), but does not uses the 
tag numbers. Instead it uses its own initiator task tags values for 
session wide identifiers, becuase we need tag values for iscsi ops. For 
example if we need to send a ping how should we get a tag from the mid 
layer? Or maybe becuase there are some transport specific tag values 
should we use someting else or keep it in the driver?

Mike

Re: PATCH [3/5] qla2xxx: TCQ fixes

[Brian King]

Mike Christie wrote:
> James Bottomley wrote:
> 
>> On Mon, 2004-07-12 at 09:05, Andrew Vasquez wrote:
>>
>>> ChangeSet
>>>  1.1867 04/07/12 09:38:28 andrew.vasquez@apc.qlogic.com +2 -0
>>>  Correct usage of tag-command-queueing methods:
>>
>>
>>
>> I really don't think it is.
>>
>> For the way the qla2xxx works, the correct tag usage would be to use per
>> HBA tags (which were introduced for aic7xxx but never used by them, so I
>> haven't actually created the mid-layer API), to populate your queue
>> handle field with the tag and to dispense with your outstanding_cmnds[]
>> array and use scsi_find_tag() to get back the command from the handle.
>>
>> Since you'd be the first user of this API, I'd be happy to come up with
>> it for a willing victim^Wvolunteer.
> 
> 
> Depending on the API, we might be able to use it for the linux-iscsi 
> driver. Today the driver uses scsi_activate_tcq(), but does not uses the 
> tag numbers. Instead it uses its own initiator task tags values for 
> session wide identifiers, becuase we need tag values for iscsi ops. For 
> example if we need to send a ping how should we get a tag from the mid 
> layer? Or maybe becuase there are some transport specific tag values 
> should we use someting else or keep it in the driver?

ipr is not interested in the tag numbers either, as the adapter 
microcode generates them.

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: PATCH [3/5] qla2xxx: TCQ fixes

[James Bottomley]

On Mon, 2004-07-12 at 21:28, Brian King wrote:
> ipr is not interested in the tag numbers either, as the adapter 
> microcode generates them.

Well, so does the qla2xxx.  However, all such implementations use driver
to controller handles which are direct tag equivalents (even if they're
not the actual tags sent over the wire). "Tag" in this sense merely
means identifying number from driver to host.

James

Re: PATCH [3/5] qla2xxx: TCQ fixes

[James Bottomley]

On Mon, 2004-07-12 at 19:19, Mike Christie wrote:
> Depending on the API, we might be able to use it for the linux-iscsi 
> driver. Today the driver uses scsi_activate_tcq(), but does not uses the 
> tag numbers. Instead it uses its own initiator task tags values for 
> session wide identifiers, becuase we need tag values for iscsi ops. For 
> example if we need to send a ping how should we get a tag from the mid 
> layer? Or maybe becuase there are some transport specific tag values 
> should we use someting else or keep it in the driver?

The only clean way I can see of doing it is to store the tags map in the
host structure.  That will pretty much require one host per transport
end point.  Is the iSCSI driver moving in that direction?

James

Re: [linux-iscsi-devel] Re: PATCH [3/5] qla2xxx: TCQ fixes

[Mike Christie]

James Bottomley wrote:

> On Mon, 2004-07-12 at 19:19, Mike Christie wrote:
> 
>>Depending on the API, we might be able to use it for the linux-iscsi 
>>driver. Today the driver uses scsi_activate_tcq(), but does not uses the 
>>tag numbers. Instead it uses its own initiator task tags values for 
>>session wide identifiers, becuase we need tag values for iscsi ops. For 
>>example if we need to send a ping how should we get a tag from the mid 
>>layer? Or maybe becuase there are some transport specific tag values 
>>should we use someting else or keep it in the driver?
> 
> 
> The only clean way I can see of doing it is to store the tags map in the
> host structure.  That will pretty much require one host per transport
> end point.  Is the iSCSI driver moving in that direction?

Unfortunately not, but it could be done. The iSCSI requirement is only 
that every iSCSI task has a tag which has the gaurantee of it being 
unique across a session, so if the host value was unique across all 
request_queues then it would still be safe to use.

The problem is that we need a tag for every iscsi task, so this includes 
untagged commands and logins (before there is a device/request_queue). 
Should we just keep it in the driver, or could the request queue have 
functions pointers so someone could override the block layer tag 
fucntions? What I am also thinking is that instead of passing the block 
layer request_queue (with the queue always allocating and setting the 
map) as an argument, it might be possible to make the tag functions 
some-new-map-structure based.

Bascially, the host could allocate this map and it could be initialized 
to have start_tag/end_tag functions that are possibly transport specific 
(iSCSI has some magic tag values, but I do not know about spi or fc). 
this way if it needed to allocate tags before a queue is present (for 
logins for iscsi's case) then it could do so (this also means the tag 
must have some new structure becuase for normal populate_msg usage the 
msg is dependant on request properties). Then when blk_init_queue is 
finally called it can inherit the existing map structure with its 
function pointers.

Or should we just ignore whatever SCSI or the block layer gives us and 
keep it in the driver?

Re: [linux-iscsi-devel] Re: PATCH [3/5] qla2xxx: TCQ fixes

[James Bottomley]

On Tue, 2004-07-13 at 14:04, Mike Christie wrote:
> Unfortunately not, but it could be done. The iSCSI requirement is only 
> that every iSCSI task has a tag which has the gaurantee of it being 
> unique across a session, so if the host value was unique across all 
> request_queues then it would still be safe to use.

Yes, I know...I've been over this several times with iscsi people.  The
basic point, I think, is that the driver could be so much tidier if
session state were stored in the host, and there were a direct
host<->end point correspondence.  You're certainly going to damage
scalability with a single host because of the way we use the host lock
to count per-host outstanding commands.

> The problem is that we need a tag for every iscsi task, so this includes 
> untagged commands and logins (before there is a device/request_queue). 
> Should we just keep it in the driver, or could the request queue have 
> functions pointers so someone could override the block layer tag 
> fucntions? What I am also thinking is that instead of passing the block 
> layer request_queue (with the queue always allocating and setting the 
> map) as an argument, it might be possible to make the tag functions 
> some-new-map-structure based.

That's the tag model.  Once TCQ is turned on, all tasks must be tagged,
even in SPI.

I don't quite understand why you need to override the block tag
functions, or why you want to operate without a request queue.  Even for
device probing in SCSI, we allocate a request queue, send the INQUIRY,
find there's nothing there and destroy the queue again.

The whole of SCSI is designed to operate with request structures backing
tasks; if you have to worry about this not being true, you'll generate a
maze of special cases in the driver.

> Bascially, the host could allocate this map and it could be initialized 
> to have start_tag/end_tag functions that are possibly transport specific 
> (iSCSI has some magic tag values, but I do not know about spi or fc). 
> this way if it needed to allocate tags before a queue is present (for 
> logins for iscsi's case) then it could do so (this also means the tag 
> must have some new structure becuase for normal populate_msg usage the 
> msg is dependant on request properties). Then when blk_init_queue is 
> finally called it can inherit the existing map structure with its 
> function pointers.

Tell me more about the magic tags.  We don't have a way to single them
out currently, but that could be easily added by just marking them
occupied in the bitmap.

James

Re: [linux-iscsi-devel] Re: PATCH [3/5] qla2xxx: TCQ fixes

[Mike Christie]

James Bottomley wrote:
> On Tue, 2004-07-13 at 14:04, Mike Christie wrote:
> 
>>Unfortunately not, but it could be done. The iSCSI requirement is only 
>>that every iSCSI task has a tag which has the gaurantee of it being 
>>unique across a session, so if the host value was unique across all 
>>request_queues then it would still be safe to use.
> 
> 
> Yes, I know...I've been over this several times with iscsi people.  The
> basic point, I think, is that the driver could be so much tidier if
> session state were stored in the host, and there were a direct
> host<->end point correspondence.  You're certainly going to damage
> scalability with a single host because of the way we use the host lock
> to count per-host outstanding commands.

I will put that on my todo list then.

> 
>>The problem is that we need a tag for every iscsi task, so this includes 
>>untagged commands and logins (before there is a device/request_queue). 
>>Should we just keep it in the driver, or could the request queue have 
>>functions pointers so someone could override the block layer tag 
>>fucntions? What I am also thinking is that instead of passing the block 
>>layer request_queue (with the queue always allocating and setting the 
>>map) as an argument, it might be possible to make the tag functions 
>>some-new-map-structure based.
> 
> 
> That's the tag model.  Once TCQ is turned on, all tasks must be tagged,
> even in SPI.
> 
> I don't quite understand why you need to override the block tag
> functions, or why you want to operate without a request queue.  Even for
> device probing in SCSI, we allocate a request queue, send the INQUIRY,
> find there's nothing there and destroy the queue again.
> 
> The whole of SCSI is designed to operate with request structures backing
> tasks; if you have to worry about this not being true, you'll generate a
> maze of special cases in the driver.

An excess of those already exist in the driver today :) :(

Maybe our driver initialization is the problem or maybe I am trying to do
too much. Here's how driver init is performed today in 2.6:

1. userspace does discovery, then passes this info down through an ioctl.
2. driver then uses this info to establish an iSCSI session. To accomplish this
we must do a login which requires a task tag. However, at this point there has not
been a request queue allocated and scsi-ml is not yet scanning.
3. session is established, so we scan away.

I think we could wait to establish a session, until a scan is actually initiated,
so we could do
1. from above.
2. initiate a scan.
3. in our slave_alloc if a session has not yet been established we could do this
there.

> 
>>Bascially, the host could allocate this map and it could be initialized 
>>to have start_tag/end_tag functions that are possibly transport specific 
>>(iSCSI has some magic tag values, but I do not know about spi or fc). 
>>this way if it needed to allocate tags before a queue is present (for 
>>logins for iscsi's case) then it could do so (this also means the tag 
>>must have some new structure becuase for normal populate_msg usage the 
>>msg is dependant on request properties). Then when blk_init_queue is 
>>finally called it can inherit the existing map structure with its 
>>function pointers.
> 
> 
> Tell me more about the magic tags.  We don't have a way to single them
> out currently, but that could be easily added by just marking them
> occupied in the bitmap.


For some operations the tag value can be 0xffffffff. This is just a reserved
value for special cases. For example if we send a NOP-out and set a valid tag
then this means we want a NOP-in back (a ping), but if the tag was set to
0xffffffff other side affects will result.


-- 
Mike Christie
mikenc@us.ibm.com

RE: PATCH [3/5] qla2xxx: TCQ fixes

[Andrew Vasquez]

On Monday, July 12, 2004 2:41 PM, James Bottomley wrote:
> 
> For the way the qla2xxx works, the correct tag usage would be to use
> per HBA tags (which were introduced for aic7xxx but never used by
> them, so I haven't actually created the mid-layer API), to populate
> your queue handle field with the tag and to dispense with your
> outstanding_cmnds[] array
>

Ok, I'll bite.  What did you have in mind? I've taken a quick peek
through the scsi code, the per-hba tag stuff must still be in your
head...

> and use scsi_find_tag() to get back the command from the handle. 
> 

Just a small comment, isn't the scsi_populate_tag_msg() function
quietly truncating the request->tag value?

Regards,
Andrew Vasquez

RE: PATCH [3/5] qla2xxx: TCQ fixes

[James Bottomley]

On Tue, 2004-07-13 at 12:24, Andrew Vasquez wrote:
> Ok, I'll bite.  What did you have in mind? I've taken a quick peek
> through the scsi code, the per-hba tag stuff must still be in your
> head...

Well, it exsits in the block layer (the ability to specify an existing
tag map to blk_queue_init_tags().  Jens is going to expose a bit more of
the tag map machinary to make that useful, then for host global tags,
we'll generate a single tag map and share it amongst all the queues.

> Just a small comment, isn't the scsi_populate_tag_msg() function
> quietly truncating the request->tag value?

Not for SPI, no.  However, we probably need a different API for the
sixteen bit tag values FC can use?

James