Skip to contentSkip to conversation

uas host lock removal conversion

RFC In review Matthew Wilcox 6 participants 8 messages Dec 16, 2010 – Jan 4, 2011

Subsystems (4)
SCSI USB USB mass storage driver USB attached SCSI
lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

RFC 1/1 uas host lock removal conversion

I thought I'd take a crack at removing the host lock from uas. I think I can now switch to using GFP_NOIO (since interrupts are enabled by scsi_request_fn()), and I have to use _irq spinlocks to protect the list that is modified from interrupt context (same reason :-).

Comments?

drivers/scsi/scsi_lib.c +1 -5
index 5b6bbae..9e881cd 100644
@@ -1443,7 +1443,7 @@ static void scsi_softirq_done(struct request *rq)
  *
  * Returns:     Nothing
  *
- * Lock status: IO request lock assumed to be held when called.
+ * Lock status: request queue lock held, interrupts disabled
  */
 static void scsi_request_fn(struct request_queue *q)
 {

@@ -1530,10 +1530,6 @@ static void scsi_request_fn(struct request_queue *q)
 		scsi_target(sdev)->target_busy++;
 		shost->host_busy++;
 
-		/*
-		 * XXX(hch): This is rather suboptimal, scsi_dispatch_cmd will
-		 *		take the lock again.
-		 */
 		spin_unlock_irq(shost->host_lock);
 
 		/*
drivers/usb/storage/uas.c +4 -9
index 23f0dd9..a2cff4b 100644
@@ -433,8 +433,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	return 0;
 }
 
-static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
-					void (*done)(struct scsi_cmnd *))
+static int uas_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 {
 	struct scsi_device *sdev = cmnd->device;
 	struct uas_dev_info *devinfo = sdev->hostdata;

@@ -453,8 +452,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		cmdinfo->stream = 1;
 	}
 
-	cmnd->scsi_done = done;
-
 	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
 

@@ -475,24 +472,22 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		cmdinfo->stream = 0;
 	}
 
-	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
+	err = uas_submit_urbs(cmnd, devinfo, GFP_NOIO);
 	if (err) {
 		/* If we did nothing, give up now */
 		if (cmdinfo->state & SUBMIT_STATUS_URB) {
 			usb_free_urb(cmdinfo->status_urb);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
-		spin_lock(&uas_work_lock);
+		spin_lock_irq(&uas_work_lock);
 		list_add_tail(&cmdinfo->list, &uas_work_list);
-		spin_unlock(&uas_work_lock);
+		spin_unlock_irq(&uas_work_lock);
 		schedule_work(&uas_work);
 	}
 
 	return 0;
 }
 
-static DEF_SCSI_QCMD(uas_queuecommand)
-
 static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 {
 	struct scsi_device *sdev = cmnd->device;
Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#1 lore raw

Land iscsi iser performance patches to upstream

Hi,

Could anyone advise if there is any plan to push these patches to land the upstream?

Patch 1/3
http://www.mail-archive.com/[email protected]/msg14533.html
Patch 2/3
http://www.mail-archive.com/[email protected]/msg14381.html
Patch 3/3
http://www.mail-archive.com/[email protected]/msg14398.html

In particular patch 2 and 3 are matching the iscsi_tcp.c values and have improved the performance that we got. It will be beneficial to include them into the vanilla kernel.

Thanks!
Zhiqi

#2 lore raw

Re: Land iscsi iser performance patches to upstream

Quoted 13 lines · Zhiqi Tao

Hi,

Could anyone advise if there is any plan to push these patches to land the upstream?

Patch 1/3
http://www.mail-archive.com/[email protected]/msg14533.html
Patch 2/3
http://www.mail-archive.com/[email protected]/msg14381.html
Patch 3/3
http://www.mail-archive.com/[email protected]/msg14398.html

Quoted 4 lines

In particular patch 2 and 3 are matching the iscsi_tcp.c values and have improved the performance that we got. It will be beneficial to include them into the vanilla kernel.

Hey Or,

These patches look ok except for #1. Did you guys have some weird restrictions? Or did you have some weird restrictions but the iser driver handles it (see iser_data_buf_aligned_len related code). I think instead of 0 you want to set the dma alignment to 4K?

#3 lore raw

Re: Land iscsi iser performance patches to upstream

Quoted 1 line · Mike Christie

These patches look ok except for #1. Did you guys have some weird restrictions? Or did you have some weird restrictions but the iser driver handles it (see iser_data_buf_aligned_len related code). I think instead of 0 you want to set the dma alignment to 4K?

Hi Mike,

Looks like I missed this ~3 years old patch-set, will look into that and let you know.

Or.

#4 lore raw

Re: Land iscsi iser performance patches to upstream

In reply to Mike Christie
Quoted 1 line · Zhiqi Tao
Quoted 1 line

In particular patch 2 and 3 are matching the iscsi_tcp.c values and have improved the performance that we got. It will be beneficial to include them into the vanilla kernel.

When saying "patch 2 and 3 have improved the performance that we got" - did you mean the performance with iser or with iscsi-tcp? if with iser, can you spare few words on the test and what was the performance improvement?

These patches allow to support e.g 1MB size commands, vs. the 512KB limit of the upstream bits, however, we manage to see peak performance with much IO sizes (e.g 128KB, 256KB), so I wasn't sure what these patches could buy you performance wise.

Or.

#5 lore raw
In reply to Matthew Wilcox
Quoted 5 lines · Matthew Wilcox

I thought I'd take a crack at removing the host lock from uas. I think I can now switch to using GFP_NOIO (since interrupts are enabled by scsi_request_fn()), and I have to use _irq spinlocks to protect the list that is modified from interrupt context (same reason :-).

->qeuecommand can be called from software interrupt context, so you'll need to stick to GFP_ATOMIC.

author
#6 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 5 lines · Matthew Wilcox

I thought I'd take a crack at removing the host lock from uas. I think I can now switch to using GFP_NOIO (since interrupts are enabled by scsi_request_fn()), and I have to use _irq spinlocks to protect the list that is modified from interrupt context (same reason :-).

->qeuecommand can be called from software interrupt context, so you'll need to stick to GFP_ATOMIC.

Ah, I couldn't find a path that did that. Could you point it out to me? I'll send a patch to update that documentation.

Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#7 lore raw
Quoted 8 lines · Matthew Wilcox
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Matthew Wilcox

I thought I'd take a crack at removing the host lock from uas. I think I can now switch to using GFP_NOIO (since interrupts are enabled by scsi_request_fn()), and I have to use _irq spinlocks to protect the list that is modified from interrupt context (same reason :-).

Quoted 5 lines
Quoted 2 lines

->qeuecommand can be called from software interrupt context, so you'll need to stick to GFP_ATOMIC.

Ah, I couldn't find a path that did that. Could you point it out to me? I'll send a patch to update that documentation.

Well, the documentation (scsi_mid_low_api.txt) says this. The reason is the blk_run_queue in softirq context.

James