[PATCH RESEND v3 0/6] AHCI: Optimize interrupt processing

[PATCH RESEND v3 0/6] AHCI: Optimize interrupt processing

[Alexander Gordeev]

As per Tejun's feedback I am sending v3.

Changes since v2:
  - single patch split in a series;
  - benchmarking statistics reworded;
  - HOST_IRQ_STAT reg optimization added (patch 6);

Cc: linux-ide@vger.kernel.org

Alexander Gordeev (6):
  AHCI: Cleanup checking of multiple MSIs/SLM modes
  AHCI: Move host activation code into ahci_host_activate()
  AHCI: Make few function names more descriptive
  AHCI: Get rid of redundant arg to ahci_handle_port_interrupt()
  AHCI: Optimize single IRQ interrupt processing
  AHCI: Do not read HOST_IRQ_STAT reg in multi-MSI mode

 drivers/ata/acard-ahci.c       |   3 +-
 drivers/ata/ahci.c             | 101 +++++++++++++++++++++++------------
 drivers/ata/ahci.h             |  10 ++--
 drivers/ata/libahci.c          | 117 +++++++++++++++++------------------------
 drivers/ata/libahci_platform.c |   3 +-
 5 files changed, 123 insertions(+), 111 deletions(-)

-- 
1.8.3.1

[PATCH RESEND v3 1/6] AHCI: Cleanup checking of multiple MSIs/SLM modes

[Alexander Gordeev]

Sharing Last Message (SLM) mode is currently checked in two
functions: ahci_host_activate() and ahci_init_interrupts().
This update consolidates SLM mode check with activation of
multiple MSIs mode.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/ahci.c | 18 +++++++-----------
 drivers/ata/ahci.h |  2 +-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a0cc0ed..7a86b32 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1221,6 +1221,9 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 		goto single_msi;
 	}
 
+	if (nvec > 1)
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+
 	return nvec;
 
 single_msi:
@@ -1237,7 +1240,6 @@ intx:
  *	ahci_host_activate - start AHCI host, request IRQs and register it
  *	@host: target ATA host
  *	@irq: base IRQ number to request
- *	@n_msis: number of MSIs allocated for this host
  *	@irq_handler: irq_handler used when requesting IRQs
  *	@irq_flags: irq_flags used when requesting IRQs
  *
@@ -1251,14 +1253,10 @@ intx:
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis)
+int ahci_host_activate(struct ata_host *host, int irq)
 {
 	int i, rc;
 
-	/* Sharing Last Message among several ports is not supported */
-	if (n_msis < host->n_ports)
-		return -EINVAL;
-
 	rc = ata_host_start(host);
 	if (rc)
 		return rc;
@@ -1306,7 +1304,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
 	struct ata_host *host;
-	int n_ports, n_msis, i, rc;
+	int n_ports, i, rc;
 	int ahci_pci_bar = AHCI_PCI_BAR_STANDARD;
 
 	VPRINTK("ENTER\n");
@@ -1459,9 +1457,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
-	n_msis = ahci_init_interrupts(pdev, n_ports, hpriv);
-	if (n_msis > 1)
-		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+	ahci_init_interrupts(pdev, n_ports, hpriv);
 
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
@@ -1514,7 +1510,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	pci_set_master(pdev);
 
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
-		return ahci_host_activate(host, pdev->irq, n_msis);
+		return ahci_host_activate(host, pdev->irq);
 
 	return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
 				 &ahci_sht);
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 59ae0ee..a2a31af 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -392,7 +392,7 @@ irqreturn_t ahci_interrupt(int irq, void *dev_instance);
 irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance);
 irqreturn_t ahci_thread_fn(int irq, void *dev_instance);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
-int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
+int ahci_host_activate(struct ata_host *host, int irq);
 void ahci_error_handler(struct ata_port *ap);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
-- 
1.8.3.1

[PATCH RESEND v3 2/6] AHCI: Move host activation code into ahci_host_activate()

[Alexander Gordeev]

Currently host activation done by calling either function
ahci_host_activate() or ata_host_activate(). Consolidate
the code by only calling ahci_host_activate() for all AHCI
devices.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/acard-ahci.c       |  3 +--
 drivers/ata/ahci.c             | 59 +++++++++++++++++++++++++-----------------
 drivers/ata/ahci.h             |  3 ++-
 drivers/ata/libahci_platform.c |  3 +--
 4 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 25d0ac3..c962886 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -498,8 +498,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
 	acard_ahci_pci_print_info(host);
 
 	pci_set_master(pdev);
-	return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
-				 &acard_ahci_sht);
+	return ahci_host_activate(host, pdev->irq, &acard_ahci_sht);
 }
 
 module_pci_driver(acard_ahci_pci_driver);
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 7a86b32..7ce8e01 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1236,24 +1236,8 @@ intx:
 	return 0;
 }
 
-/**
- *	ahci_host_activate - start AHCI host, request IRQs and register it
- *	@host: target ATA host
- *	@irq: base IRQ number to request
- *	@irq_handler: irq_handler used when requesting IRQs
- *	@irq_flags: irq_flags used when requesting IRQs
- *
- *	Similar to ata_host_activate, but requests IRQs according to AHCI-1.1
- *	when multiple MSIs were allocated. That is one MSI per port, starting
- *	from @irq.
- *
- *	LOCKING:
- *	Inherited from calling layer (may sleep).
- *
- *	RETURNS:
- *	0 on success, -errno otherwise.
- */
-int ahci_host_activate(struct ata_host *host, int irq)
+static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
+					 struct scsi_host_template *sht)
 {
 	int i, rc;
 
@@ -1281,7 +1265,7 @@ int ahci_host_activate(struct ata_host *host, int irq)
 	for (i = 0; i < host->n_ports; i++)
 		ata_port_desc(host->ports[i], "irq %d", irq + i);
 
-	rc = ata_host_register(host, &ahci_sht);
+	rc = ata_host_register(host, sht);
 	if (rc)
 		goto out_free_all_irqs;
 
@@ -1296,6 +1280,37 @@ out_free_irqs:
 	return rc;
 }
 
