linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] nvme specialized queue fixes
@ 2019-12-06 17:13 Keith Busch
  2019-12-06 17:13 ` [PATCH 1/3] nvme/pci: Fix write and poll queue types Keith Busch
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Keith Busch @ 2019-12-06 17:13 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch

The nvme pci module had been allowing bad values to the specialized
polled and write queues that could have caused the driver to allocate
queues incorrectly or unoptimally.

For example, on a system with 16 CPUs, if I have module parameter
nvme.write_queues=17, we would get only 1 read queue no matter how many
the drive supported:

  # dmesg | grep nvme | grep "poll queues"
  nvme nvme2: 16/1/2 default/read/poll queues
  nvme nvme1: 16/1/2 default/read/poll queues
  nvme nvme0: 16/1/2 default/read/poll queues
  nvme nvme3: 16/1/2 default/read/poll queues

But after fixing:

  # dmesg | grep nvme | grep "poll queues"
  nvme nvme1: 16/16/2 default/read/poll queues
  nvme nvme2: 16/13/2 default/read/poll queues
  nvme nvme0: 16/16/2 default/read/poll queues
  nvme nvme3: 16/13/2 default/read/poll queues

We just need to fix the calculation so that we don't throttle the total
number of possible desired queues incorrectly. The first two patches
ensure the module parameters are withing reasonable boundaries to
simplify counting the number of interrupts we want to allocate.

Keith Busch (3):
  nvme/pci: Fix write and poll queue types
  nvme/pci Limit write queue sizes to possible cpus
  nvme/pci: Fix read queue count

 drivers/nvme/host/pci.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.21.0


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* [PATCH 1/3] nvme/pci: Fix write and poll queue types
  2019-12-06 17:13 [PATCH 0/3] nvme specialized queue fixes Keith Busch
@ 2019-12-06 17:13 ` Keith Busch
  2019-12-06 17:13 ` [PATCH 2/3] nvme/pci Limit write queue sizes to possible cpus Keith Busch
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Keith Busch @ 2019-12-06 17:13 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch

The number of poll or write queues should never be negative. Use unsigned
types so that it's not possible to have the driver allocate no queues.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 68701921dc23..7186f21ad899 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -66,14 +66,14 @@ static int io_queue_depth = 1024;
 module_param_cb(io_queue_depth, &io_queue_depth_ops, &io_queue_depth, 0644);
 MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");
 
-static int write_queues;
-module_param(write_queues, int, 0644);
+static unsigned int write_queues;
+module_param(write_queues, uint, 0644);
 MODULE_PARM_DESC(write_queues,
 	"Number of queues to use for writes. If not set, reads and writes "
 	"will share a queue set.");
 
-static int poll_queues;
-module_param(poll_queues, int, 0644);
+static unsigned int poll_queues;
+module_param(poll_queues, uint, 0644);
 MODULE_PARM_DESC(poll_queues, "Number of queues to use for polled IO.");
 
 struct nvme_dev;
-- 
2.21.0


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* [PATCH 2/3] nvme/pci Limit write queue sizes to possible cpus
  2019-12-06 17:13 [PATCH 0/3] nvme specialized queue fixes Keith Busch
  2019-12-06 17:13 ` [PATCH 1/3] nvme/pci: Fix write and poll queue types Keith Busch
