[PATCH 0/4] ahci per-port msix support

[PATCH 0/4] ahci per-port msix support

[Dan Williams]

A series of small fixups for achi implementations that provide multiple
msi-x vectors.  The notable changes are needing to maintain support for
the current single-vector msix implementation and disabling threaded
irqs by default.

---

Dan Williams (4):
      ahci: ahci_host_activate: kill IRQF_SHARED
      ahci: cleanup ahci_host_activate_multi_irqs
      ahci: per-port msix support
      ahci: switch from 'threaded' to 'hardirq' interrupt handling


 drivers/ata/ahci.c    |   69 ++++++++++++++++++++++++++++++++---------------
 drivers/ata/ahci.h    |    2 +
 drivers/ata/libahci.c |   72 +++++++++++++++++++++++++++++++++----------------
 3 files changed, 97 insertions(+), 46 deletions(-)

[PATCH 1/4] ahci: ahci_host_activate: kill IRQF_SHARED

[Dan Williams]

MSI messages are per-device, so there will never be another "shared"
device in the interrupt chain.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libahci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index d256a66158be..887749916d3e 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2486,7 +2486,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 
 		rc = devm_request_threaded_irq(host->dev, irq + i,
 					       ahci_multi_irqs_intr,
-					       ahci_port_thread_fn, IRQF_SHARED,
+					       ahci_port_thread_fn, 0,
 					       pp->irq_desc, host->ports[i]);
 		if (rc)
 			goto out_free_irqs;

[PATCH 2/4] ahci: cleanup ahci_host_activate_multi_irqs

[Dan Williams]

With devm there is no need to explicitly free irqs on error.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libahci.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 887749916d3e..373c7b1602ff 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2489,25 +2489,10 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 					       ahci_port_thread_fn, 0,
 					       pp->irq_desc, host->ports[i]);
 		if (rc)
-			goto out_free_irqs;
-	}
-
-	for (i = 0; i < host->n_ports; i++)
+			return rc;
 		ata_port_desc(host->ports[i], "irq %d", irq + i);
-
-	rc = ata_host_register(host, sht);
-	if (rc)
-		goto out_free_all_irqs;
-
-	return 0;
-
-out_free_all_irqs:
-	i = host->n_ports;
-out_free_irqs:
-	for (i--; i >= 0; i--)
-		devm_free_irq(host->dev, irq + i, host->ports[i]);
-
-	return rc;
+	}
+	return ata_host_register(host, sht);
 }
 
 /**

Re: [PATCH 2/4] ahci: cleanup ahci_host_activate_multi_irqs

[Tejun Heo]

On Fri, Oct 30, 2015 at 05:09:35PM -0400, Dan Williams wrote:
> With devm there is no need to explicitly free irqs on error.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Applied 1-2 to libata/for-4.4.

Thanks.

-- 
tejun

[PATCH 3/4] ahci: per-port msix support

[Dan Williams]

From: Dan Williams <dan.j.williamps@intel.com>

Some AHCI controllers support per-port MSI-X vectors.  At the same time
the Linux AHCI driver needs to support one-off architectures that
implement a single MSI-X vector for all ports.  The heuristic for
enabling AHCI ports becomes, in order of preference:

1/ per-port multi-MSI-X

2/ per-port multi-MSI

3/ single MSI

4/ single MSI-X

5/ legacy INTX

This all depends on AHCI implementations with potentially broken MSI-X
requesting less vectors than the number of ports.  If this assumption is
violated we will need to start explicitly white-listing AHCI-MSIX
implementations.

Reported-by: Ricardo Neri <ricardo.neri@intel.com>
[ricardo: fix struct msix_entry handling]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/ahci.c    |   69 +++++++++++++++++++++++++++++++++----------------
 drivers/ata/ahci.h    |    2 +
 drivers/ata/libahci.c |   19 ++++++++++---
 3 files changed, 63 insertions(+), 27 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a46660204e3a..688c785bb973 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1292,15 +1292,14 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
 #endif
 
 /*
- * ahci_init_msix() only implements single MSI-X support, not multiple
- * MSI-X per-port interrupts. This is needed for host controllers that only
- * have MSI-X support implemented, but no MSI or intx.
+ * ahci_init_msix() - optionally enable per-port MSI-X otherwise defer
+ * to single msi.
  */
 static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