+/**
+ *	ahci_host_activate - start AHCI host, request IRQs and register it
+ *	@host: target ATA host
+ *	@irq: base IRQ number to request
+ *	@sht: scsi_host_template to use when registering the host
+ *
+ *	Similar to ata_host_activate, but requests IRQs according to AHCI-1.1
+ *	when multiple MSIs were allocated. That is one MSI per port, starting
+ *	from @irq.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ahci_host_activate(struct ata_host *host, int irq,
+		       struct scsi_host_template *sht)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	int rc;
+
+	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
+		rc = ahci_host_activate_multi_irqs(host, irq, sht);
+	else
+		rc = ata_host_activate(host, irq, ahci_interrupt,
+				       IRQF_SHARED, sht);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_host_activate);
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	unsigned int board_id = ent->driver_data;
@@ -1509,11 +1524,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_master(pdev);
 
-	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
-		return ahci_host_activate(host, pdev->irq);
-
-	return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
-				 &ahci_sht);
+	return ahci_host_activate(host, pdev->irq, &ahci_sht);
 }
 
 module_pci_driver(ahci_pci_driver);
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index a2a31af..1a9955c 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -392,7 +392,8 @@ irqreturn_t ahci_interrupt(int irq, void *dev_instance);
 irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance);
 irqreturn_t ahci_thread_fn(int irq, void *dev_instance);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
-int ahci_host_activate(struct ata_host *host, int irq);
+int ahci_host_activate(struct ata_host *host, int irq,
+		       struct scsi_host_template *sht);
 void ahci_error_handler(struct ata_port *ap);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 5b92c29..a085224 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -495,8 +495,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	ahci_init_controller(host);
 	ahci_print_info(host, "platform");
 
-	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
-				 &ahci_platform_sht);
+	return ahci_host_activate(host, irq, &ahci_platform_sht);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_init_host);
 
-- 
1.8.3.1

[PATCH RESEND v3 3/6] AHCI: Make few function names more descriptive

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/ahci.c    |  7 +++----
 drivers/ata/ahci.h    |  6 +++---
 drivers/ata/libahci.c | 16 ++++++++--------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 7ce8e01..4a849f8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1255,8 +1255,8 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 		}
 
 		rc = devm_request_threaded_irq(host->dev, irq + i,
-					       ahci_hw_interrupt,
-					       ahci_thread_fn, IRQF_SHARED,
+					       ahci_multi_irqs_intr,
+					       ahci_port_thread_fn, IRQF_SHARED,
 					       pp->irq_desc, host->ports[i]);
 		if (rc)
 			goto out_free_irqs;
@@ -1305,9 +1305,8 @@ int ahci_host_activate(struct ata_host *host, int irq,
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
 		rc = ahci_host_activate_multi_irqs(host, irq, sht);
 	else
-		rc = ata_host_activate(host, irq, ahci_interrupt,
+		rc = ata_host_activate(host, irq, ahci_single_irq_intr,
 				       IRQF_SHARED, sht);
-	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_host_activate);
 
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 1a9955c..44c02f7 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -388,9 +388,9 @@ int ahci_port_resume(struct ata_port *ap);
 void ahci_set_em_messages(struct ahci_host_priv *hpriv,
 			  struct ata_port_info *pi);
 int ahci_reset_em(struct ata_host *host);
-irqreturn_t ahci_interrupt(int irq, void *dev_instance);
-irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance);
-irqreturn_t ahci_thread_fn(int irq, void *dev_instance);
+irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance);
+irqreturn_t ahci_multi_irqs_intr(int irq, void *dev_instance);
+irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, int irq,
 		       struct scsi_host_template *sht);
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b784e9d..e6ef3d4 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1789,7 +1789,7 @@ static void ahci_port_intr(struct ata_port *ap)
 	ahci_handle_port_interrupt(ap, port_mmio, status);
 }
 
-irqreturn_t ahci_thread_fn(int irq, void *dev_instance)
+irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 {
 	struct ata_port *ap = dev_instance;
 	struct ahci_port_priv *pp = ap->private_data;
@@ -1809,9 +1809,9 @@ irqreturn_t ahci_thread_fn(int irq, void *dev_instance)
 
 	return IRQ_HANDLED;
 }
-EXPORT_SYMBOL_GPL(ahci_thread_fn);
+EXPORT_SYMBOL_GPL(ahci_port_thread_fn);
 
-static void ahci_hw_port_interrupt(struct ata_port *ap)
+static void ahci_update_intr_status(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
 	struct ahci_port_priv *pp = ap->private_data;
@@ -1823,7 +1823,7 @@ static void ahci_hw_port_interrupt(struct ata_port *ap)
 	pp->intr_status |= status;
 }
 
-irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance)
+irqreturn_t ahci_multi_irqs_intr(int irq, void *dev_instance)
 {
 	struct ata_port *ap_this = dev_instance;
 	struct ahci_port_priv *pp = ap_this->private_data;
@@ -1859,7 +1859,7 @@ irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance)
 
 		ap = host->ports[i];
 		if (ap) {
-			ahci_hw_port_interrupt(ap);
+			ahci_update_intr_status(ap);
 			VPRINTK("port %u\n", i);
 		} else {
 			VPRINTK("port %u (no irq)\n", i);
@@ -1877,9 +1877,9 @@ irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance)
 
 	return IRQ_WAKE_THREAD;
 }
-EXPORT_SYMBOL_GPL(ahci_hw_interrupt);
+EXPORT_SYMBOL_GPL(ahci_multi_irqs_intr);
 
-irqreturn_t ahci_interrupt(int irq, void *dev_instance)
+irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance)
 {
 	struct ata_host *host = dev_instance;
 	struct ahci_host_priv *hpriv;
@@ -1938,7 +1938,7 @@ irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 
 	return IRQ_RETVAL(handled);
 }
-EXPORT_SYMBOL_GPL(ahci_interrupt);
+EXPORT_SYMBOL_GPL(ahci_single_irq_intr);
 
 unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 {
-- 
1.8.3.1

[PATCH RESEND v3 4/6] AHCI: Get rid of redundant arg to ahci_handle_port_interrupt()

[Alexander Gordeev]

Function's ahci_handle_port_interrupt() 'port_mmio'
parameter could be derived within function. No need
to pass it from outside.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/libahci.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index e6ef3d4..cbe7757 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1693,9 +1693,9 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 		ata_port_abort(ap);
 }
 
-static void ahci_handle_port_interrupt(struct ata_port *ap,
-				       void __iomem *port_mmio, u32 status)
+static void ahci_handle_port_interrupt(struct ata_port *ap, u32 status)
 {
+	void __iomem *port_mmio = ahci_port_base(ap);
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -1786,14 +1786,13 @@ static void ahci_port_intr(struct ata_port *ap)
 	status = readl(port_mmio + PORT_IRQ_STAT);
 	writel(status, port_mmio + PORT_IRQ_STAT);
 
-	ahci_handle_port_interrupt(ap, port_mmio, status);
+	ahci_handle_port_interrupt(ap, status);
 }
 
 irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 {
 	struct ata_port *ap = dev_instance;
 	struct ahci_port_priv *pp = ap->private_data;
-	void __iomem *port_mmio = ahci_port_base(ap);
 	unsigned long flags;
 	u32 status;
 
@@ -1804,7 +1803,7 @@ irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 	spin_unlock_irqrestore(&ap->host->lock, flags);
 
 	spin_lock_bh(ap->lock);
-	ahci_handle_port_interrupt(ap, port_mmio, status);
+	ahci_handle_port_interrupt(ap, status);
 	spin_unlock_bh(ap->lock);
 
 	return IRQ_HANDLED;
-- 
1.8.3.1

[PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

Split interrupt service routine into hardware context handler
and threaded context handler. That allows to protect ports with
individual locks rather than with a single host-wide lock and
move port interrupts handling out of the hardware interrupt
context.

Testing was done by transferring 8GB on two hard drives in
parallel using command 'dd if=/dev/sd{a,b} of=/dev/null'. With
lock_stat statistics I measured access times to ata_host::lock
spinlock (since interrupt handler code is fully embraced with
this lock). The average lock's holdtime decreased eight times
while average waittime decreased two times.

Both before and after the change the transfer time is the same,
while 'perf record -e cycles:k ...' shows 1%-4% CPU time spent
in ahci_single_irq_intr() routine before the update and not even
sampled/shown ahci_single_irq_intr() after the update.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/ahci.c    | 29 ++++++++++++++++++++++++++--
 drivers/ata/ahci.h    |  1 +
 drivers/ata/libahci.c | 53 ++++++++++++++++++++++++++++++++-------------------
 3 files changed, 61 insertions(+), 22 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 4a849f8..0a6d112 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1280,6 +1280,31 @@ out_free_irqs:
 	return rc;
 }
 
+static int ahci_host_activate_single_irq(struct ata_host *host, int irq,
+					 struct scsi_host_template *sht)
+{
+	int i, rc;
+
+	rc = ata_host_start(host);
+	if (rc)
+		return rc;
+
+	rc = devm_request_threaded_irq(host->dev, irq, ahci_single_irq_intr,
+				       ahci_thread_fn, IRQF_SHARED,
+				       dev_driver_string(host->dev), host);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < host->n_ports; i++)
+		ata_port_desc(host->ports[i], "irq %d", irq);
+
+	rc = ata_host_register(host, sht);
+	if (rc)
+		devm_free_irq(host->dev, irq, host);
+
+	return rc;
+}
+
 /**
  *	ahci_host_activate - start AHCI host, request IRQs and register it
  *	@host: target ATA host
@@ -1305,8 +1330,8 @@ int ahci_host_activate(struct ata_host *host, int irq,
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
 		rc = ahci_host_activate_multi_irqs(host, irq, sht);
 	else
-		rc = ata_host_activate(host, irq, ahci_single_irq_intr,
-				       IRQF_SHARED, sht);
+		rc = ahci_host_activate_single_irq(host, irq, sht);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_host_activate);
 
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 44c02f7..c12f590 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -390,6 +390,7 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv,
 int ahci_reset_em(struct ata_host *host);
 irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance);
 irqreturn_t ahci_multi_irqs_intr(int irq, void *dev_instance);
+irqreturn_t ahci_thread_fn(int irq, void *dev_instance);
 irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, int irq,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index cbe7757..169c272 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1778,17 +1778,6 @@ static void ahci_handle_port_interrupt(struct ata_port *ap, u32 status)
 	}
 }
 
-static void ahci_port_intr(struct ata_port *ap)
-{
-	void __iomem *port_mmio = ahci_port_base(ap);
-	u32 status;
-
-	status = readl(port_mmio + PORT_IRQ_STAT);
-	writel(status, port_mmio + PORT_IRQ_STAT);
-
-	ahci_handle_port_interrupt(ap, status);
-}
-
 irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 {
 	struct ata_port *ap = dev_instance;
@@ -1810,6 +1799,35 @@ irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 }
 EXPORT_SYMBOL_GPL(ahci_port_thread_fn);
 
+irqreturn_t ahci_thread_fn(int irq, void *dev_instance)
+{
+	struct ata_host *host = dev_instance;
+	struct ahci_host_priv *hpriv = host->private_data;
+	u32 irq_masked = hpriv->port_map;
+	unsigned int i;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap;
+
+		if (!(irq_masked & (1 << i)))
+			continue;
+
+		ap = host->ports[i];
+		if (ap) {
+			ahci_port_thread_fn(irq, ap);
+			VPRINTK("port %u\n", i);
+		} else {
+			VPRINTK("port %u (no irq)\n", i);
+			if (ata_ratelimit())
+				dev_warn(host->dev,
+					 "interrupt on disabled port %u\n", i);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL_GPL(ahci_thread_fn);
+
 static void ahci_update_intr_status(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
@@ -1908,7 +1926,7 @@ irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance)
 
 		ap = host->ports[i];
 		if (ap) {
-			ahci_port_intr(ap);
+			ahci_update_intr_status(ap);
 			VPRINTK("port %u\n", i);
 		} else {
 			VPRINTK("port %u (no irq)\n", i);
@@ -1935,7 +1953,7 @@ irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance)
 
 	VPRINTK("EXIT\n");
 
-	return IRQ_RETVAL(handled);
+	return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
 }
 EXPORT_SYMBOL_GPL(ahci_single_irq_intr);
 
@@ -2348,13 +2366,8 @@ static int ahci_port_start(struct ata_port *ap)
 	 */
 	pp->intr_mask = DEF_PORT_IRQ;
 
