Re: sata_mv query

Re: sata_mv query

[Mark Lord]

On 12-08-07 10:54 AM, Barry J Sturgeon wrote:
> when loading the 3.2.1. kernel onto my supermicro pdsm4 motherboard (with
> marvell 8 port sata controller) i have encountered performance degradation
> due to excessive cpu usage (i.e. three busy kworkers). after some
> investigation i have tracked down the problem to:
> 
> +static void mv_wait_for_edma_empty_idle(struct ata_port *ap)
> +{
> +       void __iomem *port_mmio = mv_ap_base(ap);
> +       const u32 empty_idle = (edma_status_cache_empty | edma_status_idle);
> +       const int per_loop = 5, timeout = (15 * 1000 / per_loop);
> +       int i;
> +
> +       /*
> +        * wait for the edma engine to finish transactions in progress.
> +        */
> +       for (i = 0; i < timeout; ++i) {
> +               u32 edma_stat = readl(port_mmio + edma_status_ofs);
> +               if ((edma_stat & empty_idle) == empty_idle)
> +                       break;
> *+               udelay(per_loop);*
> +       }
> +       /* ata_port_printk(ap, kern_info, "%s: %u+ usecs\n", __func__, i); */
> +}
>  
> it appears that we always reach the timeout value.
> note that empty_idle = 0xc0 (as you know),
> but most edma_stat values are (1100 or 1000).

Okay, that's weird.
mv_wait_for_edma_empty_idle() is called from mv_stop_edma(),
which happens whenever we transition between sending NCQ commands
(eg. read/write) and non-NCQ commands (eg. flush cache).

The mv_qc_defer() function is supposed to ensure that things are
more or less idle before it even gets to mv_stop_edma().
So we'd expect the mv_wait_for_edma_empty_idle() function to be quick,
which is a big part of why we're willing to tolerate an inline busy-wait.

But here it seems to be waiting MUCH longer, as if it has to wait for
some IO-in-progress to complete before continuing.  Which means that
the mv_qc_defer() function isn't being as effective as it should be.

The idea is, before any new command is queued, libata invokes the defer
function to see if the driver is ready to accept the new command.
The sata_mv driver normally says "yes" (go ahead) for a new NCQ command
whenever it currently has existing NCQ commands in-flight.
Otherwise it always says "no" (please defer) unless completely idle.
That's how mv_qc_defer() is supposed to work, and that should minimize
the time spent inside mv_wait_for_edma_empty_idle() by ensuring the
edma engine is already idle whenever that gets called.

This in turn suggests two possibilities:
(1) perhaps ap->nr_active_links has an accounting glitch somewhere. Or
(2) maybe there's a locking bug somewhere around the invocation
of mv_qc_defer().

Or something else is wrong.  :)

So, focus your attentions on mv_qc_defer().
Also, you could try mounting the filesystem with the "barrier=0" option,
just to see if that makes the problem go away -- confirming the analysis above.
-- 
mark lord
real-time remedies inc.
mlord@pobox.com

Re: sata_mv query

[Mark Lord]

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

On 12-08-07 01:16 PM, Mark Lord wrote:
..
> mv_wait_for_edma_empty_idle() is called from mv_stop_edma(),
> which happens whenever we transition between sending NCQ commands
> (eg. read/write) and non-NCQ commands (eg. flush cache).
> 
> The mv_qc_defer() function is supposed to ensure that things are
> more or less idle before it even gets to mv_stop_edma().
> So we'd expect the mv_wait_for_edma_empty_idle() function to be quick,
> which is a big part of why we're willing to tolerate an inline busy-wait.
> 
> But here it seems to be waiting MUCH longer, as if it has to wait for
> some IO-in-progress to complete before continuing.  Which means that
> the mv_qc_defer() function isn't being as effective as it should be.
> 
> The idea is, before any new command is queued, libata invokes the defer
> function to see if the driver is ready to accept the new command.
> The sata_mv driver normally says "yes" (go ahead) for a new NCQ command
> whenever it currently has existing NCQ commands in-flight.
> Otherwise it always says "no" (please defer) unless completely idle.
> That's how mv_qc_defer() is supposed to work, and that should minimize
> the time spent inside mv_wait_for_edma_empty_idle() by ensuring the
> edma engine is already idle whenever that gets called.

Say, here's a thought:  We could get rid of (or relocate to upper layers)
the busy-wait by having something similar get called from ata_qc_defer().

If you are feeling adventurous, here is a 100% untested patch to do just that.
It looks correct to me, it compiles, but that's all I can say.

The attached copy is the better one to use -- my mailer mangles inline patches.


--- linux-3.4.4/drivers/ata/sata_mv.c	2012-06-22 14:37:50.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2012-08-07 14:14:43.554503157 -0400
@@ -1388,6 +1388,17 @@
 	}
 }

