RE: [PATCH V2 RESEND] fsl-sata: I/O load balancing

From: Liu Qiang-B32616
Date: Thu Feb 16 2012 - 20:55:39 EST

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@xxxxxxxxxxxxxxxxxxx]
> Sent: Friday, February 17, 2012 8:40 AM
> To: Liu Qiang-B32616
> Cc: jgarzik@xxxxxxxxx; linux-ide@xxxxxxxxxxxxxxx; linuxppc-
> dev@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 RESEND] fsl-sata: I/O load balancing
>
> On Wed, 2012-02-15 at 13:49 +0800, Qiang Liu wrote:
> ]
> > diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index
> > 0120b0d..d6577b9 100644
> > --- a/drivers/ata/sata_fsl.c
> > +++ b/drivers/ata/sata_fsl.c
> > @@ -6,7 +6,7 @@
> > * Author: Ashish Kalra <ashish.kalra@xxxxxxxxxxxxx>
> > * Li Yang <leoli@xxxxxxxxxxxxx>
> > *
> > - * Copyright (c) 2006-2007, 2011 Freescale Semiconductor, Inc.
> > + * Copyright (c) 2006-2007, 2011-2012 Freescale Semiconductor, Inc.
> > *
> > * This program is free software; you can redistribute it and/or
> modify it
> > * under the terms of the GNU General Public License as published
> > by the @@ -26,6 +26,15 @@ #include <asm/io.h> #include
> > <linux/of_platform.h>
> >
> > +static unsigned int intr_coalescing_count;
> > +module_param(intr_coalescing_count, int, S_IRUGO);
> > +MODULE_PARM_DESC(intr_coalescing_count,
> > + "INT coalescing count threshold (1..31)");
> > +
> > +static unsigned int intr_coalescing_ticks;
> > +module_param(intr_coalescing_ticks, int, S_IRUGO);
> > +MODULE_PARM_DESC(intr_coalescing_ticks,
> > + "INT coalescing timer threshold in AHB ticks");
>
> You don't seem to provide very useful defaults... for example,
> intr_coalescing_count starts at 0 which isn't even in range (the code
> fixes that up but still...).
>
> I tried a debian install on the p5020ds here and I find SATA to be
> extremely slow, generating millions of interrupts. I think the defaults
> should be a lot more aggressive at coalescing.

Hello Ben,

The default will be set in a common interface fsl_sata_set_irq_coalescing when
initialize the controller. This interface will check the range of intr count
and ticks and make sure the values are reasonably.

It's hard to find a aggressive value to adapt all scenarios, so I use echo to adjust
the value. I remember P5020 have some performance issue, I will check it.
BTW, which filesystem do you use? Ext2 is lower than ext4 because metadata is
continuously wrote to disk. You can try ext4 or xfs.

Thanks.