-	/*
-	 * Switch to per-port locking in case each port has its own MSI vector.
-	 */
-	if ((hpriv->flags & AHCI_HFLAG_MULTI_MSI)) {
-		spin_lock_init(&pp->lock);
-		ap->lock = &pp->lock;
-	}
+	spin_lock_init(&pp->lock);
+	ap->lock = &pp->lock;
 
 	ap->private_data = pp;
 
-- 
1.8.3.1

[PATCH RESEND v3 6/6] AHCI: Do not read HOST_IRQ_STAT reg in multi-MSI mode

[Alexander Gordeev]

As described in AHCI v1.0 specification chapter 10.6.2.2
"Multiple MSI Based Messages" generation of interrupts
is not controlled through the HOST_IRQ_STAT register.

Considering MMIO access is expensive remove unnecessary
reading and writing of HOST_IRQ_STAT register.

Further, serializing access to the host data is no longer
needed and the interrupt service routine can avoid competing
on the host lock.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Suggested-by: "Jiang, Dave" <dave.jiang@intel.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/ahci.h    |  2 ++
 drivers/ata/libahci.c | 59 ++++++++++++---------------------------------------
 2 files changed, 15 insertions(+), 46 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index c12f590..2476024 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -305,6 +305,8 @@ struct ahci_port_priv {
 	unsigned int		ncq_saw_dmas:1;
 	unsigned int		ncq_saw_sdb:1;
 	u32			intr_status;	/* interrupts to handle */
+	spinlock_t		*intr_lock;	/* interrupt context lock */
+	spinlock_t		__intr_lock;	/* protects intr_status */
 	spinlock_t		lock;		/* protects parent ata_port */
 	u32 			intr_mask;	/* interrupts to enable */
 	bool			fbs_supported;	/* set iff FBS is supported */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 169c272..beaeb76 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1785,11 +1785,11 @@ irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance)
 	unsigned long flags;
 	u32 status;
 
