[PATCH 0/3] nvme specialized queue fixes

[PATCH 0/3] nvme specialized queue fixes

[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

[PATCH 1/3] nvme/pci: Fix write and poll queue types

[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

[PATCH 2/3] nvme/pci Limit write queue sizes to possible cpus

[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

[PATCH 3/3] nvme/pci: Fix read queue count

[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

Re: [PATCH 0/3] nvme specialized queue fixes

[Jens Axboe]

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

Re: [PATCH 0/3] nvme specialized queue fixes

[Keith Busch]

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

Re: [PATCH 3/3] nvme/pci: Fix read queue count

[Ming Lei]

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