-			  struct ahci_host_priv *hpriv)
+			  struct ahci_host_priv *hpriv, unsigned long flags)
 {
-	int rc, nvec;
-	struct msix_entry entry = {};
+	unsigned int nvec, i;
+	int rc;
 
 	/* Do not init MSI-X if MSI is disabled for the device */
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
@@ -1310,22 +1309,40 @@ static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
 	if (nvec < 0)
 		return nvec;
 
-	if (!nvec) {
+	/*
+	 * Proper MSI-X implementations will have a vector per-port.
+	 * Barring that, we prefer single-MSI over single-MSIX.  If this
+	 * check fails (not enough MSI-X vectors for all ports) we will
+	 * be called again with the flag clear iff ahci_init_msi()
+	 * fails.
+	 */
+	if (flags & AHCI_HFLAG_MULTI_MSIX) {
+		if (nvec < n_ports)
+			return -ENODEV;
+		else
+			nvec = min(nvec, n_ports);
+	} else if (nvec) {
+		nvec = 1;
+	} else {
+		/*
+		 * Emit dev_err() since this was the non-legacy irq
+		 * method of last resort.
+		 */
 		rc = -ENODEV;
 		goto fail;
 	}
 
-	/*
-	 * There can be more than one vector (e.g. for error detection or
-	 * hdd hotplug). Only the first vector (entry.entry = 0) is used.
-	 */
-	rc = pci_enable_msix_exact(pdev, &entry, 1);
+	for (i = 0; i < nvec; i++)
+		hpriv->msix[i].entry = i;
+	rc = pci_enable_msix_exact(pdev, hpriv->msix, nvec);
 	if (rc < 0)
 		goto fail;
 
-	hpriv->irq = entry.vector;
+	if (nvec > 1)
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSIX;
+	hpriv->irq = hpriv->msix[0].vector; /* for single msi-x */
 
-	return 1;
+	return nvec;
 fail:
 	dev_err(&pdev->dev,
 		"failed to enable MSI-X with error %d, # of vectors: %d\n",
@@ -1389,20 +1406,25 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 {
 	int nvec;
 
+	/*
+	 * Try to enable per-port MSI-X.  If the host is not capable
+	 * fall back to single MSI before finally attempting single
+	 * MSI-X.
+	 */
+	nvec = ahci_init_msix(pdev, n_ports, hpriv, AHCI_HFLAG_MULTI_MSIX);
+	if (nvec >= 0)
+		return nvec;
+
 	nvec = ahci_init_msi(pdev, n_ports, hpriv);
 	if (nvec >= 0)
 		return nvec;
 
-	/*
-	 * Currently, MSI-X support only implements single IRQ mode and
-	 * exists for controllers which can't do other types of IRQ. Only
-	 * set it up if MSI fails.
-	 */
-	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	/* try single-msix */
+	nvec = ahci_init_msix(pdev, n_ports, hpriv, 0);
 	if (nvec >= 0)
 		return nvec;
 
-	/* lagacy intx interrupts */
+	/* legacy intx interrupts */
 	pci_intx(pdev, 1);
 	hpriv->irq = pdev->irq;
 
@@ -1564,7 +1586,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!host)
 		return -ENOMEM;
 	host->private_data = hpriv;
-
+	hpriv->msix = devm_kzalloc(&pdev->dev,
+			sizeof(struct msix_entry) * n_ports, GFP_KERNEL);
+	if (!hpriv->msix)
+		return -ENOMEM;
 	ahci_init_interrupts(pdev, n_ports, hpriv);
 
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 5b8e8a0fab48..5ab9d158c90c 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -240,6 +240,7 @@ enum {
 	AHCI_HFLAG_NO_FBS		= (1 << 18), /* no FBS */
 	AHCI_HFLAG_EDGE_IRQ		= (1 << 19), /* HOST_IRQ_STAT behaves as
 							Edge Triggered */
+	AHCI_HFLAG_MULTI_MSIX		= (1 << 20), /* per-port MSI-X */
 
 	/* ap->flags bits */
 
@@ -341,6 +342,7 @@ struct ahci_host_priv {
 	 * the PHY position in this array.
 	 */
 	struct phy		**phys;
+	struct msix_entry	*msix;		/* Optional MSI-X support */
 	unsigned		nports;		/* Number of ports */
 	void			*plat_data;	/* Other platform data */
 	unsigned int		irq;		/* interrupt line */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 373c7b1602ff..c6f098a0435c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -43,6 +43,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_cmnd.h>
 #include <linux/libata.h>
+#include <linux/pci.h>
 #include "ahci.h"
 #include "libata.h"
 
@@ -2463,9 +2464,10 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv,
 }
 EXPORT_SYMBOL_GPL(ahci_set_em_messages);
 
-static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
+static int ahci_host_activate_multi_irqs(struct ata_host *host,
 					 struct scsi_host_template *sht)
 {
+	struct ahci_host_priv *hpriv = host->private_data;
 	int i, rc;
 
 	rc = ata_host_start(host);
@@ -2477,6 +2479,12 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 	 */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ahci_port_priv *pp = host->ports[i]->private_data;
+		int irq;
+
+		if (hpriv->flags & AHCI_HFLAG_MULTI_MSIX)
+			irq = hpriv->msix[i].vector;
+		else
+			irq = hpriv->irq + i;
 
 		/* Do not receive interrupts sent by dummy ports */
 		if (!pp) {
@@ -2484,14 +2492,15 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 			continue;
 		}
 
-		rc = devm_request_threaded_irq(host->dev, irq + i,
+		rc = devm_request_threaded_irq(host->dev, irq,
 					       ahci_multi_irqs_intr,
 					       ahci_port_thread_fn, 0,
 					       pp->irq_desc, host->ports[i]);
 		if (rc)
 			return rc;
-		ata_port_desc(host->ports[i], "irq %d", irq + i);
+		ata_port_desc(host->ports[i], "irq %d", irq);
 	}
+
 	return ata_host_register(host, sht);
 }
 
@@ -2512,8 +2521,8 @@ int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 	int irq = hpriv->irq;
 	int rc;
 
-	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
-		rc = ahci_host_activate_multi_irqs(host, irq, sht);
+	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX))
+		rc = ahci_host_activate_multi_irqs(host, sht);
 	else if (hpriv->flags & AHCI_HFLAG_EDGE_IRQ)
 		rc = ata_host_activate(host, irq, ahci_single_edge_irq_intr,
 				       IRQF_SHARED, sht);

Re: [PATCH 3/4] ahci: per-port msix support

[Tejun Heo]

Hello,

Generally looks good to me.  One nitpick.

On Fri, Oct 30, 2015 at 05:09:40PM -0400, Dan Williams wrote:
> +	if (flags & AHCI_HFLAG_MULTI_MSIX) {
> +		if (nvec < n_ports)
> +			return -ENODEV;
> +		else
> +			nvec = min(nvec, n_ports);

Maybe the following is easier?

		if (nvec < n_ports)
			return -ENODEV;

		nvec = n_ports;

Thanks.

-- 
tejun

Re: [PATCH 3/4] ahci: per-port msix support

[Dan Williams]

On Fri, Oct 30, 2015 at 6:03 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> Generally looks good to me.  One nitpick.
>
> On Fri, Oct 30, 2015 at 05:09:40PM -0400, Dan Williams wrote:
>> +     if (flags & AHCI_HFLAG_MULTI_MSIX) {
>> +             if (nvec < n_ports)
>> +                     return -ENODEV;
>> +             else
>> +                     nvec = min(nvec, n_ports);
>
> Maybe the following is easier?
>
>                 if (nvec < n_ports)
>                         return -ENODEV;
>
>                 nvec = n_ports;
>

Yeah, I missed that.  Will fix.

[PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Dan Williams]

For high frequency I/O the overhead of threaded interrupts impacts
performance.  Add an option to make it configurable, with the default
being hardirq.

A quick out-of-the-box test (i.e. no affinity tuning) shows ~10% random
read performance at ~20% less cpu.  The cpu wins appear to be from
reduced lock contention.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libahci.c |   38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c6f098a0435c..69bbd679d6aa 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -57,6 +57,10 @@ MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)
 module_param_named(ignore_sss, ahci_ignore_sss, int, 0444);
 MODULE_PARM_DESC(ignore_sss, "Ignore staggered spinup flag (0=don't ignore, 1=ignore)");
 
+static int use_threaded_interrupts;
+module_param(use_threaded_interrupts, int, 0444);
+MODULE_PARM_DESC(, "Defer interrupt handling to a thread (0=don't defer, 1=defer)");
+
 static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			unsigned hints);
 static ssize_t ahci_led_show(struct ata_port *ap, char *buf);
@@ -1826,6 +1830,26 @@ static irqreturn_t ahci_multi_irqs_intr(int irq, void *dev_instance)
 	return IRQ_WAKE_THREAD;
 }
 
+static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
+{
+	struct ata_port *ap = dev_instance;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 status;
+
+	VPRINTK("ENTER\n");
+
+	status = readl(port_mmio + PORT_IRQ_STAT);
+	writel(status, port_mmio + PORT_IRQ_STAT);
+
+	spin_lock(ap->lock);
+	ahci_handle_port_interrupt(ap, port_mmio, status);
+	spin_unlock(ap->lock);
+
+	VPRINTK("EXIT\n");
+
+	return IRQ_HANDLED;
+}
+
 static u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
 {
 	unsigned int i, handled = 0;
@@ -2492,10 +2516,16 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 			continue;
 		}
 
-		rc = devm_request_threaded_irq(host->dev, irq,
-					       ahci_multi_irqs_intr,
-					       ahci_port_thread_fn, 0,
-					       pp->irq_desc, host->ports[i]);
+		if (use_threaded_interrupts)
+			rc = devm_request_threaded_irq(host->dev, irq,
+					ahci_multi_irqs_intr,
+					ahci_port_thread_fn, 0,
+					pp->irq_desc, host->ports[i]);
+		else
+			rc = devm_request_irq(host->dev, irq,
+					ahci_multi_irqs_intr_hard, 0,
+					pp->irq_desc, host->ports[i]);
+
 		if (rc)
 			return rc;
 		ata_port_desc(host->ports[i], "irq %d", irq);

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Tejun Heo]