-	spin_lock_irqsave(&ap->host->lock, flags);
+	spin_lock_irqsave(pp->intr_lock, flags);
 	status = pp->intr_status;
 	if (status)
 		pp->intr_status = 0;
-	spin_unlock_irqrestore(&ap->host->lock, flags);
+	spin_unlock_irqrestore(pp->intr_lock, flags);
 
 	spin_lock_bh(ap->lock);
 	ahci_handle_port_interrupt(ap, status);
@@ -1842,53 +1842,14 @@ static void ahci_update_intr_status(struct ata_port *ap)
 
 irqreturn_t ahci_multi_irqs_intr(int irq, void *dev_instance)
 {
-	struct ata_port *ap_this = dev_instance;
-	struct ahci_port_priv *pp = ap_this->private_data;
-	struct ata_host *host = ap_this->host;
-	struct ahci_host_priv *hpriv = host->private_data;
-	void __iomem *mmio = hpriv->mmio;
-	unsigned int i;
-	u32 irq_stat, irq_masked;
+	struct ata_port *ap = dev_instance;
+	struct ahci_port_priv *pp = ap->private_data;
 
 	VPRINTK("ENTER\n");
 
-	spin_lock(&host->lock);
-
-	irq_stat = readl(mmio + HOST_IRQ_STAT);
-
-	if (!irq_stat) {
-		u32 status = pp->intr_status;
-
-		spin_unlock(&host->lock);
-
-		VPRINTK("EXIT\n");
-
-		return status ? IRQ_WAKE_THREAD : IRQ_NONE;
-	}
-
-	irq_masked = irq_stat & hpriv->port_map;
-
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
-
-		if (!(irq_masked & (1 << i)))
-			continue;
-
-		ap = host->ports[i];
-		if (ap) {
-			ahci_update_intr_status(ap);
-			VPRINTK("port %u\n", i);
-		} else {
-			VPRINTK("port %u (no irq)\n", i);
-			if (ata_ratelimit())
-				dev_warn(host->dev,
-					 "interrupt on disabled port %u\n", i);
-		}
-	}
-
-	writel(irq_stat, mmio + HOST_IRQ_STAT);
-
-	spin_unlock(&host->lock);
+	spin_lock(pp->intr_lock);
+	ahci_update_intr_status(ap);
+	spin_unlock(pp->intr_lock);
 
 	VPRINTK("EXIT\n");
 
