mbox series

[V5,00/16] use sg helper to operate scatterlist

Message ID 20190618013757.22401-1-ming.lei@redhat.com (mailing list archive)
Headers show
Series use sg helper to operate scatterlist | expand

Message

Ming Lei June 18, 2019, 1:37 a.m. UTC 
Hi,

Scsi MQ makes a large static allocation for the first scatter gather
list chunk for the driver to use.  This is a performance headache we'd
like to fix by reducing the size of the allocation to a 2 element
array.  Doing this will break the current guarantee that any driver
using SG_ALL doesn't need to use the scatterlist iterators and can get
away with directly dereferencing the array.  Thus we need to update all
drivers to use the scatterlist iterators and remove direct indexing of
the scatterlist array before reducing the initial scatterlist
allocation size in SCSI.

So convert drivers to use scatterlist helper.

There are two types of direct access on scatterlist in SCSI drivers:

1) operate on the scatterlist via scsi_sglist(scmd) directly, then one
local variable of 'struct scatterlist *' is involved.

2) scsi_sglist(scmd) is stored to cmd->SCp.buffer and the scatterlist is
used via cmd->SCp.buffer.

The following coccinelle semantic patch is developed for finding the
above two types of direct scatterlist uses:

	@@ struct scatterlist *p; @@
	(
	- ++p
	+ p = sg_next(p)
	|
	- p++
	+ p = sg_next(p)
	|
	- p = p + 1
	+ p = sg_next(p)
	|
	- p += 1
	+ p = sg_next(p)
	|
	- --p
	+ p = sg_non_exist_prev(p)
	|
	- p--
	+ p = sg_non_exist_prev(p)
	|
	- p = p - 1
	+ p = sg_non_exist_prev(p)
	|
	- p -= 1
	+ p = sg_non_exist_prev(p)
	)
	
	@@
	struct scatterlist *p;
	expression data != 0;
	@@
	- p[data]
	+ '!!!!!!use sg iterator helper!!!!!!'
	
	@@
	struct scatterlist[] p;
	expression data != 0;
	@@
	- p[data]
	+ '!!!!!!use sg iterator helper!!!!!!'
	
	
	@@ struct scsi_cmnd *scmd; @@
	(
	-	scmd->SCp.buffer++
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	++scmd->SCp.buffer
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer += 1
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer = scmd->SCp.buffer + 1
	+	scmd->SCp.buffer = sg_next(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer--
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	--scmd->SCp.buffer
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer -= 1
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	|
	-	scmd->SCp.buffer = scmd->SCp.buffer - 1
	+	scmd->SCp.buffer = sg_no_exit_prev(scmd->SCp.buffer)
	)
	
	@@
	struct scsi_cmnd *scmd;
	expression data != 0;
	@@
	- scmd->SCp.buffer[data]
	+ '!!!!!!use sg iterator helper!!!!!!'


The 1st 10 patches are for handling type #1, and the other 6 patches
for handling type #2, and all the 16 are found by the above coccinelle
semantic patch.

V5:
	- one patch style fix in 5/11
	- re-write convertion for 'staging: rtsx' as suggested by Christoph
	- fix another issue in aha152x by Finn, and change the author to
	  Finn now
	- add reviewed-by tag

V4:
	- fix building failure on pmcraid's conversion 
	- improve the coccinelle semantic patch to cover both two types of
	scatterlist direct use
	- driver 'staging: rtsx' is covered

V3:
	- update commit log and cover letter, most of words are from
	James Bottomley	

V2:
	- use coccinelle semantic patch for finding direct sgl uses from
	scsi command(9 drivers found)
	- run 'git grep -E "SCp.buffer"' to find direct sgl uses
	from SCp.buffer(6 drivers are found)



Finn Thain (2):
  scsi: aha152x: use sg helper to operate scatterlist
  NCR5380: Support chained sg lists

Ming Lei (14):
  scsi: vmw_pscsi: use sg helper to operate scatterlist
  scsi: advansys: use sg helper to operate scatterlist
  scsi: lpfc: use sg helper to operate scatterlist
  scsi: mvumi: use sg helper to operate scatterlist
  scsi: ipr: use sg helper to operate scatterlist
  scsi: pmcraid: use sg helper to operate scatterlist
  usb: image: microtek: use sg helper to operate scatterlist
  staging: unisys: visorhba: use sg helper to operate scatterlist
  staging: rtsx: use sg helper to operate scatterlist
  s390: zfcp_fc: use sg helper to operate scatterlist
  scsi: imm: use sg helper to operate scatterlist
  scsi: pcmcia: nsp_cs: use sg helper to operate scatterlist
  scsi: ppa: use sg helper to operate scatterlist
  scsi: wd33c93: use sg helper to operate scatterlist

 drivers/s390/scsi/zfcp_fc.c                   |  4 +-
 drivers/scsi/NCR5380.c                        | 41 ++++++++---------
 drivers/scsi/advansys.c                       |  2 +-
 drivers/scsi/aha152x.c                        | 46 +++++++++----------
 drivers/scsi/imm.c                            |  2 +-
 drivers/scsi/ipr.c                            | 29 ++++++------
 drivers/scsi/lpfc/lpfc_nvmet.c                |  3 +-
 drivers/scsi/mvumi.c                          |  9 ++--
 drivers/scsi/pcmcia/nsp_cs.c                  |  4 +-
 drivers/scsi/pmcraid.c                        | 14 +++---
 drivers/scsi/ppa.c                            |  2 +-
 drivers/scsi/vmw_pvscsi.c                     |  2 +-
 drivers/scsi/wd33c93.c                        |  2 +-
 drivers/staging/rts5208/rtsx_transport.c      | 32 ++++++-------
 drivers/staging/rts5208/rtsx_transport.h      |  2 +-
 drivers/staging/rts5208/spi.c                 | 14 +++---
 .../staging/unisys/visorhba/visorhba_main.c   |  9 ++--
 drivers/usb/image/microtek.c                  | 20 ++++----
 drivers/usb/image/microtek.h                  |  2 +-
 19 files changed, 115 insertions(+), 124 deletions(-)

Comments

Martin K. Petersen June 19, 2019, 12:29 a.m. UTC | #1 
Hi Ming,

> So convert drivers to use scatterlist helper.

I applied this series with a bunch of edits and clarifying comments. It
was quite the nightmare to rebase 5.3/scsi-queue to satisfy the ordering
requirements, locate the scattered fixes, tweak tags, etc. Hope I got
everything right.

It was almost at the point where I gave up and postponed everything to
5.4. We're at -rc5 -- it's not exactly a good time to rebase the entire
tree. However, I know the static mq allocations are causing headaches
for several distro vendors so I begrudgingly bit the bullet.

I would have liked to see more reviews from the official driver
maintainers but at the same time I didn't want to wait any longer giving
this some soak time in linux-next.

Thanks for your work on getting all the drivers fixed up! Just goes to
show how large the fallout can be from a relatively innocuous core
change.

Oh, and I held back the rtsx patch due to lack of reviews. But since
that driver is in staging I'm not too worried about it. Hope we can get
the fix for that reviewed and merged soon.
Bart Van Assche June 19, 2019, 7:43 p.m. UTC | #2 
On 6/18/19 5:29 PM, Martin K. Petersen wrote:
> I applied this series with a bunch of edits and clarifying comments. It
> was quite the nightmare to rebase 5.3/scsi-queue to satisfy the ordering
> requirements, locate the scattered fixes, tweak tags, etc. Hope I got
> everything right.

Hi Martin,

Do you perhaps plan to push out these patches at a later time? It seems 
like that branch has not been updated recently:

$ git show --format=fuller mkp-scsi/5.3/scsi-queue | head -n7
commit f3e88ad00f58e9a05986be3028b2ed8654c601c9
Author:     Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
AuthorDate: Fri May 31 08:14:43 2019 -0400
Commit:     Martin K. Petersen <martin.petersen@oracle.com>
CommitDate: Fri Jun 7 10:17:06 2019 -0400

     scsi: mpt3sas: Update driver version to 29.100.00.00

Thanks,

Bart.
Martin K. Petersen June 19, 2019, 7:55 p.m. UTC | #3 
Bart,

> Do you perhaps plan to push out these patches at a later time? It
> seems like that branch has not been updated recently:

I had a test failure on this end, that's why I didn't push. Appears to
be hardware-related, though. Still looking into it.
Ming Lei June 24, 2019, 12:40 p.m. UTC | #4 
Hi Martin,

On Thu, Jun 20, 2019 at 3:57 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Bart,
>
> > Do you perhaps plan to push out these patches at a later time? It
> > seems like that branch has not been updated recently:
>
> I had a test failure on this end, that's why I didn't push. Appears to
> be hardware-related, though. Still looking into it.

Today I found the whole patchset disappears from 5.3/scsi-queue, seems
something is wrong?


Thanks,
Ming Lei
Martin K. Petersen June 24, 2019, 12:54 p.m. UTC | #5 
Ming,

> Today I found the whole patchset disappears from 5.3/scsi-queue, seems
> something is wrong?

Your changes are in 5.3/scsi-sg. I put them in a separate branch to
avoid having to rebase the rest of the queue in case we find more
issues.

My for-next branch is based on 5.3/scsi-queue and 5.3/scsi-sg.