On Fri, Oct 30, 2015 at 05:09:46PM -0400, Dan Williams wrote:
> For high frequency I/O the overhead of threaded interrupts impacts
> performance.  Add an option to make it configurable, with the default
> being hardirq.
> 
> A quick out-of-the-box test (i.e. no affinity tuning) shows ~10% random
> read performance at ~20% less cpu.  The cpu wins appear to be from
> reduced lock contention.

Do we need threaded irq at all?  Why not just switch to hardirq?

Thanks.

-- 
tejun

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Dan Williams]

On Fri, Oct 30, 2015 at 5:56 PM, Tejun Heo <tj@kernel.org> wrote:
> On Fri, Oct 30, 2015 at 05:09:46PM -0400, Dan Williams wrote:
>> For high frequency I/O the overhead of threaded interrupts impacts
>> performance.  Add an option to make it configurable, with the default
>> being hardirq.
>>
>> A quick out-of-the-box test (i.e. no affinity tuning) shows ~10% random
>> read performance at ~20% less cpu.  The cpu wins appear to be from
>> reduced lock contention.
>
> Do we need threaded irq at all?  Why not just switch to hardirq?
>

I can't imagine anyone doing high iops storage to also rely on the
ability to preempt the irq handler.  I'm assuming if someone notices
it missing they can scream, but otherwise hardirq seems all around
better.