@@ -2366,6 +2327,12 @@ static int ahci_port_start(struct ata_port *ap)
 	 */
 	pp->intr_mask = DEF_PORT_IRQ;
 
+	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
+		spin_lock_init(&pp->__intr_lock);
+		pp->intr_lock = &pp->__intr_lock;
+	} else {
+		pp->intr_lock = &ap->host->lock;
+	}
 	spin_lock_init(&pp->lock);
 	ap->lock = &pp->lock;
 
-- 
1.8.3.1

Re: [PATCH RESEND v3 1/6] AHCI: Cleanup checking of multiple MSIs/SLM modes

[Tejun Heo]

On Sun, Sep 21, 2014 at 03:19:24PM +0200, Alexander Gordeev wrote:
> Sharing Last Message (SLM) mode is currently checked in two
> functions: ahci_host_activate() and ahci_init_interrupts().
> This update consolidates SLM mode check with activation of
> multiple MSIs mode.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Cc: linux-ide@vger.kernel.org

Applied to libata/for-3.18.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 3/6] AHCI: Make few function names more descriptive

[Tejun Heo]

On Sun, Sep 21, 2014 at 03:19:26PM +0200, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Cc: linux-ide@vger.kernel.org
> ---
>  drivers/ata/ahci.c    |  7 +++----
>  drivers/ata/ahci.h    |  6 +++---
>  drivers/ata/libahci.c | 16 ++++++++--------
>  3 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 7ce8e01..4a849f8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
...
> @@ -1305,9 +1305,8 @@ int ahci_host_activate(struct ata_host *host, int irq,
>  	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
>  		rc = ahci_host_activate_multi_irqs(host, irq, sht);
>  	else
> -		rc = ata_host_activate(host, irq, ahci_interrupt,
> +		rc = ata_host_activate(host, irq, ahci_single_irq_intr,
>  				       IRQF_SHARED, sht);
> -	return rc;

Hmmm... why is return removed here?

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 2/6] AHCI: Move host activation code into ahci_host_activate()

[Tejun Heo]

On Sun, Sep 21, 2014 at 03:19:25PM +0200, Alexander Gordeev wrote:
> Currently host activation done by calling either function
> ahci_host_activate() or ata_host_activate(). Consolidate
> the code by only calling ahci_host_activate() for all AHCI
> devices.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Cc: linux-ide@vger.kernel.org

Applied to libata/for-3.18.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 2/6] AHCI: Move host activation code into ahci_host_activate()

[Tejun Heo]

On Tue, Sep 23, 2014 at 04:22:47PM -0400, Tejun Heo wrote:
> On Sun, Sep 21, 2014 at 03:19:25PM +0200, Alexander Gordeev wrote:
> > Currently host activation done by calling either function
> > ahci_host_activate() or ata_host_activate(). Consolidate
> > the code by only calling ahci_host_activate() for all AHCI
> > devices.
> > 
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > Cc: linux-ide@vger.kernel.org
> 
> Applied to libata/for-3.18.

Oops, build failure.  Reverting both patches.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

Hello, Alexander.

On Sun, Sep 21, 2014 at 03:19:28PM +0200, Alexander Gordeev wrote:
> Split interrupt service routine into hardware context handler
> and threaded context handler. That allows to protect ports with
> individual locks rather than with a single host-wide lock and
> move port interrupts handling out of the hardware interrupt
> context.
> 
> Testing was done by transferring 8GB on two hard drives in
> parallel using command 'dd if=/dev/sd{a,b} of=/dev/null'. With
> lock_stat statistics I measured access times to ata_host::lock
> spinlock (since interrupt handler code is fully embraced with
> this lock). The average lock's holdtime decreased eight times
> while average waittime decreased two times.
> 
> Both before and after the change the transfer time is the same,
> while 'perf record -e cycles:k ...' shows 1%-4% CPU time spent
> in ahci_single_irq_intr() routine before the update and not even
> sampled/shown ahci_single_irq_intr() after the update.

Hmmm... how does it affect single device operation tho?  It does make
individual interrupt handling heavier, no?

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Tue, Sep 23, 2014 at 04:57:10PM -0400, Tejun Heo wrote:
> Hmmm... how does it affect single device operation tho?  It does make
> individual interrupt handling heavier, no?

I think it is difficult to assess "individual interrupt handling", since
it depends from both the hardware and device access pattern. On the system
I use the results are rather counter-intuitive: ahci_thread_fn() does not
show up in perf report at all, nor ahci_single_irq_intr(). While before
the change ahci_single_irq_intr() reported 0.00%.

But since the handling is split in two parts it is rather incorrect to
apply the same metric to the threaded context. Obviously, the threaded
handler is expected slowed down by other interrupts handlers, but the
whole system should benefit from it, which is exactly the aim of this
change.

> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

Hello, Alexander.

On Wed, Sep 24, 2014 at 11:42:15AM +0100, Alexander Gordeev wrote:
> On Tue, Sep 23, 2014 at 04:57:10PM -0400, Tejun Heo wrote:
> > Hmmm... how does it affect single device operation tho?  It does make
> > individual interrupt handling heavier, no?
> 
> I think it is difficult to assess "individual interrupt handling", since
> it depends from both the hardware and device access pattern. On the system
> I use the results are rather counter-intuitive: ahci_thread_fn() does not
> show up in perf report at all, nor ahci_single_irq_intr(). While before
> the change ahci_single_irq_intr() reported 0.00%.
> 
> But since the handling is split in two parts it is rather incorrect to
> apply the same metric to the threaded context. Obviously, the threaded
> handler is expected slowed down by other interrupts handlers, but the
> whole system should benefit from it, which is exactly the aim of this
> change.

