linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hpsa: scsi-mq support
@ 2016-11-11 15:46 Hannes Reinecke
  2016-11-11 16:34 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Hannes Reinecke @ 2016-11-11 15:46 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, Don Brace, linux-scsi,
	Hannes Reinecke, Hannes Reinecke

This patch enables full scsi-mq support for the hpsa driver.
Due to some reports of performance regressions this patch
also adds a parameter 'use_blk_mq' which can be used to
disable multiqueue support if required.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c | 64 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 0df0e04..ef4e81a 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -91,6 +91,9 @@
 MODULE_PARM_DESC(hpsa_simple_mode,
 	"Use 'simple mode' rather than 'performant mode'");
 
+bool use_blk_mq = true;
+module_param(use_blk_mq, bool, 0);
+
 /* define the PCI info for the cards we can control */
 static const struct pci_device_id hpsa_pci_device_id[] = {
 	{PCI_VENDOR_ID_HP,     PCI_DEVICE_ID_HP_CISSE,     0x103C, 0x3241},
@@ -1137,12 +1140,13 @@ static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
 	}
 }
 
-static void enqueue_cmd_and_start_io(struct ctlr_info *h, struct CommandList *c)
+static void enqueue_cmd_and_start_io(struct ctlr_info *h,
+				     struct CommandList *c, int reply_queue)
 {
 	if (unlikely(hpsa_is_pending_event(c)))
 		return finish_cmd(c);
 
-	__enqueue_cmd_and_start_io(h, c, DEFAULT_REPLY_QUEUE);
+	__enqueue_cmd_and_start_io(h, c, reply_queue);
 }
 
 static inline int is_hba_lunid(unsigned char scsi3addr[])
