In 2009, While running "cache read" performance test of drives behind
SII PMP we encountered a "all 5 drives" timeout on more than 30% of the
machines under test. This patch reduces the rate by a factor of about 70.
Low enough that we didn't care to further investigate the issue.
Performance impact with any sort of "normal" use was ~2%+ CPU and less
than 1% throughput degradation. Worst case impact (cached read) was
6% IOPS reduction. This is with NCQ off (q=1) but I believe FIS based
switching enabled in the SATA driver.
The patch disables "Early ACK" in the 3726 port multiplier.
"Early ACK" is issued when device sends a FIS to the host (via PMP)
and the PMP sends an ACK immediately back to the device - well before
the host gets the response. Under worst case IOPs load (cached read
test) and more than 2 PMPs connected to a 4-port SATA controller,
I suspect the time to service all of the PMPs is exceeding the PMPs
ability to keep track of outstanding FIS it owes the Host. Reducing
the number of PMPs to 2 (or 1) reduces the frequency by several orders
of magnitude. Kudos to Gwendal for initial debugging of this issue.
[Any errors in the description are mine, not his.]
Patch is currently in production on Google servers.
Signed-off-by: Grant Grundler <[email protected]>
Signed-off-by: Gwendal Grignou <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
v2: dropped references to 4726 since I didn't test 4726,
moved register definition directly into libata-pmp.c, and
expanded the comment in the code to summarize the above description.
Code below is white space mangled. Please use attached file.
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index 00305f4..487227a 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -231,10 +231,14 @@ static const char *sata_pmp_spec_rev_str(const u32 *gscr)
return "<unknown>";
}
+#define PMP_GSCR_SII_POL 129
+
static int sata_pmp_configure(struct ata_device *dev, int print_info)
{
struct ata_port *ap = dev->link->ap;
u32 *gscr = dev->gscr;
+ u16 vendor = sata_pmp_gscr_vendor(gscr);
+ u16 devid = sata_pmp_gscr_devid(gscr);
unsigned int err_mask = 0;
const char *reason;
int nr_ports, rc;
@@ -260,12 +264,34 @@ static int sata_pmp_configure(struct ata_device
*dev, int print_info)
goto fail;
}
+ /* Disable sending Early R_OK.
+ * With "cached read" HDD testing and multiple ports busy on a SATA
+ * host controller, 3726 PMP will very rarely drop a deferred
+ * R_OK that was intended for the host. Symptom will be all
+ * 5 drives under test will timeout, get reset, and recover.
+ */
+ if (vendor == 0x1095 && devid == 0x3726) {
+ u32 reg;
+
+ err_mask = sata_pmp_read(&ap->link, PMP_GSCR_SII_POL, ®);
+ if (err_mask) {
+ rc = -EIO;
+ reason = "failed to read Sil3726 Private Register";
+ goto fail;
+ }
+ reg &= ~0x1;
+ err_mask = sata_pmp_write(&ap->link, PMP_GSCR_SII_POL, reg);
+ if (err_mask) {
+ rc = -EIO;
+ reason = "failed to write Sil3726 Private Register";
+ goto fail;
+ }
+ }
+
if (print_info) {
ata_dev_printk(dev, KERN_INFO, "Port Multiplier %s, "
"0x%04x:0x%04x r%d, %d ports, feat 0x%x/0x%x\n",
- sata_pmp_spec_rev_str(gscr),
- sata_pmp_gscr_vendor(gscr),
- sata_pmp_gscr_devid(gscr),
+ sata_pmp_spec_rev_str(gscr), vendor, devid,
sata_pmp_gscr_rev(gscr),
nr_ports, gscr[SATA_PMP_GSCR_FEAT_EN],
gscr[SATA_PMP_GSCR_FEAT]);
ping? Can this patch please be applied?
I was assuming this will show up here:
http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=shortlog;h=refs/heads/upstream-linus
thanks,
grant
On Wed, Apr 14, 2010 at 6:43 PM, Grant Grundler <[email protected]> wrote:
> In 2009, While running "cache read" performance test of drives behind
> SII PMP we encountered a "all 5 drives" timeout on more than 30% of the
> machines under test. This patch reduces the rate by a factor of about 70.
> Low enough that we didn't care to further investigate the issue.
>
> Performance impact with any sort of "normal" use was ~2%+ CPU and less
> than 1% throughput degradation. Worst case impact (cached read) was
> 6% IOPS reduction. This is with NCQ off (q=1) but I believe FIS based
> switching enabled in the SATA driver.
>
> The patch disables "Early ACK" in the 3726 port multiplier.
> "Early ACK" is issued when device sends a FIS to the host (via PMP)
> and the PMP sends an ACK immediately back to the device - well before
> the host gets the response. Under worst case IOPs load (cached read
> test) and more than 2 PMPs connected to a 4-port SATA controller,
> I suspect the time to service all of the PMPs is exceeding the PMPs
> ability to keep track of outstanding FIS it owes the Host. Reducing
> the number of PMPs to 2 (or 1) reduces the frequency by several orders
> of magnitude. Kudos to Gwendal for initial debugging of this issue.
> [Any errors in the description are mine, not his.]
>
> Patch is currently in production on Google servers.
>
> Signed-off-by: Grant Grundler <[email protected]>
> Signed-off-by: Gwendal Grignou <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
>
> ---
>
> v2: dropped references to 4726 since I didn't test 4726,
> moved register definition directly into libata-pmp.c, and
> expanded the comment in the code to summarize the above description.
>
> Code below is white space mangled. Please use attached file.
>
> diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
> index 00305f4..487227a 100644
> --- a/drivers/ata/libata-pmp.c
> +++ b/drivers/ata/libata-pmp.c
> @@ -231,10 +231,14 @@ static const char *sata_pmp_spec_rev_str(const u32 *gscr)
> return "<unknown>";
> }
>
> +#define PMP_GSCR_SII_POL 129
> +
> static int sata_pmp_configure(struct ata_device *dev, int print_info)
> {
> struct ata_port *ap = dev->link->ap;
> u32 *gscr = dev->gscr;
> + u16 vendor = sata_pmp_gscr_vendor(gscr);
> + u16 devid = sata_pmp_gscr_devid(gscr);
> unsigned int err_mask = 0;
> const char *reason;
> int nr_ports, rc;
> @@ -260,12 +264,34 @@ static int sata_pmp_configure(struct ata_device
> *dev, int print_info)
> goto fail;
> }
>
> + /* Disable sending Early R_OK.
> + * With "cached read" HDD testing and multiple ports busy on a SATA
> + * host controller, 3726 PMP will very rarely drop a deferred
> + * R_OK that was intended for the host. Symptom will be all
> + * 5 drives under test will timeout, get reset, and recover.
> + */
> + if (vendor == 0x1095 && devid == 0x3726) {
> + u32 reg;
> +
> + err_mask = sata_pmp_read(&ap->link, PMP_GSCR_SII_POL, ®);
> + if (err_mask) {
> + rc = -EIO;
> + reason = "failed to read Sil3726 Private Register";
> + goto fail;
> + }
> + reg &= ~0x1;
> + err_mask = sata_pmp_write(&ap->link, PMP_GSCR_SII_POL, reg);
> + if (err_mask) {
> + rc = -EIO;
> + reason = "failed to write Sil3726 Private Register";
> + goto fail;
> + }
> + }
> +
> if (print_info) {
> ata_dev_printk(dev, KERN_INFO, "Port Multiplier %s, "
> "0x%04x:0x%04x r%d, %d ports, feat 0x%x/0x%x\n",
> - sata_pmp_spec_rev_str(gscr),
> - sata_pmp_gscr_vendor(gscr),
> - sata_pmp_gscr_devid(gscr),
> + sata_pmp_spec_rev_str(gscr), vendor, devid,
> sata_pmp_gscr_rev(gscr),
> nr_ports, gscr[SATA_PMP_GSCR_FEAT_EN],
> gscr[SATA_PMP_GSCR_FEAT]);
>
Hello,
On 04/20/2010 10:52 PM, Grant Grundler wrote:
> ping? Can this patch please be applied?
>
> I was assuming this will show up here:
> http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=shortlog;h=refs/heads/upstream-linus
This one should be applied to libata-dev.git/upstream which will be
merged into mainline during the next merge window. I don't think
anything has changed in the PMP area since the merge window, so that
patch should apply to #upstream without too much problem (can't check
now). Jeff, can you please apply this to #upstream?
Thanks.
--
tejun
On 04/14/2010 09:43 PM, Grant Grundler wrote:
> In 2009, While running "cache read" performance test of drives behind
> SII PMP we encountered a "all 5 drives" timeout on more than 30% of the
> machines under test. This patch reduces the rate by a factor of about 70.
> Low enough that we didn't care to further investigate the issue.
>
> Performance impact with any sort of "normal" use was ~2%+ CPU and less
> than 1% throughput degradation. Worst case impact (cached read) was
> 6% IOPS reduction. This is with NCQ off (q=1) but I believe FIS based
> switching enabled in the SATA driver.
>
> The patch disables "Early ACK" in the 3726 port multiplier.
> "Early ACK" is issued when device sends a FIS to the host (via PMP)
> and the PMP sends an ACK immediately back to the device - well before
> the host gets the response. Under worst case IOPs load (cached read
> test) and more than 2 PMPs connected to a 4-port SATA controller,
> I suspect the time to service all of the PMPs is exceeding the PMPs
> ability to keep track of outstanding FIS it owes the Host. Reducing
> the number of PMPs to 2 (or 1) reduces the frequency by several orders
> of magnitude. Kudos to Gwendal for initial debugging of this issue.
> [Any errors in the description are mine, not his.]
>
> Patch is currently in production on Google servers.
>
> Signed-off-by: Grant Grundler<[email protected]>
> Signed-off-by: Gwendal Grignou<[email protected]>
> Acked-by: Tejun Heo<[email protected]>
>
> ---
>
> v2: dropped references to 4726 since I didn't test 4726,
> moved register definition directly into libata-pmp.c, and
> expanded the comment in the code to summarize the above description.
>
> Code below is white space mangled. Please use attached file.
applied manually, please figure out a way to send patches that can be
applied directly using the standard automated tools (== git am) that
everyone uses
On 04/20/2010 04:52 PM, Grant Grundler wrote:
> ping? Can this patch please be applied?
Apologies for the delay, I had been pulled away from libata this week by
baby stuff :/
> I was assuming this will show up here:
> http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=shortlog;h=refs/heads/upstream-linus
Nope, upstream-linus is always a temporary branch.
#upstream-fixes is where 2.6.X-rcY changes are found (though typically
these are pushed upstream ASAP)
#upstream is the development branch for 2.6.(X+1), where this change can
[now] be found.
Jeff
On Thu, Apr 22, 2010 at 7:16 PM, Jeff Garzik <[email protected]> wrote:
> On 04/20/2010 04:52 PM, Grant Grundler wrote:
>>
>> ping? Can this patch please be applied?
>
> Apologies for the delay, I had been pulled away from libata this week by
> baby stuff :/
No problem.
>
>
>> I was assuming this will show up here:
>>
>> http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=shortlog;h=refs/heads/upstream-linus
>
> Nope, upstream-linus is always a temporary branch.
>
> #upstream-fixes is where 2.6.X-rcY changes are found (though typically these
> are pushed upstream ASAP)
>
> #upstream is the development branch for 2.6.(X+1), where this change can
> [now] be found.
Ok! Thanks for pulling in the patch (manually) and explaining the branches.
cheers,
grant
>
> Jeff
>
>
>
>