[patch] fix unnecessary activation of blk tag queue for qla2x00

[patch] fix unnecessary activation of blk tag queue for qla2x00

[Chen, Kenneth W]

Regarding to discussion on LKML:
http://marc.theaimsgroup.com/?t=112865351000001&r=1&w=2

It turns out that qla2x00 driver unnecessarily activated generic
blk tag queue without actually use any of the tag maintained by
the block layer.  I suggest we turn that off until there is a
real consumer of the tag queuing in the qla driver. Please apply.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>

--- ./drivers/scsi/qla2xxx/qla_os.c.orig	2005-10-06
23:24:32.547238409 -0700
+++ ./drivers/scsi/qla2xxx/qla_os.c	2005-10-06 23:30:44.004265109
-0700
@@ -1101,11 +1101,7 @@ qla2xxx_slave_configure(struct scsi_devi
 	scsi_qla_host_t *ha = to_qla_host(sdev->host);
 	struct fc_rport *rport = starget_to_rport(sdev->sdev_target);
 
-	if (sdev->tagged_supported)
-		scsi_activate_tcq(sdev, 32);
-	else
-		scsi_deactivate_tcq(sdev, 32);
-
+	scsi_adjust_queue_depth(sdev, scsi_get_tag_type(sdev), 32);
 	rport->dev_loss_tmo = ha->port_down_retry_count + 5;
 
 	return 0;
@@ -1127,13 +1123,9 @@ qla2x00_change_queue_depth(struct scsi_d
 static int
 qla2x00_change_queue_type(struct scsi_device *sdev, int tag_type)
 {
-	if (sdev->tagged_supported) {
+	if (sdev->tagged_supported)
 		scsi_set_tag_type(sdev, tag_type);
-		if (tag_type)
-			scsi_activate_tcq(sdev, sdev->queue_depth);
-		else
-			scsi_deactivate_tcq(sdev, sdev->queue_depth);
-	} else
+	else
 		tag_type = 0;
 
 	return tag_type;

Re: [patch] fix unnecessary activation of blk tag queue for qla2x00

[Andrew Vasquez]

On Fri, 07 Oct 2005, Chen, Kenneth W wrote:

> Regarding to discussion on LKML:
> http://marc.theaimsgroup.com/?t=112865351000001&r=1&w=2

Ok, I had expected something like this to show on ls after the
discussion on lk.

> It turns out that qla2x00 driver unnecessarily activated generic
> blk tag queue without actually use any of the tag maintained by
> the block layer.  I suggest we turn that off until there is a
> real consumer of the tag queuing in the qla driver. Please apply.
> 
> Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> 

ACK.

Thanks,
Andrew Vasquez

Re: [patch] fix unnecessary activation of blk tag queue for qla2x00

[Patrick Mansfield]

On Fri, Oct 07, 2005 at 10:03:56AM -0700, Andrew Vasquez wrote:
> On Fri, 07 Oct 2005, Chen, Kenneth W wrote:
> 
> > Regarding to discussion on LKML:
> > http://marc.theaimsgroup.com/?t=112865351000001&r=1&w=2
> 
> Ok, I had expected something like this to show on ls after the
> discussion on lk.
> 
> > It turns out that qla2x00 driver unnecessarily activated generic
> > blk tag queue without actually use any of the tag maintained by
> > the block layer.  I suggest we turn that off until there is a
> > real consumer of the tag queuing in the qla driver. Please apply.
> > 
> > Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> > 
> 
> ACK.

And then also remove scsi_populate_tag_msg() calls also in qla2xxx, as
that becomes dead code?

And then we can't pass down the task attribute. That does not matter now
as long as there are still no blk barriers for scsi (with can_queue or
queue_depth > 0, and some other scsi_host value set).

-- Patrick Mansfield

RE: [patch] fix unnecessary activation of blk tag queue for qla2x00

[Chen, Kenneth W]

Patrick Mansfield wrote on Wednesday, October 12, 2005 1:00 PM
> > > It turns out that qla2x00 driver unnecessarily activated
> > > generic blk tag queue without actually use any of the tag
> > > maintained by the block layer.  I suggest we turn that off
> > > until there is a real consumer of the tag queuing in the
> > > qla driver. Please apply.
> > > 
> > > Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> > 
> > ACK.
> 
> And then also remove scsi_populate_tag_msg() calls also in qla2xxx,
> as that becomes dead code?

Yeah, that would indeed be a very good thing to do.  It reminds me
a while back, I was investigating cache miss profile and we
consistently see cache misses on tag[2] variable defined in
qla2x00_start_scsi (used by scsi_populate_tag_msg).  Removing it
would be a very good thing for driver performance.

Should I generate the patch?

- Ken

Re: [patch] fix unnecessary activation of blk tag queue for qla2x00

[Andrew Vasquez]

On Wed, 12 Oct 2005, Chen, Kenneth W wrote:

> Patrick Mansfield wrote on Wednesday, October 12, 2005 1:00 PM
> > > > It turns out that qla2x00 driver unnecessarily activated
> > > > generic blk tag queue without actually use any of the tag
> > > > maintained by the block layer.  I suggest we turn that off
> > > > until there is a real consumer of the tag queuing in the
> > > > qla driver. Please apply.
> > > > 
> > > > Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> > > 
> > > ACK.
> > 
> > And then also remove scsi_populate_tag_msg() calls also in qla2xxx,
> > as that becomes dead code?
> 
> Yeah, that would indeed be a very good thing to do.  It reminds me
> a while back, I was investigating cache miss profile and we
> consistently see cache misses on tag[2] variable defined in
> qla2x00_start_scsi (used by scsi_populate_tag_msg).  Removing it
> would be a very good thing for driver performance.
> 
> Should I generate the patch?

Drop scsi_populate_tag_msg() interrogation.

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

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 37f82e2..f5902ec 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -401,18 +401,6 @@ qla2x00_start_scsi(srb_t *sp)
 
 	/* Update tagged queuing modifier */
 	cmd_pkt->control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG);
-	if (scsi_populate_tag_msg(cmd, tag)) {
-		switch (tag[0]) {
-		case MSG_HEAD_TAG:
-			cmd_pkt->control_flags =
-			    __constant_cpu_to_le16(CF_HEAD_TAG);
-			break;
-		case MSG_ORDERED_TAG:
-			cmd_pkt->control_flags =
-			    __constant_cpu_to_le16(CF_ORDERED_TAG);
-			break;
-		}
-	}
 
 	/* Load SCSI command packet. */
 	memcpy(cmd_pkt->scsi_cdb, cmd->cmnd, cmd->cmd_len);
@@ -824,6 +812,7 @@ qla24xx_start_scsi(srb_t *sp)
 	cmd_pkt->handle = handle;
 
 	/* Zero out remaining portion of packet. */
+	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */
 	clr_ptr = (uint32_t *)cmd_pkt + 2;
 	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
 	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
@@ -837,18 +826,6 @@ qla24xx_start_scsi(srb_t *sp)
 	cmd_pkt->lun[1] = LSB(sp->cmd->device->lun);
 	cmd_pkt->lun[2] = MSB(sp->cmd->device->lun);
 
-	/* Update tagged queuing modifier -- default is TSK_SIMPLE (0). */
-	if (scsi_populate_tag_msg(cmd, tag)) {
-		switch (tag[0]) {
-		case MSG_HEAD_TAG:
-			cmd_pkt->task = TSK_HEAD_OF_QUEUE;
-			break;
-		case MSG_ORDERED_TAG:
-			cmd_pkt->task = TSK_ORDERED;
-			break;
-		}
-	}
-
 	/* Load SCSI command packet. */
 	memcpy(cmd_pkt->fcp_cdb, cmd->cmnd, cmd->cmd_len);
 	host_to_fcp_swap(cmd_pkt->fcp_cdb, sizeof(cmd_pkt->fcp_cdb));

RE: [patch] fix unnecessary activation of blk tag queue for qla2x00

[Chen, Kenneth W]

Andrew Vasquez wrote on Wednesday, October 12, 2005 2:10 PM
> On Wed, 12 Oct 2005, Chen, Kenneth W wrote:
> > Yeah, that would indeed be a very good thing to do.  It reminds me
> > a while back, I was investigating cache miss profile and we
> > consistently see cache misses on tag[2] variable defined in
> > qla2x00_start_scsi (used by scsi_populate_tag_msg).  Removing it
> > would be a very good thing for driver performance.
> > 
> > Should I generate the patch?
> 
> Drop scsi_populate_tag_msg() interrogation.

Great! Got a couple "unused variable" compiler warning.  You forgot
to delete the variable declaration in a hurry ;-)


--- ./drivers/scsi/qla2xxx/qla_iocb.c.orig	2005-10-12 14:17:31.544219754 -0700
+++ ./drivers/scsi/qla2xxx/qla_iocb.c	2005-10-12 14:18:08.124297431 -0700
@@ -316,7 +316,6 @@ qla2x00_start_scsi(srb_t *sp)
 	uint16_t	req_cnt;
 	uint16_t	tot_dsds;
 	struct device_reg_2xxx __iomem *reg;
-	char		tag[2];
 
 	/* Setup device pointers. */
 	ret = 0;
@@ -737,7 +736,6 @@ qla24xx_start_scsi(srb_t *sp)
 	uint16_t	req_cnt;
 	uint16_t	tot_dsds;
 	struct device_reg_24xx __iomem *reg;
-	char		tag[2];
 
 	/* Setup device pointers. */
 	ret = 0;

Re: [patch] fix unnecessary activation of blk tag queue for qla2x00

['Andrew Vasquez']

On Wed, 12 Oct 2005, Chen, Kenneth W wrote:

> Andrew Vasquez wrote on Wednesday, October 12, 2005 2:10 PM
> > On Wed, 12 Oct 2005, Chen, Kenneth W wrote:
> > > Yeah, that would indeed be a very good thing to do.  It reminds me
> > > a while back, I was investigating cache miss profile and we
> > > consistently see cache misses on tag[2] variable defined in
> > > qla2x00_start_scsi (used by scsi_populate_tag_msg).  Removing it
> > > would be a very good thing for driver performance.
> > > 
> > > Should I generate the patch?
> > 
> > Drop scsi_populate_tag_msg() interrogation.
> 
> Great! Got a couple "unused variable" compiler warning.  You forgot
> to delete the variable declaration in a hurry ;-)

