[PATCHv2] nvme-pci: allow unmanaged interrupts

[PATCHv2] nvme-pci: allow unmanaged interrupts

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Some people _really_ want to control their interrupt affinity,
preferring to sacrafice storage performance for scheduling
predicatability on some other subset of CPUs.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
Sorry for the rapid fire v2, and I know some are still aginst this; I'm
just getting v2 out because v1 breaks a different use case.

And as far as acceptance goes, this doesn't look like it carries any
longterm maintenance overhead. It's an opt-in feature, and you're own
your own if you turn it on.

v1->v2: skip the the AFFINITY vector allocation if the parameter is
provided instead trying to make the vector code handle all post_vectors.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8e0bb9692685d..def1a295284bb 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -63,6 +63,11 @@ MODULE_PARM_DESC(sgl_threshold,
 		"Use SGLs when average request segment size is larger or equal to "
 		"this size. Use 0 to disable SGLs.");
 
+static bool managed_irqs = true;
+module_param(managed_irqs, bool, 0444);
+MODULE_PARM_DESC(managed_irqs,
+		 "set to false for user controlled irq affinity");
+
 #define NVME_PCI_MIN_QUEUE_SIZE 2
 #define NVME_PCI_MAX_QUEUE_SIZE 4095
 static int io_queue_depth_set(const char *val, const struct kernel_param *kp);
@@ -456,7 +461,7 @@ static void nvme_pci_map_queues(struct blk_mq_tag_set *set)
 		 * affinity), so use the regular blk-mq cpu mapping
 		 */
 		map->queue_offset = qoff;
-		if (i != HCTX_TYPE_POLL && offset)
+		if (managed_irqs && i != HCTX_TYPE_POLL && offset)
 			blk_mq_pci_map_queues(map, to_pci_dev(dev->dev), offset);
 		else
 			blk_mq_map_queues(map);
@@ -2218,6 +2223,7 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 		.priv		= dev,
 	};
 	unsigned int irq_queues, poll_queues;
+	int ret;
 
 	/*
 	 * Poll queues don't need interrupts, but we need at least one I/O queue
@@ -2241,8 +2247,15 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 	irq_queues = 1;
 	if (!(dev->ctrl.quirks & NVME_QUIRK_SINGLE_VECTOR))
 		irq_queues += (nr_io_queues - poll_queues);
-	return pci_alloc_irq_vectors_affinity(pdev, 1, irq_queues,
+
+	if (managed_irqs)
+		return pci_alloc_irq_vectors_affinity(pdev, 1, irq_queues,
 			      PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);
+
+	ret = pci_alloc_irq_vectors(pdev, 1, irq_queues, PCI_IRQ_ALL_TYPES);
+	if (ret > 0)
+		nvme_calc_irq_sets(&affd, ret - 1);
+	return ret;
 }
 
 static unsigned int nvme_max_io_queues(struct nvme_dev *dev)
-- 
2.43.0

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Ming Lei]

On Fri, May 10, 2024 at 10:46:45AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Some people _really_ want to control their interrupt affinity,
> preferring to sacrafice storage performance for scheduling
> predicatability on some other subset of CPUs.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> Sorry for the rapid fire v2, and I know some are still aginst this; I'm
> just getting v2 out because v1 breaks a different use case.
> 
> And as far as acceptance goes, this doesn't look like it carries any
> longterm maintenance overhead. It's an opt-in feature, and you're own
> your own if you turn it on.
> 
> v1->v2: skip the the AFFINITY vector allocation if the parameter is
> provided instead trying to make the vector code handle all post_vectors.
> 
>  drivers/nvme/host/pci.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8e0bb9692685d..def1a295284bb 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -63,6 +63,11 @@ MODULE_PARM_DESC(sgl_threshold,
>  		"Use SGLs when average request segment size is larger or equal to "
>  		"this size. Use 0 to disable SGLs.");
>  
> +static bool managed_irqs = true;
> +module_param(managed_irqs, bool, 0444);
> +MODULE_PARM_DESC(managed_irqs,
> +		 "set to false for user controlled irq affinity");
> +
>  #define NVME_PCI_MIN_QUEUE_SIZE 2
>  #define NVME_PCI_MAX_QUEUE_SIZE 4095
>  static int io_queue_depth_set(const char *val, const struct kernel_param *kp);
> @@ -456,7 +461,7 @@ static void nvme_pci_map_queues(struct blk_mq_tag_set *set)
>  		 * affinity), so use the regular blk-mq cpu mapping
>  		 */
>  		map->queue_offset = qoff;
> -		if (i != HCTX_TYPE_POLL && offset)
> +		if (managed_irqs && i != HCTX_TYPE_POLL && offset)
>  			blk_mq_pci_map_queues(map, to_pci_dev(dev->dev), offset);
>  		else
>  			blk_mq_map_queues(map);

