mbox series

[v2,0/6] scsi: libsas: Use request tag in more drivers

Message ID 1664528184-162714-1-git-send-email-john.garry@huawei.com
Headers show
Series scsi: libsas: Use request tag in more drivers | expand

Message

John Garry Sept. 30, 2022, 8:56 a.m. UTC 
Currently hisi_sas is the only libsas driver which uses the request tag
for per-HW IO tag.

All other libsas drivers manage the tags internally. Tag management in
pm8001 and mvsas is currently using a simple bitmap, so use the request
tag when available there. With this change we still need to manage tags
for libsas "internal" commands, like SMP commands, and any other
private commands so reserve some tags for this:
- For pm8001 I went with pre-existing and unused PM8001_RESERVE_SLOT size.
  The value is 8, which should be enough. It is greater than mvsas, below,
  but this driver sends a lot of other private commands to HW.
- For mvsas I went with 4, which still should be enough.

isci and aic9xx have elaborate tag alloc schemes, so I'm not going to
bother changing them, especially since I have no HW to test with.

Helper sas_task_find_rq() is added to get the request and associated tag
per sas_task when it is available.

This series looks not to conflict with
https://lore.kernel.org/linux-scsi/20220928070130.3657183-1-yanaijie@huawei.com/T/#mefdcb7b63b4e6ebc8b77a689b3923571ab3d33ab

Based on https://lore.kernel.org/linux-scsi/1664262298-239952-1-git-send-email-john.garry@huawei.com/T/#t

Differences to v1:
- Rework sas_task_find_rq()
- Rename tags->rsvd and remove tag size struct arg for both mvsas and
  pm8001
- Decrement can_queue for mvsas
- Use sas_task_find_rq() in pm80xx_chip_get_q_index()
- Add Damien's tags (thanks)

Igor Pylypiv (1):
  scsi: pm8001: Remove pm8001_tag_init()

John Garry (5):
  scsi: libsas: Add sas_task_find_rq()
  scsi: hisi_sas: Use sas_task_find_rq()
  scsi: pm8001: Use sas_task_find_rq() for tagging
  scsi: mvsas: Delete mvs_tag_init()
  scsi: mvsas: Use sas_task_find_rq() for tagging

 drivers/scsi/hisi_sas/hisi_sas_main.c | 26 +++++-----------
 drivers/scsi/mvsas/mv_defs.h          |  1 +
 drivers/scsi/mvsas/mv_init.c          | 11 +++----
 drivers/scsi/mvsas/mv_sas.c           | 45 +++++++++++++++------------
 drivers/scsi/mvsas/mv_sas.h           |  8 +----
 drivers/scsi/pm8001/pm8001_init.c     | 14 +++------
 drivers/scsi/pm8001/pm8001_sas.c      | 23 +++++++-------
 drivers/scsi/pm8001/pm8001_sas.h      | 12 ++++---
 drivers/scsi/pm8001/pm80xx_hwi.c      | 17 ++--------
 include/scsi/libsas.h                 | 18 +++++++++++
 10 files changed, 85 insertions(+), 90 deletions(-)

Comments

Jason Yan Sept. 30, 2022, 9:22 a.m. UTC | #1 
On 2022/9/30 16:56, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry<john.garry@huawei.com>
> ---
>   include/scsi/libsas.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

Reviewed-by: Jason Yan <yanaijie@huawei.com>
John Garry Sept. 30, 2022, 11:05 a.m. UTC | #2 
On 30/09/2022 11:20, John Garry wrote:
> BTW, I do notice that we still have this global lock in delivery path 
> which should be removed at some stage:
> > int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
> {
>      ...
> 
>      spin_lock_irqsave(&mvi->lock, flags);
>      rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
>      ...
>      spin_unlock_irqrestore(&mvi->lock, flags);
> }
> 

oops... that's mvsas. But pm8001 does still use a big lock (which we 
should get rid off):

int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
{
	...
	pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n");

	spin_lock_irqsave(&pm8001_ha->lock, flags);


Thanks,
John

> That really will affect performance...
Jinpu Wang Oct. 4, 2022, 4:51 a.m. UTC | #3 
On Fri, Sep 30, 2022 at 1:05 PM John Garry <john.garry@huawei.com> wrote:
>
> On 30/09/2022 11:20, John Garry wrote:
> > BTW, I do notice that we still have this global lock in delivery path
> > which should be removed at some stage:
> > > int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
> > {
> >      ...
> >
> >      spin_lock_irqsave(&mvi->lock, flags);
> >      rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
> >      ...
> >      spin_unlock_irqrestore(&mvi->lock, flags);
> > }
> >
>
> oops... that's mvsas. But pm8001 does still use a big lock (which we
> should get rid off):
Yes, would be great to get rid of the per ha lock.
>
> int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> {
>         ...
>         pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n");
>
>         spin_lock_irqsave(&pm8001_ha->lock, flags);
>
>
> Thanks,
> John
>
> > That really will affect performance...
>