@@ -4614,6 +4618,7 @@ static int hpsa_scsi_ioaccel1_queue_command(struct ctlr_info *h,
 	int use_sg, i;
 	struct SGDescriptor *curr_sg;
 	u32 control = IOACCEL1_CONTROL_SIMPLEQUEUE;
+	int reply_queue = DEFAULT_REPLY_QUEUE;
 
 	/* TODO: implement chaining support */
 	if (scsi_sg_count(cmd) > h->ioaccel_maxsg) {
@@ -4683,8 +4688,12 @@ static int hpsa_scsi_ioaccel1_queue_command(struct ctlr_info *h,
 	cp->control = cpu_to_le32(control);
 	memcpy(cp->CDB, cdb, cdb_len);
 	memcpy(cp->CISS_LUN, scsi3addr, 8);
+	if (shost_use_blk_mq(cmd->device->host)) {
+		u32 blk_tag = blk_mq_unique_tag(cmd->request);
+		reply_queue = blk_mq_unique_tag_to_hwq(blk_tag);
+	}
 	/* Tag was already set at init time. */
-	enqueue_cmd_and_start_io(h, c);
+	enqueue_cmd_and_start_io(h, c, reply_queue);
 	return 0;
 }
 
@@ -4778,6 +4787,7 @@ static int hpsa_scsi_ioaccel2_queue_command(struct ctlr_info *h,
 	u64 addr64;
 	u32 len;
 	u32 total_len = 0;
+	int reply_queue = DEFAULT_REPLY_QUEUE;
 
 	if (!cmd->device)
 		return -1;
@@ -4882,7 +4892,11 @@ static int hpsa_scsi_ioaccel2_queue_command(struct ctlr_info *h,
 	} else
 		cp->sg_count = (u8) use_sg;
 
-	enqueue_cmd_and_start_io(h, c);
+	if (shost_use_blk_mq(cmd->device->host)) {
+		u32 blk_tag = blk_mq_unique_tag(cmd->request);
+		reply_queue = blk_mq_unique_tag_to_hwq(blk_tag);
+	}
+	enqueue_cmd_and_start_io(h, c, reply_queue);
 	return 0;
 }
 
@@ -5284,6 +5298,8 @@ static int hpsa_ciss_submit(struct ctlr_info *h,
 	struct CommandList *c, struct scsi_cmnd *cmd,
 	unsigned char scsi3addr[])
 {
+	int reply_queue = DEFAULT_REPLY_QUEUE;
+
 	cmd->host_scribble = (unsigned char *) c;
 	c->cmd_type = CMD_SCSI;
 	c->scsi_cmd = cmd;
@@ -5339,7 +5355,11 @@ static int hpsa_ciss_submit(struct ctlr_info *h,
 		hpsa_cmd_resolve_and_free(h, c);
 		return SCSI_MLQUEUE_HOST_BUSY;
 	}
-	enqueue_cmd_and_start_io(h, c);
+	if (shost_use_blk_mq(cmd->device->host)) {
+		u32 blk_tag = blk_mq_unique_tag(cmd->request);
+		reply_queue = blk_mq_unique_tag_to_hwq(blk_tag);
+	}
+	enqueue_cmd_and_start_io(h, c, reply_queue);
 	/* the cmd'll come back via intr handler in complete_scsi_command()  */
 	return 0;
 }
@@ -5642,13 +5662,23 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
 static int hpsa_scsi_add_host(struct ctlr_info *h)
 {
 	int rv;
+	struct Scsi_Host *sh = h->scsi_host;
 
-	rv = scsi_add_host(h->scsi_host, &h->pdev->dev);
+	if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
+		sh->nr_hw_queues = h->msix_vectors;
+	else
+		sh->nr_hw_queues = 1;
+
+	if (use_blk_mq) {
+		sh->can_queue = sh->can_queue / sh->nr_hw_queues;
+		sh->use_blk_mq = 1;
+	}
+	rv = scsi_add_host(sh, &h->pdev->dev);
 	if (rv) {
 		dev_err(&h->pdev->dev, "scsi_add_host failed\n");
 		return rv;
 	}
-	scsi_scan_host(h->scsi_host);
+	scsi_scan_host(sh);
 	return 0;
 }
 
@@ -5658,10 +5688,20 @@ static int hpsa_scsi_add_host(struct ctlr_info *h)
  * 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)
+static int hpsa_get_cmd_index(struct ctlr_info *h, struct scsi_cmnd *scmd)
 {
 	int idx = scmd->request->tag;
 
+	if (shost_use_blk_mq(scmd->device->host)) {
+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
+		u16 hwq = blk_mq_unique_tag_to_hwq(blk_tag);
+		u16 tag = blk_mq_unique_tag_to_tag(blk_tag);
+		int msix_vectors, hwq_size;
+
+		msix_vectors = h->msix_vectors > 0 ? h->msix_vectors : 1;
+		hwq_size = (h->nr_cmds - HPSA_NRESERVED_CMDS) / msix_vectors;
+		idx = (hwq * hwq_size) + tag;
+	}
 	if (idx < 0)
 		return idx;
 
@@ -5811,7 +5851,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	if (lockup_detected(h)) {
 		snprintf(msg, sizeof(msg),
 			 "cmd %d RESET FAILED, lockup detected",
-			 hpsa_get_cmd_index(scsicmd));
+			 hpsa_get_cmd_index(h, scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
@@ -5820,7 +5860,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	if (detect_controller_lockup(h)) {
 		snprintf(msg, sizeof(msg),
 			 "cmd %d RESET FAILED, new lockup detected",
-			 hpsa_get_cmd_index(scsicmd));
+			 hpsa_get_cmd_index(h, scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
@@ -6293,7 +6333,7 @@ static int hpsa_eh_abort_handler(struct scsi_cmnd *sc)
 static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 					    struct scsi_cmnd *scmd)
 {
-	int idx = hpsa_get_cmd_index(scmd);
+	int idx = hpsa_get_cmd_index(h, scmd);
 	struct CommandList *c = h->cmd_pool + idx;
 
 	if (idx < HPSA_NRESERVED_CMDS || idx >= h->nr_cmds) {
@@ -6857,7 +6897,7 @@ static void hpsa_send_host_reset(struct ctlr_info *h, unsigned char *scsi3addr,
 		RAID_CTLR_LUNID, TYPE_MSG);
 	c->Request.CDB[1] = reset_type; /* fill_cmd defaults to target reset */
 	c->waiting = NULL;
-	enqueue_cmd_and_start_io(h, c);
+	enqueue_cmd_and_start_io(h, c, DEFAULT_REPLY_QUEUE);
 	/* Don't wait for completion, the reset won't complete.  Don't free
 	 * the command either.  This is the last command we will send before
 	 * re-initializing everything, so it doesn't matter and won't leak.
-- 
1.8.5.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 15:46 [PATCH] hpsa: scsi-mq support Hannes Reinecke
@ 2016-11-11 16:34 ` kbuild test robot
  2016-11-11 16:35 ` Christoph Hellwig
  2016-11-11 16:57 ` kbuild test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2016-11-11 16:34 UTC (permalink / raw)
  Cc: kbuild-all, Christoph Hellwig, Martin K. Petersen,
	James Bottomley, Don Brace, linux-scsi, Hannes Reinecke,
	Hannes Reinecke

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

Hi Hannes,

[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.9-rc4 next-20161111]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/hpsa-scsi-mq-support/20161111-235235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-x017-201645 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/scsi/hpsa.c: In function 'hpsa_scsi_add_host':
>> drivers/scsi/hpsa.c:5653:40: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
                                           ^~
   drivers/scsi/hpsa.c:5654:23: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      sh->nr_hw_queues = h->msix_vectors;
                          ^~
   drivers/scsi/hpsa.c: In function 'hpsa_get_cmd_index':
   drivers/scsi/hpsa.c:5687:19: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      msix_vectors = h->msix_vectors > 0 ? h->msix_vectors : 1;
                      ^~
   drivers/scsi/hpsa.c:5687:41: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      msix_vectors = h->msix_vectors > 0 ? h->msix_vectors : 1;
                                            ^~

vim +5653 drivers/scsi/hpsa.c

  5647	
  5648	static int hpsa_scsi_add_host(struct ctlr_info *h)
  5649	{
  5650		int rv;
  5651		struct Scsi_Host *sh = h->scsi_host;
  5652	
> 5653		if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
  5654			sh->nr_hw_queues = h->msix_vectors;
  5655		else
  5656			sh->nr_hw_queues = 1;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24338 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 15:46 [PATCH] hpsa: scsi-mq support Hannes Reinecke
  2016-11-11 16:34 ` kbuild test robot
@ 2016-11-11 16:35 ` Christoph Hellwig
  2016-11-11 18:22   ` Hannes Reinecke
  2016-11-11 16:57 ` kbuild test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-11-11 16:35 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Christoph Hellwig, Martin K. Petersen, James Bottomley, Don Brace,
	linux-scsi, Hannes Reinecke, Jens Axboe

On Fri, Nov 11, 2016 at 04:46:34PM +0100, Hannes Reinecke wrote:
> This patch enables full scsi-mq support for the hpsa driver.
> Due to some reports of performance regressions this patch
> also adds a parameter 'use_blk_mq' which can be used to
> disable multiqueue support if required.

This patch looks odd to me.  The hardware does not seem to support
multiple submission queues, which makes exposing nr_hw_queues > 1
rather pointless given that the block layer (using blk-mq or the legacy
path) can already steer completions to the submitting cpu.

You also mention there are performance regressions, but don't talk
about any benefits.  I also think per-driver module parameters for this
are generally a rather bad idea.

So what exactly are the benefits of this MQ-mode for a single submission
queue device, and what do we need to do to get these without lying about
the number of submission queues?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 15:46 [PATCH] hpsa: scsi-mq support Hannes Reinecke
  2016-11-11 16:34 ` kbuild test robot
  2016-11-11 16:35 ` Christoph Hellwig
@ 2016-11-11 16:57 ` kbuild test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2016-11-11 16:57 UTC (permalink / raw)
  Cc: kbuild-all, Christoph Hellwig, Martin K. Petersen,
	James Bottomley, Don Brace, linux-scsi, Hannes Reinecke,
	Hannes Reinecke

[-- Attachment #1: Type: text/plain, Size: 4341 bytes --]

Hi Hannes,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.9-rc4 next-20161111]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/hpsa-scsi-mq-support/20161111-235235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-x016-201645 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/module.h:9,
                    from drivers/scsi/hpsa.c:20:
   drivers/scsi/hpsa.c: In function 'hpsa_scsi_add_host':
   drivers/scsi/hpsa.c:5653:40: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
                                           ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/scsi/hpsa.c:5653:2: note: in expansion of macro 'if'
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
     ^~
   drivers/scsi/hpsa.c:5653:40: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
                                           ^
   include/linux/compiler.h:149:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> drivers/scsi/hpsa.c:5653:2: note: in expansion of macro 'if'
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
     ^~
   drivers/scsi/hpsa.c:5653:40: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
                                           ^
   include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> drivers/scsi/hpsa.c:5653:2: note: in expansion of macro 'if'
     if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
     ^~
   drivers/scsi/hpsa.c:5654:23: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      sh->nr_hw_queues = h->msix_vectors;
                          ^~
   drivers/scsi/hpsa.c: In function 'hpsa_get_cmd_index':
   drivers/scsi/hpsa.c:5687:19: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      msix_vectors = h->msix_vectors > 0 ? h->msix_vectors : 1;
                      ^~
   drivers/scsi/hpsa.c:5687:41: error: 'struct ctlr_info' has no member named 'msix_vectors'; did you mean 'msix_vector'?
      msix_vectors = h->msix_vectors > 0 ? h->msix_vectors : 1;
                                            ^~

vim +/if +5653 drivers/scsi/hpsa.c

  5637		sh->cmd_per_lun = sh->can_queue;
  5638		sh->sg_tablesize = h->maxsgentries;
  5639		sh->transportt = hpsa_sas_transport_template;
  5640		sh->hostdata[0] = (unsigned long) h;
  5641		sh->irq = h->intr[h->intr_mode];
  5642		sh->unique_id = sh->irq;
  5643	
  5644		h->scsi_host = sh;
  5645		return 0;
  5646	}
  5647	
  5648	static int hpsa_scsi_add_host(struct ctlr_info *h)
  5649	{
  5650		int rv;
  5651		struct Scsi_Host *sh = h->scsi_host;
  5652	
> 5653		if (h->intr_mode == PERF_MODE_INT && h->msix_vectors > 0)
  5654			sh->nr_hw_queues = h->msix_vectors;
  5655		else
  5656			sh->nr_hw_queues = 1;
  5657	
  5658		if (use_blk_mq) {
  5659			sh->can_queue = sh->can_queue / sh->nr_hw_queues;
  5660			sh->use_blk_mq = 1;
  5661		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29563 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 16:35 ` Christoph Hellwig
@ 2016-11-11 18:22   ` Hannes Reinecke
  2016-11-12 17:32     ` Christoph Hellwig
  2016-11-12 18:30     ` Jens Axboe
  0 siblings, 2 replies; 10+ messages in thread
From: Hannes Reinecke @ 2016-11-11 18:22 UTC (permalink / raw)
  To: Christoph Hellwig, Hannes Reinecke
  Cc: Christoph Hellwig, Martin K. Petersen, James Bottomley, Don Brace,
	linux-scsi, Jens Axboe

On 11/11/2016 05:35 PM, Christoph Hellwig wrote:
> On Fri, Nov 11, 2016 at 04:46:34PM +0100, Hannes Reinecke wrote:
>> This patch enables full scsi-mq support for the hpsa driver.
>> Due to some reports of performance regressions this patch
>> also adds a parameter 'use_blk_mq' which can be used to
>> disable multiqueue support if required.
>
> This patch looks odd to me.  The hardware does not seem to support
> multiple submission queues, which makes exposing nr_hw_queues > 1
> rather pointless given that the block layer (using blk-mq or the legacy
> path) can already steer completions to the submitting cpu.
>
Well, it's the same as with megasas and mpt3sas. Each of those have
a single MMIO register where the driver writes the address of the
command into. What exactly the hardware does in the back doesn't
really matter here; the command is in memory and the hardware can
access it as it sees fit. So from that point of view we can assume
having a submission queue to match the completion queue;
With that setup we do have a contention point on the single command 
register, but that's about it.
We still should benefit from scsi-mq, though.

> You also mention there are performance regressions, but don't talk
> about any benefits.  I also think per-driver module parameters for this
> are generally a rather bad idea.
>
Well; this is also suffering from the same performance regression I 
noticed when doing performance testing on megasas.
I'm still trying to figure out _where_ to problem actually is (it might 
be fio itself, using several submission queues in parallel thereby
defeating merging), but it might also be due to the setup I've been
testing with.
As it's inconclusive I've added this parameter.

> So what exactly are the benefits of this MQ-mode for a single submission
> queue device, and what do we need to do to get these without lying about
> the number of submission queues?
>
As I said; for a single submission queue you are correct, but I don't 
think that is the case here.
But then Don should be able to shed some light here.

Cheers,

Hannes

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 18:22   ` Hannes Reinecke
@ 2016-11-12 17:32     ` Christoph Hellwig
  2016-11-13  9:44       ` Hannes Reinecke
  2016-11-12 18:30     ` Jens Axboe
  1 sibling, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-11-12 17:32 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Christoph Hellwig, Hannes Reinecke, Christoph Hellwig,
	Martin K. Petersen, James Bottomley, Don Brace, linux-scsi,
	Jens Axboe

On Fri, Nov 11, 2016 at 07:22:05PM +0100, Hannes Reinecke wrote:
> Well, it's the same as with megasas and mpt3sas. Each of those have
> a single MMIO register where the driver writes the address of the
> command into. What exactly the hardware does in the back doesn't
> really matter here; the command is in memory and the hardware can
> access it as it sees fit. So from that point of view we can assume
> having a submission queue to match the completion queue;
> With that setup we do have a contention point on the single command
> register, but that's about it.
> We still should benefit from scsi-mq, though.

How do we benefit from scsi-mq in this case?  We still hit global
cachelines like commands_outstanding in the driver, and we lost the
batching done by the ctx -> hw_ctx layering for the single queue
blk-mq case.  We also get much less efficient merging and will not
have the chance of having and I/O schedule in the near future.

But back to my question from the last mail:  What workload is improved
by using this patch?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-11 18:22   ` Hannes Reinecke
  2016-11-12 17:32     ` Christoph Hellwig
@ 2016-11-12 18:30     ` Jens Axboe
  1 sibling, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2016-11-12 18:30 UTC (permalink / raw)
  To: Hannes Reinecke, Christoph Hellwig, Hannes Reinecke
  Cc: Christoph Hellwig, Martin K. Petersen, James Bottomley, Don Brace,
	linux-scsi

On 11/11/2016 11:22 AM, Hannes Reinecke wrote:
> On 11/11/2016 05:35 PM, Christoph Hellwig wrote:
>> On Fri, Nov 11, 2016 at 04:46:34PM +0100, Hannes Reinecke wrote:
>>> This patch enables full scsi-mq support for the hpsa driver.
>>> Due to some reports of performance regressions this patch
>>> also adds a parameter 'use_blk_mq' which can be used to
>>> disable multiqueue support if required.
>>
>> This patch looks odd to me.  The hardware does not seem to support
>> multiple submission queues, which makes exposing nr_hw_queues > 1
>> rather pointless given that the block layer (using blk-mq or the legacy
>> path) can already steer completions to the submitting cpu.
>>
> Well, it's the same as with megasas and mpt3sas. Each of those have
> a single MMIO register where the driver writes the address of the
> command into. What exactly the hardware does in the back doesn't
> really matter here; the command is in memory and the hardware can
> access it as it sees fit. So from that point of view we can assume
> having a submission queue to match the completion queue;
> With that setup we do have a contention point on the single command
> register, but that's about it.

That's just wrong. We can have multiple completion queues without having
multiple submission queues. And for that to work ideally, you tell
blk/scsi-mq that you have 1 queue, since that is what you have. You
don't get great submission side scaling, but we can't pull that stuff
out of thin air. How you interface to the hardware DOES matter, it's the
whole point of multiple submission queues. Trying to make up some
imaginary 1:1 mapping between submission and completion queues is
nonsensical, when it doesn't exist.

> We still should benefit from scsi-mq, though.

Sure, but with 1 submission queue.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-12 17:32     ` Christoph Hellwig
@ 2016-11-13  9:44       ` Hannes Reinecke
  2016-11-13 11:58         ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: Hannes Reinecke @ 2016-11-13  9:44 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Hannes Reinecke, Christoph Hellwig, Martin K. Petersen,
	James Bottomley, Don Brace, linux-scsi, Jens Axboe

On 11/12/2016 06:32 PM, Christoph Hellwig wrote:
> On Fri, Nov 11, 2016 at 07:22:05PM +0100, Hannes Reinecke wrote:
>> Well, it's the same as with megasas and mpt3sas. Each of those have
>> a single MMIO register where the driver writes the address of the
>> command into. What exactly the hardware does in the back doesn't
>> really matter here; the command is in memory and the hardware can
>> access it as it sees fit. So from that point of view we can assume
>> having a submission queue to match the completion queue;
>> With that setup we do have a contention point on the single command
>> register, but that's about it.
>> We still should benefit from scsi-mq, though.
>
> How do we benefit from scsi-mq in this case?  We still hit global
> cachelines like commands_outstanding in the driver, and we lost the
> batching done by the ctx -> hw_ctx layering for the single queue
> blk-mq case.  We also get much less efficient merging and will not
> have the chance of having and I/O schedule in the near future.
>
One day to mark with bright red in the calendar.

Christoph Hellwig is telling me _NOT_ to use scsi-mq.

 > But back to my question from the last mail:  What workload is improved
 > by using this patch?
 >
This patch was done so see what would needed to be done to convert a 
legacy driver.
As I was under the impression that scsi-mq is the way forward, seeing 
that it should be enabled per default.
But I must have been mistaken. Apparently.

Slightly confused,

Hannes

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] hpsa: scsi-mq support
  2016-11-13  9:44       ` Hannes Reinecke
@ 2016-11-13 11:58         ` Christoph Hellwig
  2016-11-14 11:05           ` Kashyap Desai
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-11-13 11:58 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Christoph Hellwig, Hannes Reinecke, Christoph Hellwig,
	Martin K. Petersen, James Bottomley, Don Brace, linux-scsi,
	Jens Axboe

On Sun, Nov 13, 2016 at 10:44:47AM +0100, Hannes Reinecke wrote:
> One day to mark with bright red in the calendar.
>
> Christoph Hellwig is telling me _NOT_ to use scsi-mq.

That's not what I'm doing.

> This patch was done so see what would needed to be done to convert a legacy 
> driver.
> As I was under the impression that scsi-mq is the way forward, seeing that 
> it should be enabled per default.
> But I must have been mistaken. Apparently.

What I am doing is to tell you you should not expose multiple queues
unless the hardware actually has multiple submissions queues.  The blk-mq
and scsi-mq code works just fine with a single submission queue, and
the hpsa driver in particular works really well with scsi-mq and a single
submission queue.  E.g. the RAID HBA on slide 19 of this presentations is
an hpsa one:

http://events.linuxfoundation.org/sites/events/files/slides/scsi.pdf

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] hpsa: scsi-mq support
  2016-11-13 11:58         ` Christoph Hellwig
@ 2016-11-14 11:05           ` Kashyap Desai
  0 siblings, 0 replies; 10+ messages in thread
From: Kashyap Desai @ 2016-11-14 11:05 UTC (permalink / raw)
  To: Christoph Hellwig, Hannes Reinecke
  Cc: Christoph Hellwig, Hannes Reinecke, Martin K. Petersen,
	James Bottomley, Don Brace, linux-scsi, Jens Axboe

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Sunday, November 13, 2016 5:29 PM
> To: Hannes Reinecke
> Cc: Christoph Hellwig; Hannes Reinecke; Christoph Hellwig; Martin K.
Petersen;
> James Bottomley; Don Brace; linux-scsi@vger.kernel.org; Jens Axboe
> Subject: Re: [PATCH] hpsa: scsi-mq support
>
> On Sun, Nov 13, 2016 at 10:44:47AM +0100, Hannes Reinecke wrote:
> > One day to mark with bright red in the calendar.
> >
> > Christoph Hellwig is telling me _NOT_ to use scsi-mq.
>
> That's not what I'm doing.
>
> > This patch was done so see what would needed to be done to convert a
> > legacy driver.
> > As I was under the impression that scsi-mq is the way forward, seeing
> > that it should be enabled per default.
> > But I must have been mistaken. Apparently.
>
> What I am doing is to tell you you should not expose multiple queues
unless the
> hardware actually has multiple submissions queues.  The blk-mq and
scsi-mq
> code works just fine with a single submission queue, and the hpsa driver
in
> particular works really well with scsi-mq and a single submission queue.
E.g. the
> RAID HBA on slide 19 of this presentations is an hpsa one:

I have similar results for MegaRaid where seen MR driver gives significant
improvement for Single Submission Queue and multiple Completion Queue.
Having said that scsi-mq is enabled but with single Queue is more than
enough to maximize improvement of SCSI-MQ.

Major advantage was seen while IO load is cross the boundary of Physical
CPU socket.

>From this discussion I understood that - Similar logical changes proposed
for megaraid_sas  and we are not really going to gain with fake multiple
submission exposed to SML.

Kashyap

>
> http://events.linuxfoundation.org/sites/events/files/slides/scsi.pdf
> --
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-11-14 11:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-11 15:46 [PATCH] hpsa: scsi-mq support Hannes Reinecke
2016-11-11 16:34 ` kbuild test robot
2016-11-11 16:35 ` Christoph Hellwig
2016-11-11 18:22   ` Hannes Reinecke
2016-11-12 17:32     ` Christoph Hellwig
2016-11-13  9:44       ` Hannes Reinecke
2016-11-13 11:58         ` Christoph Hellwig
2016-11-14 11:05           ` Kashyap Desai
2016-11-12 18:30     ` Jens Axboe
2016-11-11 16:57 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).