I deserved that...

Updated patch...follows...

---

Drop scsi_populate_tag_msg() interrogation.

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

---

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 37f82e2..4b57973 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -316,7 +316,6 @@ qla2x00_start_scsi(srb_t *sp)
 	uint16_t	req_cnt;
 	uint16_t	tot_dsds;
 	struct device_reg_2xxx __iomem *reg;
-	char		tag[2];
 
 	/* Setup device pointers. */
 	ret = 0;
@@ -401,18 +400,6 @@ qla2x00_start_scsi(srb_t *sp)
 
 	/* Update tagged queuing modifier */
 	cmd_pkt->control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG);
-	if (scsi_populate_tag_msg(cmd, tag)) {
-		switch (tag[0]) {
-		case MSG_HEAD_TAG:
-			cmd_pkt->control_flags =
-			    __constant_cpu_to_le16(CF_HEAD_TAG);
-			break;
-		case MSG_ORDERED_TAG:
-			cmd_pkt->control_flags =
-			    __constant_cpu_to_le16(CF_ORDERED_TAG);
-			break;
-		}
-	}
 
 	/* Load SCSI command packet. */
 	memcpy(cmd_pkt->scsi_cdb, cmd->cmnd, cmd->cmd_len);
@@ -749,7 +736,6 @@ qla24xx_start_scsi(srb_t *sp)
 	uint16_t	req_cnt;
 	uint16_t	tot_dsds;
 	struct device_reg_24xx __iomem *reg;