Now the queue mapping is built with nothing from irq affinity which is
setup from userspace, and performance could be pretty bad.

Is there any benefit to use unmanaged irq in this way?


Thanks,
Ming

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Keith Busch]

On Sat, May 11, 2024 at 07:47:26AM +0800, Ming Lei wrote:
> On Fri, May 10, 2024 at 10:46:45AM -0700, Keith Busch wrote:
> >  		map->queue_offset = qoff;
> > -		if (i != HCTX_TYPE_POLL && offset)
> > +		if (managed_irqs && i != HCTX_TYPE_POLL && offset)
> >  			blk_mq_pci_map_queues(map, to_pci_dev(dev->dev), offset);
> >  		else
> >  			blk_mq_map_queues(map);
> 
> Now the queue mapping is built with nothing from irq affinity which is
> setup from userspace, and performance could be pretty bad.

This just decouples the sw from the irq mappings. Every cpu still has a
blk-mq hctx, there's just no connection to the completing CPU if you
enable this.

Everyone expects nvme performance will suffer. IO latency and CPU
efficieny are not everyone's top priority, so allowing people to
optimize for something else seems like a reasonable request.
 
> Is there any benefit to use unmanaged irq in this way?

The immediate desire is more predictable scheduling on a subset of CPUs
by steering hardware interrupts somewhere else. It's the same reason
RDMA undid managed interrupts.

  231243c82793428 ("Revert "mlx5: move affinity hints assignments to generic code")

Yes, the kernel's managed interrupts are the best choice for optimizing
interaction with that device, but it's not free, and maybe you want to
exchange that optimization for something else.

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Ming Lei]

On Fri, May 10, 2024 at 06:29:23PM -0600, Keith Busch wrote:
> On Sat, May 11, 2024 at 07:47:26AM +0800, Ming Lei wrote:
> > On Fri, May 10, 2024 at 10:46:45AM -0700, Keith Busch wrote:
> > >  		map->queue_offset = qoff;
> > > -		if (i != HCTX_TYPE_POLL && offset)
> > > +		if (managed_irqs && i != HCTX_TYPE_POLL && offset)
> > >  			blk_mq_pci_map_queues(map, to_pci_dev(dev->dev), offset);
> > >  		else
> > >  			blk_mq_map_queues(map);
> > 
> > Now the queue mapping is built with nothing from irq affinity which is
> > setup from userspace, and performance could be pretty bad.
> 
> This just decouples the sw from the irq mappings. Every cpu still has a
> blk-mq hctx, there's just no connection to the completing CPU if you
> enable this.

I don't object to unmanaged irq, which is actually supported in some scsi
hosts too, but all or most of them still wire pci irq vector affinities with
hw queue, instead of using mapping from blk_mq_map_queues() simply. 

> 
> Everyone expects nvme performance will suffer. IO latency and CPU
> efficieny are not everyone's top priority, so allowing people to
> optimize for something else seems like a reasonable request.

I guess more people may be interested in 'something else', care to share
them in the commit log, cause nvme is going to support it.

>  
> > Is there any benefit to use unmanaged irq in this way?
> 
> The immediate desire is more predictable scheduling on a subset of CPUs
> by steering hardware interrupts somewhere else. It's the same reason
> RDMA undid managed interrupts.
> 
>   231243c82793428 ("Revert "mlx5: move affinity hints assignments to generic code")

The above commit only mentions it becomes not flexible since user can't
adjust irq affinity any more.

It is understandable for network, there is long history people need to adjust
irq affinity from user space.


Thanks,
Ming

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Sagi Grimberg]

>> Everyone expects nvme performance will suffer. IO latency and CPU
>> efficieny are not everyone's top priority, so allowing people to
>> optimize for something else seems like a reasonable request.
> I guess more people may be interested in 'something else', care to share
> them in the commit log, cause nvme is going to support it.

I don't have a special interest in this, but I can share what I heard 
several
times. The use-case is that people want to dedicate a few cores to handle
interrupts so they know it does not take cpu time from their application 
threads
that are running (usually pinned to different cores).

The app threads isolation is more important to them than affinity to the 
device...