+static int mv_check_for_edma_empty_idle(struct ata_port *ap)
+{
+	void __iomem *port_mmio = mv_ap_base(ap);
+	const u32 empty_idle = (EDMA_STATUS_CACHE_EMPTY | EDMA_STATUS_IDLE);
+
+	u32 edma_stat = readl(port_mmio + EDMA_STATUS);
+	if ((edma_stat & empty_idle) == empty_idle)
+		return ATA_DEFER_PORT;
+	return 0;
+}
+
 static int mv_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
@@ -1423,7 +1434,7 @@
 	 * If the port is completely idle, then allow the new qc.
 	 */
 	if (ap->nr_active_links == 0)
-		return 0;
+		return mv_check_for_edma_empty_idle(ap);

 	/*
 	 * The port is operating in host queuing mode (EDMA) with NCQ

[-- Attachment #2: xx_sata_mv_move_busywait.patch --]
[-- Type: text/x-patch, Size: 838 bytes --]

--- linux-3.4.4/drivers/ata/sata_mv.c	2012-06-22 14:37:50.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2012-08-07 14:14:43.554503157 -0400
@@ -1388,6 +1388,17 @@
 	}
 }
 
+static int mv_check_for_edma_empty_idle(struct ata_port *ap)
+{
+	void __iomem *port_mmio = mv_ap_base(ap);
+	const u32 empty_idle = (EDMA_STATUS_CACHE_EMPTY | EDMA_STATUS_IDLE);
+
+	u32 edma_stat = readl(port_mmio + EDMA_STATUS);
+	if ((edma_stat & empty_idle) == empty_idle)
+		return ATA_DEFER_PORT;
+	return 0;
+}
+
 static int mv_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
@@ -1423,7 +1434,7 @@
 	 * If the port is completely idle, then allow the new qc.
 	 */
 	if (ap->nr_active_links == 0)
-		return 0;
+		return mv_check_for_edma_empty_idle(ap);
 
 	/*
 	 * The port is operating in host queuing mode (EDMA) with NCQ

Re: sata_mv query

[Mark Lord]

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

On 12-08-07 02:17 PM, Mark Lord wrote:
..
> Say, here's a thought:  We could get rid of (or relocate to upper layers)
> the busy-wait by having something similar get called from ata_qc_defer().
> 
> If you are feeling adventurous, here is a 100% untested patch to do just that.
> It looks correct to me, it compiles, but that's all I can say.

Whoops.. return codes were reversed.
Here's the corrected patch.

The attached copy is the better one to use -- my mailer mangles inline patches.

--- linux-3.4.4/drivers/ata/sata_mv.c	2012-06-22 14:37:50.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2012-08-07 14:14:43.554503157 -0400
@@ -1388,6 +1388,17 @@
 	}
 }

+static int mv_check_for_edma_empty_idle(struct ata_port *ap)
+{
+	void __iomem *port_mmio = mv_ap_base(ap);
+	const u32 empty_idle = (EDMA_STATUS_CACHE_EMPTY | EDMA_STATUS_IDLE);
+
+	u32 edma_stat = readl(port_mmio + EDMA_STATUS);
+	if ((edma_stat & empty_idle) == empty_idle)
+		return 0;
+	return ATA_DEFER_PORT;
+}
+
 static int mv_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
@@ -1423,7 +1434,7 @@
 	 * If the port is completely idle, then allow the new qc.
 	 */
 	if (ap->nr_active_links == 0)
-		return 0;
+		return mv_check_for_edma_empty_idle(ap);

 	/*
 	 * The port is operating in host queuing mode (EDMA) with NCQ

[-- Attachment #2: xx_sata_mv_move_busywait.patch --]
[-- Type: text/x-patch, Size: 838 bytes --]

--- linux-3.4.4/drivers/ata/sata_mv.c	2012-06-22 14:37:50.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2012-08-07 14:14:43.554503157 -0400
@@ -1388,6 +1388,17 @@
 	}
 }
 
+static int mv_check_for_edma_empty_idle(struct ata_port *ap)
+{
+	void __iomem *port_mmio = mv_ap_base(ap);
+	const u32 empty_idle = (EDMA_STATUS_CACHE_EMPTY | EDMA_STATUS_IDLE);
+
+	u32 edma_stat = readl(port_mmio + EDMA_STATUS);
+	if ((edma_stat & empty_idle) == empty_idle)
+		return 0;
+	return ATA_DEFER_PORT;
+}
+
 static int mv_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
@@ -1423,7 +1434,7 @@
 	 * If the port is completely idle, then allow the new qc.
 	 */
 	if (ap->nr_active_links == 0)
-		return 0;
+		return mv_check_for_edma_empty_idle(ap);
 
 	/*
 	 * The port is operating in host queuing mode (EDMA) with NCQ