@ 2019-12-06 17:13 ` Keith Busch
  2019-12-06 17:13 ` [PATCH 3/3] nvme/pci: Fix read queue count Keith Busch
  2019-12-06 17:46 ` [PATCH 0/3] nvme specialized queue fixes Jens Axboe
  3 siblings, 0 replies; 7+ messages in thread
From: Keith Busch @ 2019-12-06 17:13 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch

The driver can never use more queues of any type than the number of
possible CPUs. A larger parameter value causes the driver to allocate
more memory for IO queues than it could ever use. Limit the parameter
at module load time to the number of possible cpus.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7186f21ad899..6b6452486155 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3146,6 +3146,9 @@ static int __init nvme_init(void)
 	BUILD_BUG_ON(sizeof(struct nvme_create_sq) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_delete_queue) != 64);
 	BUILD_BUG_ON(IRQ_AFFINITY_MAX_SETS < 2);
+
+	write_queues = min(write_queues, num_possible_cpus());
+	poll_queues = min(poll_queues, num_possible_cpus());
 	return pci_register_driver(&nvme_driver);
 }
 
-- 
2.21.0


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* [PATCH 3/3] nvme/pci: Fix read queue count
  2019-12-06 17:13 [PATCH 0/3] nvme specialized queue fixes Keith Busch
  2019-12-06 17:13 ` [PATCH 1/3] nvme/pci: Fix write and poll queue types Keith Busch
  2019-12-06 17:13 ` [PATCH 2/3] nvme/pci Limit write queue sizes to possible cpus Keith Busch
@ 2019-12-06 17:13 ` Keith Busch
  2019-12-07  8:55   ` Ming Lei
  2019-12-06 17:46 ` [PATCH 0/3] nvme specialized queue fixes Jens Axboe
  3 siblings, 1 reply; 7+ messages in thread
From: Keith Busch @ 2019-12-06 17:13 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch

If nvme.write_queues and poll_queues are specified, we expect the driver
may request more queues than CPUs if the device's queue count feature
is large enough. The driver, however, had been decreasing the number of
possible interrupt enabled queues, artifically limiting the number of
read queues even if the controller could support more.

The driver doesn't request more IO queues than CPUs for each queues type
anyway, so remove the cpu count comparison to the number of interrupt
enabled io queues.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6b6452486155..d3bed5df9ef1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2062,7 +2062,6 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 		.priv		= dev,
 	};
 	unsigned int irq_queues, this_p_queues;