NVMe also has this optional via module parameter, but talking to Keith
he does not know of anyone using it.

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Tejun Heo]

On Fri, Oct 30, 2015 at 06:59:14PM -0700, Dan Williams wrote:
> On Fri, Oct 30, 2015 at 5:56 PM, Tejun Heo <tj@kernel.org> wrote:
> > On Fri, Oct 30, 2015 at 05:09:46PM -0400, Dan Williams wrote:
> >> For high frequency I/O the overhead of threaded interrupts impacts
> >> performance.  Add an option to make it configurable, with the default
> >> being hardirq.
> >>
> >> A quick out-of-the-box test (i.e. no affinity tuning) shows ~10% random
> >> read performance at ~20% less cpu.  The cpu wins appear to be from
> >> reduced lock contention.
> >
> > Do we need threaded irq at all?  Why not just switch to hardirq?
> >
> 
> I can't imagine anyone doing high iops storage to also rely on the
> ability to preempt the irq handler.  I'm assuming if someone notices
> it missing they can scream, but otherwise hardirq seems all around
> better.
> 
> NVMe also has this optional via module parameter, but talking to Keith
> he does not know of anyone using it.

Let's remove it for now and do the conditional thing if anybody misses
it.  No need to keep around dead code proactively.

Thanks.

-- 
tejun

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Mark Lord]