Hmmm, how would the whole system benefit from it if there's only
single device?  Each individual servicing of the interrupt does more
now which includes scheduling which may end up adding to completion
latency.

The thing I don't get is why multiple MSI handling and this patchset
are tied to threaded interrupt handling.  Splitting locks don't
necessarily have much to do with threaded handling and it's not like
ahci interrupt handling is heavy.  The hot path is pretty short
actually.  The meat of the work - completing requests and propagating
completions - is offloaded to softirq by block layer anyway.

Just to be clear, I'm not against the proposed changes but wanna
understand the justifications behind them.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Chuck Ebbert]

On Wed, 24 Sep 2014 09:04:44 -0400
Tejun Heo <tj@kernel.org> wrote:

> Hello, Alexander.
> 
> On Wed, Sep 24, 2014 at 11:42:15AM +0100, Alexander Gordeev wrote:
> > On Tue, Sep 23, 2014 at 04:57:10PM -0400, Tejun Heo wrote:
> > > Hmmm... how does it affect single device operation tho?  It does make
> > > individual interrupt handling heavier, no?
> > 
> > I think it is difficult to assess "individual interrupt handling", since
> > it depends from both the hardware and device access pattern. On the system
> > I use the results are rather counter-intuitive: ahci_thread_fn() does not
> > show up in perf report at all, nor ahci_single_irq_intr(). While before
> > the change ahci_single_irq_intr() reported 0.00%.
> > 
> > But since the handling is split in two parts it is rather incorrect to
> > apply the same metric to the threaded context. Obviously, the threaded
> > handler is expected slowed down by other interrupts handlers, but the
> > whole system should benefit from it, which is exactly the aim of this
> > change.
> 
> Hmmm, how would the whole system benefit from it if there's only
> single device?  Each individual servicing of the interrupt does more
> now which includes scheduling which may end up adding to completion
> latency.
> 

I think he meant other, non-AHCI, interrupt handlers would benefit. A
good test of this patch might be to stream 10Gb ethernet while also
streaming writes to an AHCI device.

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

On Wed, Sep 24, 2014 at 08:27:12AM -0500, Chuck Ebbert wrote:
> I think he meant other, non-AHCI, interrupt handlers would benefit. A
> good test of this patch might be to stream 10Gb ethernet while also
> streaming writes to an AHCI device.

I'm a bit doubtful this'd make a noticeable difference.  ahci
interrupt handler doesn't do that much to begin with.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Wed, Sep 24, 2014 at 09:04:44AM -0400, Tejun Heo wrote:
> Hello, Alexander.
> 
> On Wed, Sep 24, 2014 at 11:42:15AM +0100, Alexander Gordeev wrote:
> > On Tue, Sep 23, 2014 at 04:57:10PM -0400, Tejun Heo wrote:
> > > Hmmm... how does it affect single device operation tho?  It does make
> > > individual interrupt handling heavier, no?
> > 
> > I think it is difficult to assess "individual interrupt handling", since
> > it depends from both the hardware and device access pattern. On the system
> > I use the results are rather counter-intuitive: ahci_thread_fn() does not
> > show up in perf report at all, nor ahci_single_irq_intr(). While before
> > the change ahci_single_irq_intr() reported 0.00%.
> > 
> > But since the handling is split in two parts it is rather incorrect to
> > apply the same metric to the threaded context. Obviously, the threaded
> > handler is expected slowed down by other interrupts handlers, but the
> > whole system should benefit from it, which is exactly the aim of this
> > change.
> 
> Hmmm, how would the whole system benefit from it if there's only
> single device?  Each individual servicing of the interrupt does more
> now which includes scheduling which may end up adding to completion
> latency.

As Chuck noticed, non-AHCI hardware context handlers will benefit.

> The thing I don't get is why multiple MSI handling and this patchset
> are tied to threaded interrupt handling.

Multiple MSIs were implemented with the above aim (let's say aim #1)
right away. Single MSI/IRQ handling is getting updated with this series.

> Splitting locks don't
> necessarily have much to do with threaded handling and it's not like
> ahci interrupt handling is heavy.  The hot path is pretty short
> actually.  The meat of the work - completing requests and propagating
> completions - is offloaded to softirq by block layer anyway.

So the aim (let's say aim #2) is to avoid any of those to compete with
hardware context handler. IOW, not to wait on host/port spinlocks with
local interrupts disabled unnecessarily.

I assume, if at the time of writing of original handlers the two
interrupt context existed, they were written the way I propose now :)

> Just to be clear, I'm not against the proposed changes but wanna
> understand the justifications behind them.

Should I send the fixed series? ;)

> Thanks.
> 
> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

Hello, Alexander.

On Wed, Sep 24, 2014 at 03:08:44PM +0100, Alexander Gordeev wrote:
> > Hmmm, how would the whole system benefit from it if there's only
> > single device?  Each individual servicing of the interrupt does more
> > now which includes scheduling which may end up adding to completion
> > latency.
> 
> As Chuck noticed, non-AHCI hardware context handlers will benefit.

Maybe I'm off but I'm kinda skeptical that we'd be gaining back the
overhead we pay by punting to a thread.

