Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Jeff Garzik wrote: Mark Lord wrote: Just say exactly what you require here. .. .. repost the whole thing .. .. Okay, will do in a couple of days. -ml - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Mark Lord wrote: Just say exactly what you require here. And keep in mind that any change I make incurs a 2-day wait while the Marvell folks vet it again (not my choice). (about to go to sleep, so the rest must wait) Bandwidth and mailing list archives are cheap, emailing is scriptable and its less confusion to simply discuss, revise, then repost the whole thing. Keeping track of the latest revision of each of 15 patches is an exercise in complexity management :) When its just one or two patches, replying with an updated "[PATCH v2] ..." generally works, but that doesn't scale well. (and, more practically, I made a guess that the patch series would need some revisions somewhere, assumed it would be reposted post-discussion, and deleted it locally after review) Jeff - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Mark Lord wrote:
Jeff Garzik wrote:
Mark Lord wrote:
..
static int __init mv_init(void)
{
+/* Ideally, a device (second parameter) would "own" these pools.
+ * But for maximum memory efficiency, we really need one global
+ * set of each, shared among all like devices. As below.
+ */
+mv_crqb_pool = dma_pool_create("mv_crqb_q", NULL, MV_CRQB_Q_SZ,
+MV_CRQB_Q_SZ, 0);
+mv_crpb_pool = dma_pool_create("mv_crpb_q", NULL, MV_CRPB_Q_SZ,
+MV_CRPB_Q_SZ, 0);
+mv_sg_tbl_pool = dma_pool_create("mv_sg_tbl", NULL, MV_SG_TBL_SZ,
+ MV_SG_TBL_SZ, 0);
Sorry, I would far rather waste a tiny bit of memory than this.
..
The waste amounts to perhaps half a page, per port.
The chips have either 4 or 8 ports.
But whatever. I think libata should actually provide the pools anyway,
since most drivers need hardware aligned DMA memory of very similar
requirements.
The pre-existing scheme in the driver is insufficient,
as it does not guarantee correct alignment. Right now it does
appear to work by accident, but there's no alignment guarantee
in the interface unless pools are used.
When we allocate 32 SG tables *per port*, this becomes much more of
a bother, so pools are needed there to avoid waste.
Having the pools per-port rather than per-chip multiplies any waste
by factors of 4 or 8. I prefer not wasting memory, myself.
..
Actually, it currently does one set of pools for the driver.
An only slightly worse solution might be to do one set of pools per chip,
as much of the time there's likely only a single chip anyway.
And per-chip means we'll have a struct device to pass to the pools interface.
So I'll change the driver to do it that way.
I can do this as a supplemental patch immediately, which will minimize
the need for retest/rebuild of other changes.
Or re-issue patches 10,11 as a single new patch, and possibly reissue
patches 12,13,14,15 but only if they no longer apply cleanly.
Just say exactly what you require here.
And keep in mind that any change I make incurs a 2-day wait
while the Marvell folks vet it again (not my choice).
Thanks
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Jeff Garzik wrote: .. The amount of memory used in the pre-existing configuration is small, it is already allocated on a per-device basis, and it is statically allocated -- meaning no worry about allocations failing inside of interrupts or similar nastiness. .. Note that there's also "no worry about allocations failing inside of interrupts or similar nastiness" with the patch posted, either. Cheers - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Jeff Garzik wrote:
Mark Lord wrote:
..
static int __init mv_init(void)
{
+/* Ideally, a device (second parameter) would "own" these pools.
+ * But for maximum memory efficiency, we really need one global
+ * set of each, shared among all like devices. As below.
+ */
+mv_crqb_pool = dma_pool_create("mv_crqb_q", NULL, MV_CRQB_Q_SZ,
+MV_CRQB_Q_SZ, 0);
+mv_crpb_pool = dma_pool_create("mv_crpb_q", NULL, MV_CRPB_Q_SZ,
+MV_CRPB_Q_SZ, 0);
+mv_sg_tbl_pool = dma_pool_create("mv_sg_tbl", NULL, MV_SG_TBL_SZ,
+ MV_SG_TBL_SZ, 0);
Sorry, I would far rather waste a tiny bit of memory than this.
..
The waste amounts to perhaps half a page, per port.
The chips have either 4 or 8 ports.
But whatever. I think libata should actually provide the pools anyway,
since most drivers need hardware aligned DMA memory of very similar
requirements.
The pre-existing scheme in the driver is insufficient,
as it does not guarantee correct alignment. Right now it does
appear to work by accident, but there's no alignment guarantee
in the interface unless pools are used.
When we allocate 32 SG tables *per port*, this becomes much more of
a bother, so pools are needed there to avoid waste.
Having the pools per-port rather than per-chip multiplies any waste
by factors of 4 or 8. I prefer not wasting memory, myself.
Cheers
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables
Mark Lord wrote:
sata_mv Use DMA memory pools for hardware memory tables.
Create module-owned DMA memory pools, for use in allocating/freeing
per-port
command/response queues and SG tables. This gives us a way to guarantee we
meet the hardware address alignment requirements, and also reduces
memory that
might otherwise be wasted on alignment gaps.
Signed-off-by: Mark Lord <[EMAIL PROTECTED]>
--- old/drivers/ata/sata_mv.c2008-01-24 12:40:10.0 -0500
+++ new/drivers/ata/sata_mv.c2008-01-24 14:17:39.0 -0500
@@ -107,14 +107,12 @@
/* CRQB needs alignment on a 1KB boundary. Size == 1KB
* CRPB needs alignment on a 256B boundary. Size == 256B
- * SG count of 176 leads to MV_PORT_PRIV_DMA_SZ == 4KB
* ePRD (SG) entries need alignment on a 16B boundary. Size == 16B
*/
MV_CRQB_Q_SZ= (32 * MV_MAX_Q_DEPTH),
MV_CRPB_Q_SZ= (8 * MV_MAX_Q_DEPTH),
-MV_MAX_SG_CT= 176,
+MV_MAX_SG_CT= 256,
MV_SG_TBL_SZ= (16 * MV_MAX_SG_CT),
-MV_PORT_PRIV_DMA_SZ= (MV_CRQB_Q_SZ + MV_CRPB_Q_SZ + MV_SG_TBL_SZ),
MV_PORTS_PER_HC= 4,
/* == (port / MV_PORTS_PER_HC) to determine HC from 0-7 port */
@@ -701,6 +699,33 @@
*/
static int msi; /* Use PCI msi; either zero (off, default) or
non-zero */
+/*
+ * These consistent DMA memory pools are owned by this module,
+ * and shared among all device instances. This gives us guaranteed
+ * alignment for hardware-accessed data structures, and low memory
+ * waste in accomplishing the alignment.
+ */
+static struct dma_pool *mv_crpb_pool;
+static struct dma_pool *mv_crqb_pool;
+static struct dma_pool *mv_sg_tbl_pool;
+
+static void mv_free_pool_items(struct ata_port *ap)
+{
+struct mv_port_priv *pp = ap->private_data;
+
+if (pp->sg_tbl) {
+dma_pool_free(mv_sg_tbl_pool, pp->sg_tbl, pp->sg_tbl_dma);
+pp->sg_tbl = NULL;
+}
+if (pp->crqb) {
+dma_pool_free(mv_crqb_pool, pp->crqb, pp->crqb_dma);
+pp->crpb = NULL;
+}
+if (pp->crqb) {
+dma_pool_free(mv_crqb_pool, pp->crqb, pp->crqb_dma);
+pp->crqb = NULL;
+}
+}
/* move to PCI layer or libata core? */
static int pci_go_64(struct pci_dev *pdev)
@@ -1110,8 +1135,6 @@
struct mv_host_priv *hpriv = ap->host->private_data;
struct mv_port_priv *pp;
void __iomem *port_mmio = mv_ap_base(ap);
-void *mem;
-dma_addr_t mem_dma;
unsigned long flags;
int rc;
@@ -1119,37 +1142,21 @@
if (!pp)
return -ENOMEM;
-mem = dmam_alloc_coherent(dev, MV_PORT_PRIV_DMA_SZ, &mem_dma,
- GFP_KERNEL);
-if (!mem)
-return -ENOMEM;
-memset(mem, 0, MV_PORT_PRIV_DMA_SZ);
-
rc = ata_pad_alloc(ap, dev);
if (rc)
return rc;
-/* First item in chunk of DMA memory:
- * 32-slot command request table (CRQB), 32 bytes each in size
- */
-pp->crqb = mem;
-pp->crqb_dma = mem_dma;
-mem += MV_CRQB_Q_SZ;
-mem_dma += MV_CRQB_Q_SZ;
-
-/* Second item:
- * 32-slot command response table (CRPB), 8 bytes each in size
- */
-pp->crpb = mem;
-pp->crpb_dma = mem_dma;
-mem += MV_CRPB_Q_SZ;
-mem_dma += MV_CRPB_Q_SZ;
-
-/* Third item:
- * Table of scatter-gather descriptors (ePRD), 16 bytes each
- */
-pp->sg_tbl = mem;
-pp->sg_tbl_dma = mem_dma;
+pp->crqb = dma_pool_alloc(mv_crqb_pool, GFP_KERNEL, &pp->crqb_dma);
+if (!pp->crqb)
+goto out_alloc_failed;
+
+pp->crpb = dma_pool_alloc(mv_crpb_pool, GFP_KERNEL, &pp->crpb_dma);
+if (!pp->crpb)
+goto out_alloc_failed;
+
+pp->sg_tbl = dma_pool_alloc(mv_sg_tbl_pool, GFP_KERNEL,
&pp->sg_tbl_dma);
+if (!pp->sg_tbl)
+goto out_alloc_failed;
spin_lock_irqsave(&ap->host->lock, flags);
@@ -1165,6 +1172,10 @@
*/
ap->private_data = pp;
return 0;
+
+out_alloc_failed:
+mv_free_pool_items(ap);
+return -ENOMEM;
}
/**
@@ -1179,6 +1190,7 @@
static void mv_port_stop(struct ata_port *ap)
{
mv_stop_dma(ap);
+mv_free_pool_items(ap);
}
/**
@@ -2825,14 +2837,39 @@
IS_GEN_I(hpriv) ? &mv5_sht : &mv6_sht);
}
+static void mv_destroy_pools(void)
+{
+if (mv_sg_tbl_pool)
+dma_pool_destroy(mv_sg_tbl_pool);
+if (mv_crpb_pool)
+dma_pool_destroy(mv_crpb_pool);
+if (mv_crqb_pool)
+dma_pool_destroy(mv_crqb_pool);
+}
+
static int __init mv_init(void)
{
+/* Ideally, a device (second parameter) would "own" these pools.
+ * But for maximum memory efficiency, we really need one global
+ * set of each, shared among all like devices. As below.
+ */
+mv_crqb_pool = dma_pool_create("mv_crqb_q", NULL, MV_CRQB_Q_SZ,
+MV_CRQB_Q_SZ, 0);
+mv_crpb_pool = dma_pool_create("mv_crpb_q", NULL, MV_CRPB_Q_SZ,
+MV_CRPB_Q_SZ, 0);
+mv_sg_tbl_pool = dma_pool_create("mv_sg_tbl", NULL, MV_SG_TBL_SZ,
+ MV_SG_TBL_SZ, 0);
Sorry, I woul