>
> Cheers,
> Ben.
>
> > /* Controller information */
> > enum {
> > SATA_FSL_QUEUE_DEPTH = 16,
> > @@ -83,6 +92,16 @@ enum {
> > };
> >
> > /*
> > + * Interrupt Coalescing Control Register bitdefs */ enum {
> > + ICC_MIN_INT_COUNT_THRESHOLD = 1,
> > + ICC_MAX_INT_COUNT_THRESHOLD = ((1 << 5) - 1),
> > + ICC_MIN_INT_TICKS_THRESHOLD = 0,
> > + ICC_MAX_INT_TICKS_THRESHOLD = ((1 << 19) - 1),
> > + ICC_SAFE_INT_TICKS = 1,
> > +};
> > +
> > +/*
> > * Host Controller command register set - per port */ enum { @@
> > -263,8 +282,65 @@ struct sata_fsl_host_priv {
> > void __iomem *csr_base;
> > int irq;
> > int data_snoop;
> > + struct device_attribute intr_coalescing;
> > };
> >
> > +static void fsl_sata_set_irq_coalescing(struct ata_host *host,
> > + unsigned int count, unsigned int ticks) {
> > + struct sata_fsl_host_priv *host_priv = host->private_data;
> > + void __iomem *hcr_base = host_priv->hcr_base;
> > +
> > + if (count > ICC_MAX_INT_COUNT_THRESHOLD)
> > + count = ICC_MAX_INT_COUNT_THRESHOLD;
> > + else if (count < ICC_MIN_INT_COUNT_THRESHOLD)
> > + count = ICC_MIN_INT_COUNT_THRESHOLD;
> > +
> > + if (ticks > ICC_MAX_INT_TICKS_THRESHOLD)
> > + ticks = ICC_MAX_INT_TICKS_THRESHOLD;
> > + else if ((ICC_MIN_INT_TICKS_THRESHOLD == ticks) &&
> > + (count > ICC_MIN_INT_COUNT_THRESHOLD))
> > + ticks = ICC_SAFE_INT_TICKS;
> > +
> > + spin_lock(&host->lock);
> > + iowrite32((count << 24 | ticks), hcr_base + ICC);
> > +
> > + intr_coalescing_count = count;
> > + intr_coalescing_ticks = ticks;
> > + spin_unlock(&host->lock);
> > +
> > + DPRINTK("intrrupt coalescing, count = 0x%x, ticks = %x\n",
> > + intr_coalescing_count, intr_coalescing_ticks);
> > + DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n",
> > + hcr_base, ioread32(hcr_base + ICC)); }
> > +
> > +static ssize_t fsl_sata_intr_coalescing_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + return sprintf(buf, "%d %d\n",
> > + intr_coalescing_count, intr_coalescing_ticks); }
> > +
> > +static ssize_t fsl_sata_intr_coalescing_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + unsigned int coalescing_count, coalescing_ticks;
> > +
> > + if (sscanf(buf, "%d%d",
> > + &coalescing_count,
> > + &coalescing_ticks) != 2) {
> > + printk(KERN_ERR "fsl-sata: wrong parameter format.\n");
> > + return -EINVAL;
> > + }
> > +
> > + fsl_sata_set_irq_coalescing(dev_get_drvdata(dev),
> > + coalescing_count, coalescing_ticks);
> > +
> > + return strlen(buf);
> > +}
> > +
> > static inline unsigned int sata_fsl_tag(unsigned int tag,
> > void __iomem *hcr_base)
> > {
> > @@ -346,10 +422,10 @@ static unsigned int sata_fsl_fill_sg(struct
> ata_queued_cmd *qc, void *cmd_desc,
> > (unsigned long long)sg_addr, sg_len);
> >
> > /* warn if each s/g element is not dword aligned */
> > - if (sg_addr & 0x03)
> > + if (unlikely(sg_addr & 0x03))
> > ata_port_err(qc->ap, "s/g addr unaligned : 0x%llx\n",
> > (unsigned long long)sg_addr);
> > - if (sg_len & 0x03)
> > + if (unlikely(sg_len & 0x03))
> > ata_port_err(qc->ap, "s/g len unaligned : 0x%x\n",
> > sg_len);
> >
> > @@ -1245,6 +1321,13 @@ static int sata_fsl_init_controller(struct
> ata_host *host)
> > iowrite32(0x00000FFFF, hcr_base + CE);
> > iowrite32(0x00000FFFF, hcr_base + DE);
> >
> > + /*
> > + * reset the number of command complete bits which will cause the
> > + * interrupt to be signaled
> > + */
> > + fsl_sata_set_irq_coalescing(host, intr_coalescing_count,
> > + intr_coalescing_ticks);
> > +
> > /*
> > * host controller will be brought on-line, during xx_port_start()
> > * callback, that should also initiate the OOB, COMINIT sequence @@
> > -1309,7 +1392,7 @@ static int sata_fsl_probe(struct platform_device
> *ofdev)
> > void __iomem *csr_base = NULL;
> > struct sata_fsl_host_priv *host_priv = NULL;
> > int irq;
> > - struct ata_host *host;
> > + struct ata_host *host = NULL;
> > u32 temp;
> >
> > struct ata_port_info pi = sata_fsl_port_info[0]; @@ -1356,6
> +1439,10
> > @@ static int sata_fsl_probe(struct platform_device *ofdev)
> >
> > /* allocate host structure */
> > host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS);
> > + if (!host) {
> > + retval = -ENOMEM;
> > + goto error_exit_with_cleanup;
> > + }
> >
> > /* host->iomap is not used currently */
> > host->private_data = host_priv;
> > @@ -1373,10 +1460,24 @@ static int sata_fsl_probe(struct
> > platform_device *ofdev)
> >
> > dev_set_drvdata(&ofdev->dev, host);
> >
> > + host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show;
> > + host_priv->intr_coalescing.store = fsl_sata_intr_coalescing_store;
> > + sysfs_attr_init(&host_priv->intr_coalescing.attr);
> > + host_priv->intr_coalescing.attr.name = "intr_coalescing";
> > + host_priv->intr_coalescing.attr.mode = S_IRUGO | S_IWUSR;
> > + retval = device_create_file(host->dev, &host_priv->intr_coalescing);
> > + if (retval)
> > + goto error_exit_with_cleanup;
> > +
> > return 0;
> >
> > error_exit_with_cleanup:
> >
> > + if (host) {
> > + dev_set_drvdata(&ofdev->dev, NULL);
> > + ata_host_detach(host);
> > + }
> > +
> > if (hcr_base)
> > iounmap(hcr_base);
> > if (host_priv)
> > @@ -1390,6 +1491,8 @@ static int sata_fsl_remove(struct platform_device
> *ofdev)
> > struct ata_host *host = dev_get_drvdata(&ofdev->dev);
> > struct sata_fsl_host_priv *host_priv = host->private_data;
> >
> > + device_remove_file(&ofdev->dev, &host_priv->intr_coalescing);
> > +
> > ata_host_detach(host);
> >
> > dev_set_drvdata(&ofdev->dev, NULL);
> > --
> > 1.6.4
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@xxxxxxxxxxxxxxxx
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>

N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ 0¶ìh®å’i