On 15-10-30 10:01 PM, Tejun Heo wrote:
> On Fri, Oct 30, 2015 at 06:59:14PM -0700, Dan Williams wrote:
>> On Fri, Oct 30, 2015 at 5:56 PM, Tejun Heo <tj@kernel.org> wrote:
>>> On Fri, Oct 30, 2015 at 05:09:46PM -0400, Dan Williams wrote:
>>>> For high frequency I/O the overhead of threaded interrupts impacts
>>>> performance.  Add an option to make it configurable, with the default
>>>> being hardirq.
>>>>
>>>> A quick out-of-the-box test (i.e. no affinity tuning) shows ~10% random
>>>> read performance at ~20% less cpu.  The cpu wins appear to be from
>>>> reduced lock contention.
>>>
>>> Do we need threaded irq at all?  Why not just switch to hardirq?
>>>
>>
>> I can't imagine anyone doing high iops storage to also rely on the
>> ability to preempt the irq handler.  I'm assuming if someone notices
>> it missing they can scream, but otherwise hardirq seems all around
>> better.
>>
>> NVMe also has this optional via module parameter, but talking to Keith
>> he does not know of anyone using it.
>
> Let's remove it for now and do the conditional thing if anybody misses
> it.  No need to keep around dead code proactively.

Aren't threaded IRQs there to improve overall system real-time response?
If so, it would seem a step backwards to remove them completely,
especially considering the low cost of maintaining them here.

Cheers

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Tejun Heo]

Hello, Mark.

On Mon, Nov 16, 2015 at 08:52:35AM -0500, Mark Lord wrote:
> >Let's remove it for now and do the conditional thing if anybody misses
> >it.  No need to keep around dead code proactively.
> 
> Aren't threaded IRQs there to improve overall system real-time response?
> If so, it would seem a step backwards to remove them completely,
> especially considering the low cost of maintaining them here.

For ahci, it was added without any justification.  It just came with
the MSI changes.  If there's any data indicating that this makes any
noticeable difference, it's easy to restore.

Thanks.

-- 
tejun

Re: [PATCH 4/4] ahci: switch from 'threaded' to 'hardirq' interrupt handling

[Dan Williams]

On Mon, Nov 16, 2015 at 7:33 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Mark.
>
> On Mon, Nov 16, 2015 at 08:52:35AM -0500, Mark Lord wrote:
>> >Let's remove it for now and do the conditional thing if anybody misses
>> >it.  No need to keep around dead code proactively.
>>
>> Aren't threaded IRQs there to improve overall system real-time response?
>> If so, it would seem a step backwards to remove them completely,
>> especially considering the low cost of maintaining them here.
>
> For ahci, it was added without any justification.  It just came with
> the MSI changes.  If there's any data indicating that this makes any
> noticeable difference, it's easy to restore.
>

Also, the non-mult-msi case has been living without threaded
interrupts since forever.  If they were needed I expect the
functionality would have been ported over by now.  As is this provides
a noticeable throughput improvement.