> > The thing I don't get is why multiple MSI handling and this patchset
> > are tied to threaded interrupt handling.
> 
> Multiple MSIs were implemented with the above aim (let's say aim #1)
> right away. Single MSI/IRQ handling is getting updated with this series.

Yeah, I get that.  I'm curious whether that was justified.

> > Splitting locks don't
> > necessarily have much to do with threaded handling and it's not like
> > ahci interrupt handling is heavy.  The hot path is pretty short
> > actually.  The meat of the work - completing requests and propagating
> > completions - is offloaded to softirq by block layer anyway.
> 
> So the aim (let's say aim #2) is to avoid any of those to compete with
> hardware context handler. IOW, not to wait on host/port spinlocks with
> local interrupts disabled unnecessarily.
> 
> I assume, if at the time of writing of original handlers the two
> interrupt context existed, they were written the way I propose now :)

Maybe it makes sense with many high speed devices attached to a single
host; otherwise, I think we'd prolly be paying more than we're
gaining.  Lock splitting itself is likely beneficial as our issue path
is a lot heavier than completion path but I'm not too sure about
splitting completion contexts especially given that completion for
block layer and up are already punted to softirq.

Would it be possible for you compare threaded vs. unthreaded under
relatively heavy load?  ie. let the interrupt handler access irq
status under host lock but release it and then go through per-port
locks from the interrupt handler.

Thanks for doing this!

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Wed, Sep 24, 2014 at 10:39:13AM -0400, Tejun Heo wrote:
> Would it be possible for you compare threaded vs. unthreaded under
> relatively heavy load?

I will try, although not quite soon.

In the meantime I could fix and resend patches 1,2,3 and 6 as
they are not related to this topic. Makes sense?

> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

RE: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Tejun Heo
...
> The thing I don't get is why multiple MSI handling and this patchset
> are tied to threaded interrupt handling.  Splitting locks don't
> necessarily have much to do with threaded handling and it's not like
> ahci interrupt handling is heavy.  The hot path is pretty short
> actually.  The meat of the work - completing requests and propagating
> completions - is offloaded to softirq by block layer anyway.

blk-mq/scsi-mq chose to move all completion work into hardirq context,
so this seems headed in a different direction.

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

On Wed, Sep 24, 2014 at 03:59:07PM +0100, Alexander Gordeev wrote:
> On Wed, Sep 24, 2014 at 10:39:13AM -0400, Tejun Heo wrote:
> > Would it be possible for you compare threaded vs. unthreaded under
> > relatively heavy load?
> 
> I will try, although not quite soon.
> 
> In the meantime I could fix and resend patches 1,2,3 and 6 as
> they are not related to this topic. Makes sense?

Yeap, sure thing.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Wed, Sep 24, 2014 at 10:39:13AM -0400, Tejun Heo wrote:
> Hello, Alexander.
> 
> On Wed, Sep 24, 2014 at 03:08:44PM +0100, Alexander Gordeev wrote:
> > > Hmmm, how would the whole system benefit from it if there's only
> > > single device?  Each individual servicing of the interrupt does more
> > > now which includes scheduling which may end up adding to completion
> > > latency.
> > 
> > As Chuck noticed, non-AHCI hardware context handlers will benefit.
> 
> Maybe I'm off but I'm kinda skeptical that we'd be gaining back the
> overhead we pay by punting to a thread.

Hi Tejun,

As odd as it sounds, I did not mention there is *no* change in IO
performance at all (in my system): neither with one drive nor two.
The change is only about how the interrupt handlers co-exist with
other devices.

I am attaching excerpts from some new perf tests I have done (this
time in legacy interrupt mode). As you can notice, ahci_interrupt()
CPU time drops from 4% to none.

As of your concern wrt threaded handler invocation overhead - I am
not quite sure here, but if SCHED_FIFO policy (the handler runs with)
makes the difference? Anyway, as said above the overall IO does not
suffer.

> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

On Wed, Oct 01, 2014 at 04:31:14PM +0100, Alexander Gordeev wrote:
> I am attaching excerpts from some new perf tests I have done (this

Attaching :)

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[-- Attachment #2: perf.hist.0-3.16.0.txt --]
[-- Type: text/plain, Size: 1285 bytes --]

  0.04%     dd  [kernel.kallsyms]  [k] ata_std_qc_defer
  0.04%     dd  [kernel.kallsyms]  [k] ata_scsi_rw_xlat
  0.03%     dd  [kernel.kallsyms]  [k] ata_scsi_queuecmd
  0.03%     dd  [kernel.kallsyms]  [k] __ata_scsi_find_dev
  0.02%     dd  [kernel.kallsyms]  [k] ata_scsi_translate
  0.02%     dd  [kernel.kallsyms]  [k] ata_qc_new_init
  0.02%     dd  [kernel.kallsyms]  [k] ata_scsi_qc_complete
  0.02%     dd  [kernel.kallsyms]  [k] ata_build_rw_tf
  0.01%     dd  [kernel.kallsyms]  [k] ata_qc_issue
  0.01%     dd  [kernel.kallsyms]  [k] ata_sg_init
  0.00%     dd  [kernel.kallsyms]  [k] ata_tf_to_fis
  0.00%     dd  [kernel.kallsyms]  [k] ata_qc_complete_multiple
  0.00%     dd  [kernel.kallsyms]  [k] __ata_qc_complete
  0.00%     dd  [kernel.kallsyms]  [k] ata_qc_complete
  0.00%     dd  [kernel.kallsyms]  [k] ata_scsi_find_dev
  0.00%     dd  [kernel.kallsyms]  [k] ata_qc_free
  0.04%     dd  [kernel.kallsyms]  [k] ahci_interrupt
  0.02%     dd  [kernel.kallsyms]  [k] ahci_qc_issue
  0.02%     dd  [kernel.kallsyms]  [k] ahci_qc_prep
  0.02%     dd  [kernel.kallsyms]  [k] ahci_scr_read
  0.01%     dd  [kernel.kallsyms]  [k] ahci_handle_port_interrupt
  0.01%     dd  [kernel.kallsyms]  [k] ahci_pmp_qc_defer
  0.00%     dd  [kernel.kallsyms]  [k] ahci_scr_write

[-- Attachment #3: perf.hist.0-3.16.0.opt-intr.txt --]
[-- Type: text/plain, Size: 771 bytes --]

  0.04%     dd  [kernel.kallsyms]  [k] ata_scsi_queuecmd
  0.03%     dd  [kernel.kallsyms]  [k] ata_scsi_rw_xlat
  0.03%     dd  [kernel.kallsyms]  [k] ata_scsi_translate
  0.03%     dd  [kernel.kallsyms]  [k] ata_qc_new_init
  0.02%     dd  [kernel.kallsyms]  [k] ata_build_rw_tf
  0.02%     dd  [kernel.kallsyms]  [k] ata_qc_issue
  0.01%     dd  [kernel.kallsyms]  [k] ata_sg_init
  0.01%     dd  [kernel.kallsyms]  [k] ata_std_qc_defer
  0.01%     dd  [kernel.kallsyms]  [k] ata_tf_to_fis
  0.00%     dd  [kernel.kallsyms]  [k] ata_scsi_find_dev
  0.00%     dd  [kernel.kallsyms]  [k] __ata_scsi_find_dev
  0.02%     dd  [kernel.kallsyms]  [k] ahci_qc_prep
  0.02%     dd  [kernel.kallsyms]  [k] ahci_qc_issue
  0.01%     dd  [kernel.kallsyms]  [k] ahci_pmp_qc_defer

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

Hey, Alexander.

On Wed, Oct 01, 2014 at 04:31:15PM +0100, Alexander Gordeev wrote:
> As of your concern wrt threaded handler invocation overhead - I am
> not quite sure here, but if SCHED_FIFO policy (the handler runs with)
> makes the difference? Anyway, as said above the overall IO does not
> suffer.

Hmmm.... so, AFAICS, there's no real pros or cons of going either way,
right?  The only thing which could be different is possibly slightly
lower latency in servicing other IRQs or RT tasks on the same CPU but
given that the ahci IRQ handler already doesn't do anything which
takes time, I'm doubtful whether that'd be anything measureable.

I just don't get why ahci bothers with threaded irq, MMSI or not.

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

A bit of addition.

On Sat, Oct 04, 2014 at 10:23:11PM -0400, Tejun Heo wrote:
> Hmmm.... so, AFAICS, there's no real pros or cons of going either way,
> right?  The only thing which could be different is possibly slightly
> lower latency in servicing other IRQs or RT tasks on the same CPU but
> given that the ahci IRQ handler already doesn't do anything which
> takes time, I'm doubtful whether that'd be anything measureable.
> 
> I just don't get why ahci bothers with threaded irq, MMSI or not.

I think the thing which bothers me is that due to softirq we end up
bouncing the context twice.  IRQ schedules threaded IRQ handler after
doing minimal amount of work.  The threaded IRQ handler gets scheduled
and again it doesn't do much but basically just schedules block
softirq to actually run completions which is the heavier part.
Apparently this doesn't seem to hurt measureably but it's just weird.
Why are we bouncing the context twice?

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Sun, Oct 05, 2014 at 12:16:46PM -0400, Tejun Heo wrote:
> I think the thing which bothers me is that due to softirq we end up
> bouncing the context twice.  IRQ schedules threaded IRQ handler after
> doing minimal amount of work.  The threaded IRQ handler gets scheduled
> and again it doesn't do much but basically just schedules block
> softirq to actually run completions which is the heavier part.
> Apparently this doesn't seem to hurt measureably but it's just weird.

Hi Tejun,

That is exactly the point I was concerned with when stated in one of
changelogs "The downside of this change is introduction of a kernel
thread". Splitting the service routine in two parts is a small change
(in terms of code familiarity). Yet it right away provides benefits I
could observe and justify (to myself at least).

> Why are we bouncing the context twice?

I *did* consider moving the threaded handler code to the softirq part.
I just wanted to get updates in stages: to address hardware interrupts
latency first and possibly threaded hander next. Getting done these two
together would be too big change for me ;)

> -- 
> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

Hello, Alexander.

On Mon, Oct 06, 2014 at 08:27:11AM +0100, Alexander Gordeev wrote:
> > Why are we bouncing the context twice?
> 
> I *did* consider moving the threaded handler code to the softirq part.
> I just wanted to get updates in stages: to address hardware interrupts
> latency first and possibly threaded hander next. Getting done these two
> together would be too big change for me ;)

I don't think we'd be able to move libata handling to block softirq
and probably end up just doing it from the irq context.  Anyways, as
long as you're gonna keep working on it, I have no objection to the
proposed changes.  Do you have a refreshed version or is the current
version good for inclusion?

Thanks.

-- 
tejun

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Alexander Gordeev]

On Mon, Oct 06, 2014 at 08:58:17AM -0400, Tejun Heo wrote:
> I don't think we'd be able to move libata handling to block softirq
> and probably end up just doing it from the irq context.  Anyways, as
> long as you're gonna keep working on it, I have no objection to the
> proposed changes.  Do you have a refreshed version or is the current
> version good for inclusion?

No, this one would not apply. I can send updated version on top of
v5 I posted earlier. Should I?

> tejun

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH RESEND v3 5/6] AHCI: Optimize single IRQ interrupt processing

[Tejun Heo]

On Mon, Oct 06, 2014 at 02:24:46PM +0100, Alexander Gordeev wrote:
> On Mon, Oct 06, 2014 at 08:58:17AM -0400, Tejun Heo wrote:
> > I don't think we'd be able to move libata handling to block softirq
> > and probably end up just doing it from the irq context.  Anyways, as
> > long as you're gonna keep working on it, I have no objection to the
> > proposed changes.  Do you have a refreshed version or is the current
> > version good for inclusion?
> 
> No, this one would not apply. I can send updated version on top of
> v5 I posted earlier. Should I?

Yeap, please do so.

Thanks a lot for your patience!  :)

-- 
tejun