-	char		tag[2];
 
 	/* Setup device pointers. */
 	ret = 0;
@@ -824,6 +810,7 @@ qla24xx_start_scsi(srb_t *sp)
 	cmd_pkt->handle = handle;
 
 	/* Zero out remaining portion of packet. */
+	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */
 	clr_ptr = (uint32_t *)cmd_pkt + 2;
 	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
 	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
@@ -837,18 +824,6 @@ qla24xx_start_scsi(srb_t *sp)
 	cmd_pkt->lun[1] = LSB(sp->cmd->device->lun);
 	cmd_pkt->lun[2] = MSB(sp->cmd->device->lun);
 
-	/* Update tagged queuing modifier -- default is TSK_SIMPLE (0). */
-	if (scsi_populate_tag_msg(cmd, tag)) {
-		switch (tag[0]) {
-		case MSG_HEAD_TAG:
-			cmd_pkt->task = TSK_HEAD_OF_QUEUE;
-			break;
-		case MSG_ORDERED_TAG:
-			cmd_pkt->task = TSK_ORDERED;
-			break;
-		}
-	}
-
 	/* Load SCSI command packet. */
 	memcpy(cmd_pkt->fcp_cdb, cmd->cmnd, cmd->cmd_len);
 	host_to_fcp_swap(cmd_pkt->fcp_cdb, sizeof(cmd_pkt->fcp_cdb));