-	unsigned int nr_cpus = num_possible_cpus();
 
 	/*
 	 * Poll queues don't need interrupts, but we need at least one IO
@@ -2073,10 +2072,7 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 		this_p_queues = nr_io_queues - 1;
 		irq_queues = 1;
 	} else {
-		if (nr_cpus < nr_io_queues - this_p_queues)
-			irq_queues = nr_cpus + 1;
-		else
-			irq_queues = nr_io_queues - this_p_queues + 1;
+		irq_queues = nr_io_queues - this_p_queues + 1;
 	}
 	dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
 
-- 
2.21.0


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* Re: [PATCH 0/3] nvme specialized queue fixes
  2019-12-06 17:13 [PATCH 0/3] nvme specialized queue fixes Keith Busch
                   ` (2 preceding siblings ...)
  2019-12-06 17:13 ` [PATCH 3/3] nvme/pci: Fix read queue count Keith Busch
@ 2019-12-06 17:46 ` Jens Axboe
  2019-12-06 17:58   ` Keith Busch
  3 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2019-12-06 17:46 UTC (permalink / raw)
  To: Keith Busch, linux-nvme, hch, sagi

On 12/6/19 10:13 AM, Keith Busch wrote:
> The nvme pci module had been allowing bad values to the specialized
> polled and write queues that could have caused the driver to allocate
> queues incorrectly or unoptimally.
> 
> For example, on a system with 16 CPUs, if I have module parameter
> nvme.write_queues=17, we would get only 1 read queue no matter how many
> the drive supported:
> 
>   # dmesg | grep nvme | grep "poll queues"
>   nvme nvme2: 16/1/2 default/read/poll queues
>   nvme nvme1: 16/1/2 default/read/poll queues
>   nvme nvme0: 16/1/2 default/read/poll queues
>   nvme nvme3: 16/1/2 default/read/poll queues
> 
> But after fixing:
> 
>   # dmesg | grep nvme | grep "poll queues"
>   nvme nvme1: 16/16/2 default/read/poll queues
>   nvme nvme2: 16/13/2 default/read/poll queues
>   nvme nvme0: 16/16/2 default/read/poll queues
>   nvme nvme3: 16/13/2 default/read/poll queues
> 
> We just need to fix the calculation so that we don't throttle the total
> number of possible desired queues incorrectly. The first two patches
> ensure the module parameters are withing reasonable boundaries to
> simplify counting the number of interrupts we want to allocate.
> 
> Keith Busch (3):
>   nvme/pci: Fix write and poll queue types
>   nvme/pci Limit write queue sizes to possible cpus
>   nvme/pci: Fix read queue count
> 
>  drivers/nvme/host/pci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

Looks good to me:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* Re: [PATCH 0/3] nvme specialized queue fixes
  2019-12-06 17:46 ` [PATCH 0/3] nvme specialized queue fixes Jens Axboe
@ 2019-12-06 17:58   ` Keith Busch
  0 siblings, 0 replies; 7+ messages in thread
From: Keith Busch @ 2019-12-06 17:58 UTC (permalink / raw)
  To: Jens Axboe; +Cc: hch, linux-nvme, sagi

On Fri, Dec 06, 2019 at 10:46:04AM -0700, Jens Axboe wrote:
> Looks good to me:
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>

Thank you, queued up in nvme/for-5.5. Will be sending you the next pull
request later today. I'm just hoping to hear test results on another
patch.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

* Re: [PATCH 3/3] nvme/pci: Fix read queue count
  2019-12-06 17:13 ` [PATCH 3/3] nvme/pci: Fix read queue count Keith Busch
@ 2019-12-07  8:55   ` Ming Lei
  0 siblings, 0 replies; 7+ messages in thread
From: Ming Lei @ 2019-12-07  8:55 UTC (permalink / raw)
  To: Keith Busch; +Cc: Minwoo Im, hch, linux-nvme, sagi

On 12/7/19, Keith Busch <kbusch@kernel.org> wrote:
> If nvme.write_queues and poll_queues are specified, we expect the driver
> may request more queues than CPUs if the device's queue count feature
> is large enough. The driver, however, had been decreasing the number of
> possible interrupt enabled queues, artifically limiting the number of
> read queues even if the controller could support more.
>
> The driver doesn't request more IO queues than CPUs for each queues type
> anyway, so remove the cpu count comparison to the number of interrupt
> enabled io queues.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/pci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6b6452486155..d3bed5df9ef1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2062,7 +2062,6 @@ static int nvme_setup_irqs(struct nvme_dev *dev,
> unsigned int nr_io_queues)
>  		.priv		= dev,
>  	};
>  	unsigned int irq_queues, this_p_queues;
> -	unsigned int nr_cpus = num_possible_cpus();
>
>  	/*
>  	 * Poll queues don't need interrupts, but we need at least one IO
> @@ -2073,10 +2072,7 @@ static int nvme_setup_irqs(struct nvme_dev *dev,
> unsigned int nr_io_queues)
>  		this_p_queues = nr_io_queues - 1;
>  		irq_queues = 1;
>  	} else {
> -		if (nr_cpus < nr_io_queues - this_p_queues)
> -			irq_queues = nr_cpus + 1;
> -		else
> -			irq_queues = nr_io_queues - this_p_queues + 1;
> +		irq_queues = nr_io_queues - this_p_queues + 1;
>  	}
>  	dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
>
> --
> 2.21.0

Fixes: dad77d63903e("nvme-pci: adjust irq max_vector using num_possible_cpus()")


Thanks,
Ming Lei

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

end of thread, other threads:[~2019-12-07  8:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 17:13 [PATCH 0/3] nvme specialized queue fixes Keith Busch
2019-12-06 17:13 ` [PATCH 1/3] nvme/pci: Fix write and poll queue types Keith Busch
2019-12-06 17:13 ` [PATCH 2/3] nvme/pci Limit write queue sizes to possible cpus Keith Busch
2019-12-06 17:13 ` [PATCH 3/3] nvme/pci: Fix read queue count Keith Busch
2019-12-07  8:55   ` Ming Lei
2019-12-06 17:46 ` [PATCH 0/3] nvme specialized queue fixes Jens Axboe
2019-12-06 17:58   ` Keith Busch

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).