Re: [PATCH v3 37/42] hpsa: use block layer tag for command allocation

[Webb Scales <webbnh@hp.com>]

Tomas,

You are correct that the previous approach of using find_next_zero_bit() 
with a persistent offset is more run-time efficient in the worst case; 
however, given that "normal" operation doesn't call this allocation 
routine, and given that, when this routine is called, the bit mask being 
searched only has about 16 bits in it, I opted for simplicity over 
efficiency -- that is, I doubt that the difference in efficiency is 
discernible, while getting rid of the last_allocation field is a 
worthwhile savings in both memory use and code.

Regarding your earlier comment on the refcount variable, I believe that 
it was handy for debugging purposes.  The code has undergone numerous 
revisions, and the variable certainly could now be removed from the 
source per your suggestion.  (Of course, the compiler is already 
removing it, I'm sure. ;-) )


                 Webb



-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com]
Sent: Monday, March 23, 2015 11:58 AM
To: Don Brace; Scott Teel; Kevin Barnett; james.bottomley@parallels.com; hch@infradead.org; Justin Lindley; brace
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH v3 37/42] hpsa: use block layer tag for command allocation

On 03/17/2015 09:06 PM, Don Brace wrote:

> From: Webb Scales <webbnh@hp.com>
>
> Rework slave allocation:
>    - separate the tagging support setup from the hostdata setup
>    - make the hostdata setup act consistently when the lookup fails
>    - make the hostdata setup act consistently when the device is not added
>    - set up the queue depth consistently across these scenarios
>    - if the block layer mq support is not available, explicitly enable and
>      activate the SCSI layer tcq support (and do this at allocation-time so
>      that the tags will be available for INQUIRY commands)
>
> Tweak slave configuration so that devices which are masked are also
> not attached.
>
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> Signed-off-by: Webb Scales <webbnh@hp.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
>   drivers/scsi/hpsa.c |  153 +++++++++++++++++++++++++++++++++++++++++----------
>   drivers/scsi/hpsa.h |    1
>   2 files changed, 123 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 34c178c..4e34a62 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -44,6 +44,7 @@
>   #include <scsi/scsi_host.h>
>   #include <scsi/scsi_tcq.h>
>   #include <scsi/scsi_eh.h>
> +#include <scsi/scsi_dbg.h>
>   #include <linux/cciss_ioctl.h>
>   #include <linux/string.h>
>   #include <linux/bitmap.h>
> @@ -212,6 +213,9 @@ static int hpsa_compat_ioctl(struct scsi_device *dev, int cmd,
>   
>   static void cmd_free(struct ctlr_info *h, struct CommandList *c);
>   static struct CommandList *cmd_alloc(struct ctlr_info *h);
> +static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c);
> +static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
> +					    struct scsi_cmnd *scmd);
>   static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
>   	void *buff, size_t size, u16 page_code, unsigned char *scsi3addr,
>   	int cmd_type);
> @@ -2047,11 +2051,17 @@ static void hpsa_cmd_resolve_events(struct ctlr_info *h,
>   	}
>   }
>   
> +static void hpsa_cmd_resolve_and_free(struct ctlr_info *h,
> +				      struct CommandList *c)
> +{
> +	hpsa_cmd_resolve_events(h, c);
> +	cmd_tagged_free(h, c);
> +}
> +
>   static void hpsa_cmd_free_and_done(struct ctlr_info *h,
>   		struct CommandList *c, struct scsi_cmnd *cmd)
>   {
> -	hpsa_cmd_resolve_events(h, c);
> -	cmd_free(h, c);
> +	hpsa_cmd_resolve_and_free(h, c);
>   	cmd->scsi_done(cmd);
>   }
>   
> @@ -2072,8 +2082,7 @@ static void hpsa_cmd_abort_and_free(struct ctlr_info *h, struct CommandList *c,
>   	hpsa_set_scsi_cmd_aborted(cmd);
>   	dev_warn(&h->pdev->dev, "CDB %16phN was aborted with status 0x%x\n",
>   			 c->Request.CDB, c->err_info->ScsiStatus);
> -	hpsa_cmd_resolve_events(h, c);
> -	cmd_free(h, c);		/* FIX-ME:  change to cmd_tagged_free(h, c) */
> +	hpsa_cmd_resolve_and_free(h, c);
>   }
>   
>   static void process_ioaccel2_completion(struct ctlr_info *h,
> @@ -4535,7 +4544,7 @@ static int hpsa_ciss_submit(struct ctlr_info *h,
>   	}
>   
>   	if (hpsa_scatter_gather(h, c, cmd) < 0) { /* Fill SG list */
> -		cmd_free(h, c);
> +		hpsa_cmd_resolve_and_free(h, c);
>   		return SCSI_MLQUEUE_HOST_BUSY;
>   	}
>   	enqueue_cmd_and_start_io(h, c);
> @@ -4581,6 +4590,8 @@ static inline void hpsa_cmd_partial_init(struct ctlr_info *h, int index,
>   {
>   	dma_addr_t cmd_dma_handle = h->cmd_pool_dhandle + index * sizeof(*c);
>   
> +	BUG_ON(c->cmdindex != index);
> +
>   	memset(c->Request.CDB, 0, sizeof(c->Request.CDB));
>   	memset(c->err_info, 0, sizeof(*c->err_info));
>   	c->busaddr = (u32) cmd_dma_handle;
> @@ -4675,27 +4686,24 @@ static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
>   
>   	/* Get the ptr to our adapter structure out of cmd->host. */
>   	h = sdev_to_hba(cmd->device);
> +
> +	BUG_ON(cmd->request->tag < 0);
> +
>   	dev = cmd->device->hostdata;
>   	if (!dev) {
>   		cmd->result = DID_NO_CONNECT << 16;
>   		cmd->scsi_done(cmd);
>   		return 0;
>   	}
> -	memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
>   
> -	if (unlikely(lockup_detected(h))) {
> -		cmd->result = DID_NO_CONNECT << 16;
> -		cmd->scsi_done(cmd);
> -		return 0;
> -	}
> -	c = cmd_alloc(h);
> +	memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
>   
>   	if (unlikely(lockup_detected(h))) {
>   		cmd->result = DID_NO_CONNECT << 16;
> -		cmd_free(h, c);
>   		cmd->scsi_done(cmd);
>   		return 0;
>   	}
> +	c = cmd_tagged_alloc(h, cmd);
>   
>   	/*
>   	 * Call alternate submit routine for I/O accelerated commands.
> @@ -4708,7 +4716,7 @@ static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
>   		if (rc == 0)
>   			return 0;
>   		if (rc == SCSI_MLQUEUE_HOST_BUSY) {
> -			cmd_free(h, c);
> +			hpsa_cmd_resolve_and_free(h, c);
>   			return SCSI_MLQUEUE_HOST_BUSY;
>   		}
>   	}
> @@ -4822,15 +4830,23 @@ static int hpsa_register_scsi(struct ctlr_info *h)
>   	sh->hostdata[0] = (unsigned long) h;
>   	sh->irq = h->intr[h->intr_mode];
>   	sh->unique_id = sh->irq;
> +	error = scsi_init_shared_tag_map(sh, sh->can_queue);
> +	if (error) {
> +		dev_err(&h->pdev->dev,
> +			"%s: scsi_init_shared_tag_map failed for controller %d\n",
> +			__func__, h->ctlr);
> +		goto fail_host_put;
> +	}
>   	error = scsi_add_host(sh, &h->pdev->dev);
> -	if (error)
> +	if (error) {
> +		dev_err(&h->pdev->dev, "%s: scsi_add_host failed for controller %d\n",
> +			__func__, h->ctlr);
>   		goto fail_host_put;
> +	}
>   	scsi_scan_host(sh);
>   	return 0;
>   
>    fail_host_put:
> -	dev_err(&h->pdev->dev, "%s: scsi_add_host"
> -		" failed for controller %d\n", __func__, h->ctlr);
>   	scsi_host_put(sh);
>   	return error;
>    fail:
> @@ -4840,6 +4856,23 @@ static int hpsa_register_scsi(struct ctlr_info *h)
>   }
>   
>   /*
> + * The block layer has already gone to the trouble of picking out a unique,
> + * small-integer tag for this request.  We use an offset from that value as
> + * an index to select our command block.  (The offset allows us to reserve the
> + * low-numbered entries for our own uses.)
> + */
> +static int hpsa_get_cmd_index(struct scsi_cmnd *scmd)
> +{
> +	int idx = scmd->request->tag;
> +
> +	if (idx < 0)
> +		return idx;
> +
> +	/* Offset to leave space for internal cmds. */
> +	return idx += HPSA_NRESERVED_CMDS;
> +}
> +
> +/*
>    * Send a TEST_UNIT_READY command to the specified LUN using the specified
>    * reply queue; returns zero if the unit is ready, and non-zero otherwise.
>    */
> @@ -4979,18 +5012,18 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
>   	/* if controller locked up, we can guarantee command won't complete */
>   	if (lockup_detected(h)) {
>   		dev_warn(&h->pdev->dev,
> -			"scsi %d:%d:%d:%d RESET FAILED, lockup detected\n",
> -			h->scsi_host->host_no, dev->bus, dev->target,
> -			dev->lun);
> +			 "scsi %d:%d:%d:%u cmd %d RESET FAILED, lockup detected\n",
> +			 h->scsi_host->host_no, dev->bus, dev->target, dev->lun,
> +			 hpsa_get_cmd_index(scsicmd));
>   		return FAILED;
>   	}
>   
>   	/* this reset request might be the result of a lockup; check */
>   	if (detect_controller_lockup(h)) {
>   		dev_warn(&h->pdev->dev,
> -			 "scsi %d:%d:%d:%d RESET FAILED, new lockup detected\n",
> +			 "scsi %d:%d:%d:%u cmd %d RESET FAILED, new lockup detected\n",
>   			 h->scsi_host->host_no, dev->bus, dev->target,
> -			 dev->lun);
> +			 dev->lun, hpsa_get_cmd_index(scsicmd));
>   		return FAILED;
>   	}
>   
> @@ -5442,6 +5475,59 @@ static int hpsa_eh_abort_handler(struct scsi_cmnd *sc)
>   }
>   
>   /*
> + * For operations with an associated SCSI command, a command block is allocated
> + * at init, and managed by cmd_tagged_alloc() and cmd_tagged_free() using the
> + * block request tag as an index into a table of entries.  cmd_tagged_free() is
> + * the complement, although cmd_free() may be called instead.
> + */
> +static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
> +					    struct scsi_cmnd *scmd)
> +{
> +	int idx = hpsa_get_cmd_index(scmd);
> +	struct CommandList *c = h->cmd_pool + idx;
> +	int refcount = 0;
> +
> +	if (idx < HPSA_NRESERVED_CMDS || idx >= h->nr_cmds) {
> +		dev_err(&h->pdev->dev, "Bad block tag: %d not in [%d..%d]\n",
> +			idx, HPSA_NRESERVED_CMDS, h->nr_cmds - 1);
> +		/* The index value comes from the block layer, so if it's out of
> +		 * bounds, it's probably not our bug.
> +		 */
> +		BUG();
> +	}
> +
> +	refcount = atomic_inc_return(&c->refcount);

refcount is never used, use atomic_inc(&c->refcount); instead?

> +	if (unlikely(!hpsa_is_cmd_idle(c))) {
> +		/*
> +		 * We expect that the SCSI layer will hand us a unique tag
> +		 * value.  Thus, there should never be a collision here between
> +		 * two requests...because if the selected command isn't idle
> +		 * then someone is going to be very disappointed.
> +		 */
> +		dev_err(&h->pdev->dev,
> +			"tag collision (tag=%d) in cmd_tagged_alloc().\n",
> +			idx);
> +		if (c->scsi_cmd != NULL)
> +			scsi_print_command(c->scsi_cmd);
> +		scsi_print_command(scmd);
> +	}
> +
> +	hpsa_cmd_partial_init(h, idx, c);
> +	return c;
> +}
> +
> +static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c)
> +{
> +	/*
> +	 * Release our reference to the block.  We don't need to do anything
> +	 * else to free it, because it is accessed by index.  (There's no point
> +	 * in checking the result of the decrement, since we cannot guarantee
> +	 * that there isn't a concurrent abort which is also accessing it.)
> +	 */
> +	(void)atomic_dec(&c->refcount);
> +}
> +
> +/*
>    * For operations that cannot sleep, a command block is allocated at init,
>    * and managed by cmd_alloc() and cmd_free() using a simple bitmap to track
>    * which ones are free or in use.  Lock must be held when calling this.
> @@ -5454,7 +5540,6 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
>   {
>   	struct CommandList *c;
>   	int refcount, i;
> -	unsigned long offset;
>   
>   	/*
>   	 * There is some *extremely* small but non-zero chance that that
> @@ -5466,31 +5551,39 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
>   	 * very unlucky thread might be starved anyway, never able to
>   	 * beat the other threads.  In reality, this happens so
>   	 * infrequently as to be indistinguishable from never.
> +	 *
> +	 * Note that we start allocating commands before the SCSI host structure
> +	 * is initialized.  Since the search starts at bit zero, this
> +	 * all works, since we have at least one command structure available;
> +	 * however, it means that the structures with the low indexes have to be
> +	 * reserved for driver-initiated requests, while requests from the block
> +	 * layer will use the higher indexes.
>   	 */
>   
> -	offset = h->last_allocation; /* benignly racy */
>   	for (;;) {
> -		i = find_next_zero_bit(h->cmd_pool_bits, h->nr_cmds, offset);
> -		if (unlikely(i == h->nr_cmds)) {
> -			offset = 0;
> +		i = find_first_zero_bit(h->cmd_pool_bits, HPSA_NRESERVED_CMDS);
> +		if (unlikely(i >= HPSA_NRESERVED_CMDS))
>   			continue;
> -		}
>   		c = h->cmd_pool + i;
>   		refcount = atomic_inc_return(&c->refcount);
>   		if (unlikely(refcount > 1)) {
>   			cmd_free(h, c); /* already in use */
> -			offset = (i + 1) % h->nr_cmds;

Hi Don,
when this happens - a command has its bitfield flag cleared and, but it's taken - refcount is > 1
it will be so likely for next several thousands of tests in this function until the it is freed.
When it is the first bit in the bitfield it will block all following commands sent to the card for that time.
The previous variant  'find_next_zero_bit + offset = (i + 1) % h->nr_cmds' seems to handle this better.
Cheers, Tomas

>   			continue;
>   		}
>   		set_bit(i & (BITS_PER_LONG - 1),
>   			h->cmd_pool_bits + (i / BITS_PER_LONG));
>   		break; /* it's ours now. */
>   	}
> -	h->last_allocation = i; /* benignly racy */
>   	hpsa_cmd_partial_init(h, i, c);
>   	return c;
>   }
>   
> +/*
> + * This is the complementary operation to cmd_alloc().  Note, however, in some
> + * corner cases it may also be used to free blocks allocated by
> + * cmd_tagged_alloc() in which case the ref-count decrement does the trick and
> + * the clear-bit is harmless.
> + */
>   static void cmd_free(struct ctlr_info *h, struct CommandList *c)
>   {
>   	if (atomic_dec_and_test(&c->refcount)) {
> diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
> index 3ec8934..2536b67 100644
> --- a/drivers/scsi/hpsa.h
> +++ b/drivers/scsi/hpsa.h
> @@ -141,7 +141,6 @@ struct ctlr_info {
>   	struct CfgTable __iomem *cfgtable;
>   	int	interrupts_enabled;
>   	int 	max_commands;
> -	int last_allocation;
>   	atomic_t commands_outstanding;
>   #	define PERF_MODE_INT	0
>   #	define DOORBELL_INT	1
>
> --
> 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