[PATCH v2 0/2] ahci per-port msix support

[PATCH v2 0/2] ahci per-port msix support

[Dan Williams]

Changes since v1 [1]:

1/ Clean up "nvec = min(nvec, n_ports);" statement (Tejun)

2/ Drop threaded interrupt handling option for now.

[1]: http://marc.info/?l=linux-ide&m=144623970817213&w=2


---

A series of small fixups for ahci 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 (2):
      ahci: per-port msix support
      ahci: switch from 'threaded' to 'hardirq' interrupt handling


 drivers/ata/ahci.c    |   68 +++++++++++++++++++++++++++++++++----------------
 drivers/ata/ahci.h    |    2 +
 drivers/ata/libahci.c |   44 ++++++++++++++++++++++++++------
 3 files changed, 84 insertions(+), 30 deletions(-)

[PATCH v2 1/2] 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    |   68 +++++++++++++++++++++++++++++++++----------------
 drivers/ata/ahci.h    |    2 +
 drivers/ata/libahci.c |   19 ++++++++++----
 3 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a46660204e3a..bda8ec2d1a20 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,39 @@ 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;
+		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 +1405,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 +1585,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);

[PATCH v2 2/2] 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 |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c6f098a0435c..f10a702abfb5 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1826,6 +1826,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 +2512,9 @@ 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]);
+		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 v2 0/2] ahci per-port msix support

[Tejun Heo]

On Sun, Nov 01, 2015 at 01:43:35PM -0500, Dan Williams wrote:
> Changes since v1 [1]:
> 
> 1/ Clean up "nvec = min(nvec, n_ports);" statement (Tejun)
> 
> 2/ Drop threaded interrupt handling option for now.
> 
> [1]: http://marc.info/?l=linux-ide&m=144623970817213&w=2

Applied to libata/for-4.4.

Thanks.

-- 
tejun

Re: [PATCH v2 0/2] ahci per-port msix support

[Tejun Heo]

On Mon, Nov 02, 2015 at 02:23:02PM -0500, Tejun Heo wrote:
> On Sun, Nov 01, 2015 at 01:43:35PM -0500, Dan Williams wrote:
> > Changes since v1 [1]:
> > 
> > 1/ Clean up "nvec = min(nvec, n_ports);" statement (Tejun)
> > 
> > 2/ Drop threaded interrupt handling option for now.
> > 
> > [1]: http://marc.info/?l=linux-ide&m=144623970817213&w=2
> 
> Applied to libata/for-4.4.

Dan, I got the following build failures from kbuild and reverted the
two patches from for-4.4.  It's too late for this merge window anyway.
Let's try the next one.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tj/libata for-4.4
head:   3758b0e5028c4b5083f2148ddc06b719d73e7627
commit: 6cb1c51aa65039ab3860c9741f9a190b7ab199cb [19/20] ahci: per-port msix support
config: mips-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout 6cb1c51aa65039ab3860c9741f9a190b7ab199cb
        # save the attached .config to linux build tree
        make.cross ARCH=mips

All errors (new ones prefixed by >>):

   drivers/ata/libahci.c: In function 'ahci_host_activate_multi_irqs':
>> drivers/ata/libahci.c:2492:4: error: invalid use of undefined type 'struct msix_entry'
       irq = hpriv->msix[i].vector;
       ^
>> drivers/ata/libahci.c:2492:21: error: dereferencing pointer to incomplete type 'struct msix_entry'
       irq = hpriv->msix[i].vector;
                        ^

vim +2492 drivers/ata/libahci.c

  2486           */
  2487          for (i = 0; i < host->n_ports; i++) {
  2488                  struct ahci_port_priv *pp = host->ports[i]->private_data;
  2489                  int irq;
  2490
  2491                  if (hpriv->flags & AHCI_HFLAG_MULTI_MSIX)
> 2492                          irq = hpriv->msix[i].vector;
  2493                  else
  2494                          irq = hpriv->irq + i;
  2495

-- 
tejun

Re: [PATCH v2 0/2] ahci per-port msix support

[Dan Williams]

On Tue, Nov 3, 2015 at 2:32 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Nov 02, 2015 at 02:23:02PM -0500, Tejun Heo wrote:
>> On Sun, Nov 01, 2015 at 01:43:35PM -0500, Dan Williams wrote:
>> > Changes since v1 [1]:
>> >
>> > 1/ Clean up "nvec = min(nvec, n_ports);" statement (Tejun)
>> >
>> > 2/ Drop threaded interrupt handling option for now.
>> >
>> > [1]: http://marc.info/?l=linux-ide&m=144623970817213&w=2
>>
>> Applied to libata/for-4.4.
>
> Dan, I got the following build failures from kbuild and reverted the
> two patches from for-4.4.  It's too late for this merge window anyway.
> Let's try the next one.
>

Ok, I'll resend after -rc1 is sent out.