>
>>   
>>> Is there any benefit to use unmanaged irq in this way?
>> The immediate desire is more predictable scheduling on a subset of CPUs
>> by steering hardware interrupts somewhere else. It's the same reason
>> RDMA undid managed interrupts.
>>
>>    231243c82793428 ("Revert "mlx5: move affinity hints assignments to generic code")
> The above commit only mentions it becomes not flexible since user can't
> adjust irq affinity any more.
>
> It is understandable for network, there is long history people need to adjust
> irq affinity from user space.

I suspect that the reasoning is similar to nvme as well.

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Keith Busch]

On Sun, May 12, 2024 at 05:16:13PM +0300, Sagi Grimberg wrote:
> 
> > > Everyone expects nvme performance will suffer. IO latency and CPU
> > > efficieny are not everyone's top priority, so allowing people to
> > > optimize for something else seems like a reasonable request.
> > I guess more people may be interested in 'something else', care to share
> > them in the commit log, cause nvme is going to support it.
> 
> I don't have a special interest in this, but I can share what I heard
> several
> times. The use-case is that people want to dedicate a few cores to handle
> interrupts so they know it does not take cpu time from their application
> threads
> that are running (usually pinned to different cores).
> 
> The app threads isolation is more important to them than affinity to the
> device...

Yes, that is consistently the same reasoning I've heard. While managed
irq is overwhelmingly the best choice for most use cases, it's clearly
been communicated that some users do not want it for exactly this
reason.

As far as I can tell, there's no techincal reason to prevent letting
people make that choice. This "kernel knows better than you" argument is
less sustainable than letting users do whatever they want with their
CPUs.

> > > > Is there any benefit to use unmanaged irq in this way?
> > > The immediate desire is more predictable scheduling on a subset of CPUs
> > > by steering hardware interrupts somewhere else. It's the same reason
> > > RDMA undid managed interrupts.
> > > 
> > >    231243c82793428 ("Revert "mlx5: move affinity hints assignments to generic code")
> > The above commit only mentions it becomes not flexible since user can't
> > adjust irq affinity any more.
> > 
> > It is understandable for network, there is long history people need to adjust
> > irq affinity from user space.
> 
> I suspect that the reasoning is similar to nvme as well.

+1, exactly.

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Ming Lei]

On Sun, May 12, 2024 at 05:16:13PM +0300, Sagi Grimberg wrote:
> 
> > > Everyone expects nvme performance will suffer. IO latency and CPU
> > > efficieny are not everyone's top priority, so allowing people to
> > > optimize for something else seems like a reasonable request.
> > I guess more people may be interested in 'something else', care to share
> > them in the commit log, cause nvme is going to support it.
> 
> I don't have a special interest in this, but I can share what I heard
> several
> times. The use-case is that people want to dedicate a few cores to handle
> interrupts so they know it does not take cpu time from their application
> threads
> that are running (usually pinned to different cores).
> 
> The app threads isolation is more important to them than affinity to the
> device...

That is exactly what CPU isolation is doing, include 'isolcpus=managed_irq',
isn't it?

Thanks, 
Ming

Re: [PATCHv2] nvme-pci: allow unmanaged interrupts

[Keith Busch]

On Mon, May 13, 2024 at 09:12:58AM +0800, Ming Lei wrote:
> On Sun, May 12, 2024 at 05:16:13PM +0300, Sagi Grimberg wrote:
> > 
> > > > Everyone expects nvme performance will suffer. IO latency and CPU
> > > > efficieny are not everyone's top priority, so allowing people to
> > > > optimize for something else seems like a reasonable request.
> > > I guess more people may be interested in 'something else', care to share
> > > them in the commit log, cause nvme is going to support it.
> > 
> > I don't have a special interest in this, but I can share what I heard
> > several
> > times. The use-case is that people want to dedicate a few cores to handle
> > interrupts so they know it does not take cpu time from their application
> > threads
> > that are running (usually pinned to different cores).
> > 
> > The app threads isolation is more important to them than affinity to the
> > device...
> 
> That is exactly what CPU isolation is doing, include 'isolcpus=managed_irq',
> isn't it?

As I've mentioned previously, that option is a no-op when the incoming
mask matches the isolcated cpus. The use case for the kernel's isolated
CPUs doesn't align with the use cases for user defined IRQ affinity.

Let me redirect this discussion please. Is there a techincal reason why
Linux can't let users use their CPUs as they intend? They will take out
of tree patches if that's the position we're forceing them into, but why
is that is Linux taking that position in the first place?