Re: [PATCH] libsas: remove task_collector mode

[Hannes Reinecke <hare@suse.de>]

On 11/24/2014 09:41 AM, Christoph Hellwig wrote:
> The task_collector mode (or "latency_injector", (C) Dan Willians) is an
> optional I/O path in libsas that queues up scsi commands instead of
> directly sending it to the hardware.  It generall increases latencies
> to in the optiomal case slightly reduce mmio traffic to the hardware.
> 
> Only the obsolete aic94xx driver and the mvsas driver allowed to use
> it without recompiling the kernel, and most drivers didn't support it
> at all.
> 
> Remove the giant blob of code to allow better optimizations for scsi-mq
> in the future.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/scsi/libsas.txt       |  82 +----------------
>  drivers/scsi/aic94xx/aic94xx.h      |   2 +-
>  drivers/scsi/aic94xx/aic94xx_hwi.c  |   3 +-
>  drivers/scsi/aic94xx/aic94xx_init.c |  11 ---
>  drivers/scsi/aic94xx/aic94xx_task.c |  13 ++-
>  drivers/scsi/isci/init.c            |   2 -
>  drivers/scsi/isci/task.c            | 147 ++++++++++++++----------------
>  drivers/scsi/isci/task.h            |   1 -
>  drivers/scsi/libsas/sas_ata.c       |   9 +-
>  drivers/scsi/libsas/sas_expander.c  |   2 +-
>  drivers/scsi/libsas/sas_init.c      |  21 -----
>  drivers/scsi/libsas/sas_internal.h  |   2 -
>  drivers/scsi/libsas/sas_scsi_host.c | 176 +-----------------------------------
>  drivers/scsi/mvsas/mv_init.c        |  22 -----
>  drivers/scsi/mvsas/mv_sas.c         | 109 +---------------------
>  drivers/scsi/mvsas/mv_sas.h         |  10 +-
>  drivers/scsi/pm8001/pm8001_init.c   |   2 -
>  drivers/scsi/pm8001/pm8001_sas.c    |  22 +----
>  drivers/scsi/pm8001/pm8001_sas.h    |   3 +-
>  include/scsi/libsas.h               |  14 +--
>  20 files changed, 97 insertions(+), 556 deletions(-)
> 
> diff --git a/Documentation/scsi/libsas.txt b/Documentation/scsi/libsas.txt
> index 3cc9c78..8cac649 100644
> --- a/Documentation/scsi/libsas.txt
> +++ b/Documentation/scsi/libsas.txt
> @@ -226,9 +226,6 @@ static int register_sas_ha(struct my_sas_ha *my_ha)
>  	my_ha->sas_ha.lldd_dev_found = my_dev_found;
>  	my_ha->sas_ha.lldd_dev_gone = my_dev_gone;
>  
> -	my_ha->sas_ha.lldd_max_execute_num = lldd_max_execute_num; (1)
> -
> -	my_ha->sas_ha.lldd_queue_size = ha_can_queue;
>  	my_ha->sas_ha.lldd_execute_task = my_execute_task;
>  
>  	my_ha->sas_ha.lldd_abort_task     = my_abort_task;
> @@ -247,28 +244,6 @@ static int register_sas_ha(struct my_sas_ha *my_ha)
>  	return sas_register_ha(&my_ha->sas_ha);
>  }
>  
> -(1) This is normally a LLDD parameter, something of the
> -lines of a task collector.  What it tells the SAS Layer is
> -whether the SAS layer should run in Direct Mode (default:
> -value 0 or 1) or Task Collector Mode (value greater than 1).
> -
> -In Direct Mode, the SAS Layer calls Execute Task as soon as
> -it has a command to send to the SDS, _and_ this is a single
> -command, i.e. not linked.
> -
> -Some hardware (e.g. aic94xx) has the capability to DMA more
> -than one task at a time (interrupt) from host memory.  Task
> -Collector Mode is an optional feature for HAs which support
> -this in their hardware.  (Again, it is completely optional
> -even if your hardware supports it.)
> -
> -In Task Collector Mode, the SAS Layer would do _natural_
> -coalescing of tasks and at the appropriate moment it would
> -call your driver to DMA more than one task in a single HA
> -interrupt. DMBS may want to use this by insmod/modprobe
> -setting the lldd_max_execute_num to something greater than
> -1.
> -
>  (2) SAS 1.1 does not define I_T Nexus Reset TMF.
>  
>  Events
> @@ -325,71 +300,22 @@ PHYE_SPINUP_HOLD -- SATA is present, COMWAKE not sent.
>  
>  The Execute Command SCSI RPC:
>  
> -	int (*lldd_execute_task)(struct sas_task *, int num,
> -				 unsigned long gfp_flags);
> +	int (*lldd_execute_task)(struct sas_task *, gfp_t gfp_flags);
>  
> -Used to queue a task to the SAS LLDD.  @task is the tasks to
> -be executed.  @num should be the number of tasks being
> -queued at this function call (they are linked listed via
> -task::list), @gfp_mask should be the gfp_mask defining the
> -context of the caller.
> +Used to queue a task to the SAS LLDD.  @task is the task to be executed.
> +@gfp_mask is the gfp_mask defining the context of the caller.
>  
>  This function should implement the Execute Command SCSI RPC,
> -or if you're sending a SCSI Task as linked commands, you
> -should also use this function.
>  
> -That is, when lldd_execute_task() is called, the command(s)
> +That is, when lldd_execute_task() is called, the command
>  go out on the transport *immediately*.  There is *no*
>  queuing of any sort and at any level in a SAS LLDD.
>  
> -The use of task::list is two-fold, one for linked commands,
> -the other discussed below.
> -
> -It is possible to queue up more than one task at a time, by
> -initializing the list element of struct sas_task, and
> -passing the number of tasks enlisted in this manner in num.
> -
>  Returns: -SAS_QUEUE_FULL, -ENOMEM, nothing was queued;
>  	 0, the task(s) were queued.
>  
> -If you want to pass num > 1, then either
> -A) you're the only caller of this function and keep track
> -   of what you've queued to the LLDD, or
> -B) you know what you're doing and have a strategy of
> -   retrying.
> -
> -As opposed to queuing one task at a time (function call),
> -batch queuing of tasks, by having num > 1, greatly
> -simplifies LLDD code, sequencer code, and _hardware design_,
> -and has some performance advantages in certain situations
> -(DBMS).
> -
> -The LLDD advertises if it can take more than one command at
> -a time at lldd_execute_task(), by setting the
> -lldd_max_execute_num parameter (controlled by "collector"
> -module parameter in aic94xx SAS LLDD).
> -
> -You should leave this to the default 1, unless you know what
> -you're doing.
> -
> -This is a function of the LLDD, to which the SAS layer can
> -cater to.
> -
> -int lldd_queue_size
> -	The host adapter's queue size.  This is the maximum
> -number of commands the lldd can have pending to domain
> -devices on behalf of all upper layers submitting through
> -lldd_execute_task().
> -
> -You really want to set this to something (much) larger than
> -1.
> -
> -This _really_ has absolutely nothing to do with queuing.
> -There is no queuing in SAS LLDDs.
> -
>  struct sas_task {
>  	dev -- the device this task is destined to
> -	list -- must be initialized (INIT_LIST_HEAD)
>  	task_proto -- _one_ of enum sas_proto
>  	scatter -- pointer to scatter gather list array
>  	num_scatter -- number of elements in scatter
> diff --git a/drivers/scsi/aic94xx/aic94xx.h b/drivers/scsi/aic94xx/aic94xx.h
> index 66cda66..26d4ad9 100644
> --- a/drivers/scsi/aic94xx/aic94xx.h
> +++ b/drivers/scsi/aic94xx/aic94xx.h
> @@ -78,7 +78,7 @@ void asd_dev_gone(struct domain_device *dev);
>  
>  void asd_invalidate_edb(struct asd_ascb *ascb, int edb_id);
>  
> -int  asd_execute_task(struct sas_task *, int num, gfp_t gfp_flags);
> +int  asd_execute_task(struct sas_task *task, gfp_t gfp_flags);
>  
>  void asd_set_dmamode(struct domain_device *dev);
>  
> diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.c b/drivers/scsi/aic94xx/aic94xx_hwi.c
> index 4df867e..9f636a3 100644
> --- a/drivers/scsi/aic94xx/aic94xx_hwi.c
> +++ b/drivers/scsi/aic94xx/aic94xx_hwi.c
> @@ -1200,8 +1200,7 @@ static void asd_start_scb_timers(struct list_head *list)
>   * Case A: we can send the whole batch at once.  Increment "pending"
>   * in the beginning of this function, when it is checked, in order to
>   * eliminate races when this function is called by multiple processes.
> - * Case B: should never happen if the managing layer considers
> - * lldd_queue_size.
> + * Case B: should never happen.
>   */
>  int asd_post_ascb_list(struct asd_ha_struct *asd_ha, struct asd_ascb *ascb,
>  		       int num)
> diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
> index 579dc2f..49e4465 100644
> --- a/drivers/scsi/aic94xx/aic94xx_init.c
> +++ b/drivers/scsi/aic94xx/aic94xx_init.c
> @@ -49,14 +49,6 @@ MODULE_PARM_DESC(use_msi, "\n"
>  	"\tEnable(1) or disable(0) using PCI MSI.\n"
>  	"\tDefault: 0");
>  
> -static int lldd_max_execute_num = 0;
> -module_param_named(collector, lldd_max_execute_num, int, S_IRUGO);
> -MODULE_PARM_DESC(collector, "\n"
> -	"\tIf greater than one, tells the SAS Layer to run in Task Collector\n"
> -	"\tMode.  If 1 or 0, tells the SAS Layer to run in Direct Mode.\n"
> -	"\tThe aic94xx SAS LLDD supports both modes.\n"
> -	"\tDefault: 0 (Direct Mode).\n");
> -
>  static struct scsi_transport_template *aic94xx_transport_template;
>  static int asd_scan_finished(struct Scsi_Host *, unsigned long);
>  static void asd_scan_start(struct Scsi_Host *);
> @@ -710,9 +702,6 @@ static int asd_register_sas_ha(struct asd_ha_struct *asd_ha)
>  	asd_ha->sas_ha.sas_port= sas_ports;
>  	asd_ha->sas_ha.num_phys= ASD_MAX_PHYS;
>  
> -	asd_ha->sas_ha.lldd_queue_size = asd_ha->seq.can_queue;
> -	asd_ha->sas_ha.lldd_max_execute_num = lldd_max_execute_num;
> -
>  	return sas_register_ha(&asd_ha->sas_ha);
>  }
>  
> diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
> index 59b86e2..5ff1ce7 100644
> --- a/drivers/scsi/aic94xx/aic94xx_task.c
> +++ b/drivers/scsi/aic94xx/aic94xx_task.c
> @@ -543,8 +543,7 @@ static int asd_can_queue(struct asd_ha_struct *asd_ha, int num)
>  	return res;
>  }
>  
> -int asd_execute_task(struct sas_task *task, const int num,
> -		     gfp_t gfp_flags)
> +int asd_execute_task(struct sas_task *task, gfp_t gfp_flags)
>  {
>  	int res = 0;
>  	LIST_HEAD(alist);
> @@ -553,11 +552,11 @@ int asd_execute_task(struct sas_task *task, const int num,
>  	struct asd_ha_struct *asd_ha = task->dev->port->ha->lldd_ha;
>  	unsigned long flags;
>  
> -	res = asd_can_queue(asd_ha, num);
> +	res = asd_can_queue(asd_ha, 1);
>  	if (res)
>  		return res;
>  
> -	res = num;
> +	res = 1;
>  	ascb = asd_ascb_alloc_list(asd_ha, &res, gfp_flags);
>  	if (res) {
>  		res = -ENOMEM;
> @@ -568,7 +567,7 @@ int asd_execute_task(struct sas_task *task, const int num,
>  	list_for_each_entry(a, &alist, list) {
>  		a->uldd_task = t;
>  		t->lldd_task = a;
> -		t = list_entry(t->list.next, struct sas_task, list);
> +		break;
>  	}
>  	list_for_each_entry(a, &alist, list) {
>  		t = a->uldd_task;
> @@ -601,7 +600,7 @@ int asd_execute_task(struct sas_task *task, const int num,
>  	}
>  	list_del_init(&alist);
>  
> -	res = asd_post_ascb_list(asd_ha, ascb, num);
> +	res = asd_post_ascb_list(asd_ha, ascb, 1);
>  	if (unlikely(res)) {
>  		a = NULL;
>  		__list_add(&alist, ascb->list.prev, &ascb->list);
> @@ -639,6 +638,6 @@ out_err_unmap:
>  out_err:
>  	if (ascb)
>  		asd_ascb_free_list(ascb);
> -	asd_can_dequeue(asd_ha, num);
> +	asd_can_dequeue(asd_ha, 1);
>  	return res;
>  }
Why did you not remove the last argument from asd_can_queue() and
asd_can_dequeue(), too?
With that patch it's always '1' ...

[ .. ]
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 76570e6..b93f289 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -350,7 +350,7 @@ static int sas_find_local_port_id(struct domain_device *dev)
>    */
>  #define DEV_IS_GONE(pm8001_dev)	\
>  	((!pm8001_dev || (pm8001_dev->dev_type == SAS_PHY_UNUSED)))
> -static int pm8001_task_exec(struct sas_task *task, const int num,
> +static int pm8001_task_exec(struct sas_task *task,
>  	gfp_t gfp_flags, int is_tmf, struct pm8001_tmf_task *tmf)
>  {
>  	struct domain_device *dev = task->dev;
> @@ -360,7 +360,6 @@ static int pm8001_task_exec(struct sas_task *task, const int num,
>  	struct sas_task *t = task;
>  	struct pm8001_ccb_info *ccb;
>  	u32 tag = 0xdeadbeef, rc, n_elem = 0;
> -	u32 n = num;
>  	unsigned long flags = 0;
>  
>  	if (!dev->port) {
> @@ -387,18 +386,12 @@ static int pm8001_task_exec(struct sas_task *task, const int num,
>  				spin_unlock_irqrestore(&pm8001_ha->lock, flags);
>  				t->task_done(t);
>  				spin_lock_irqsave(&pm8001_ha->lock, flags);
> -				if (n > 1)
> -					t = list_entry(t->list.next,
> -							struct sas_task, list);
>  				continue;
>  			} else {
>  				struct task_status_struct *ts = &t->task_status;
>  				ts->resp = SAS_TASK_UNDELIVERED;
>  				ts->stat = SAS_PHY_DOWN;
>  				t->task_done(t);
> -				if (n > 1)
> -					t = list_entry(t->list.next,
> -							struct sas_task, list);
>  				continue;
>  			}
>  		}
> @@ -460,9 +453,7 @@ static int pm8001_task_exec(struct sas_task *task, const int num,
>  		t->task_state_flags |= SAS_TASK_AT_INITIATOR;
>  		spin_unlock(&t->task_state_lock);
>  		pm8001_dev->running_req++;
> -		if (n > 1)
> -			t = list_entry(t->list.next, struct sas_task, list);
> -	} while (--n);
> +	} while (0);
>  	rc = 0;
>  	goto out_done;
>  
This 'while' loop can go, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html