generic device DMA implementation

I think you saw that patch to let the new 2.5.53 generic dma code replace one of the two indirections USB needs. Here are some of the key open issues I'm thinking of:

- DMA mapping calls still return no errors; so BUG() out instead?

Consider systems where DMA-able memory is limited (like SA-1111, to 1 MByte); clearly it should be possible for these calls to fail, when they can't allocate a bounce buffer. Or (see below) when an invalid argument is provided to a dma mapping call.

Fix by defining fault returns for the current signatures, starting with the api specs:

* dma_map_sg() returns negative errno (or zero?) when it fails. (Those are illegal sglist lengths.)

* dma_map_single() returns an arch-specific value, like DMA_ADDR_INVALID, when it fails. (DaveM's suggestion, from a while back; it's seemingly arch-specific.)

Yes, the PCI dma calls would benefit from those bugfixes too.

- Implementation-wise, I'm rather surprised that the generic version doesn't just add new bus driver methods rather than still insisting everything be PCI underneath.

It's not clear to me how I'd make, for example, a USB device or interface work with dma_map_sg() ... those "generic" calls are going to fail (except on x86, where all memory is DMA-able) since USB != PCI. Even when usb_buffer_map_sg() would succeed. (The second indirection: the usb controller hardware does the mapping, not the device or hcd. That's usually PCI.)

Hmm, I suppose there'd need to be a default implementation of the mapping operations (for all non-pci busses) that'd fail cleanly ... :)

- There's no analogue to pci_pool, and there's nothing like "kmalloc" (likely built from N dma-coherent pools).

That forces drivers to write and maintain memory allocators, is a waste of energy as well as being bug-prone. So in that sense this API isn't a complete functional replacement of the current PCI (has pools, ~= kmem_cache_t) or USB (with simpler buffer_alloc ~= kmalloc) APIs for dma.

- The API says drivers "must" satisfy dma_get_cache_alignment(), yet both implementations, asm-{i386,generic}/dma-mapping.h, ignore that rule.

Are you certain of that rule, for all cache coherency models? I thought only some machines (with dma-incoherent caches) had that as a hard constraint. (Otherwise it's a soft one: even if there's cacheline contention, the hardware won't lose data when drivers use memory barriers correctly.)

I expect that combination is likely to be problematic, since the previous rule has been (wrongly?) that kmalloc or kmem_cache memory is fine for DMA mappings, no size restrictions. Yet for one example on x86 dma_get_cache_alignment() returns 128 bytes, but kmalloc has several smaller pool sizes ... and lately will align to L1_CACHE_BYTES (wasting memory on small allocs?) even when that's smaller than L1_CACHE_MAX (in the new dma calls).

All the more reason to have a drop-in kmalloc alternative for dma-aware code to use, handling such details transparently!

- Isn't arch/i386/kernel/pci-dma.c handling DMA masks wrong? It's passing GFP_DMA in cases where GFP_HIGHMEM is correct ...

I'm glad to see progress on making DMA more generic, thanks!

- Dave

You mean dma-mapping.h in asm-generic? The reason for that is to provide an implementation that functions now for non x86 (and non-parisc) archs without having to write specific code for them all. Since all the other arch's now function in terms of the pci_ API, that was the only way of sliding the dma_ API in without breaking them all.

Bus driver methods have been advocated before, but it's not clear to me that they should be exposed in the *generic* API.

Actually, they should work on parisc out of the box as well because of the way its DMA implementation is built in terms of the generic dma_ API.

As far as implementing this generically, just adding a case for the usb_bus_type in asm-generic/dma-mapping.h will probably get you where you need to be. (the asm-generic is, after all, only intended as a stopgap. Fully coherent platforms with no IOMMUs will probably take the x86 route to implementing the dma_ API, platforms with IOMMUs will probably (eventually) do similar things to parisc).

Could you clarify this a little. I tend to think of "mapping" as something done by the IO MMU managing the bus. I think you mean that the usb controller will mark a region of memory to be accessed by the device. If such a region were also "mapped" by an IOMMU, it would be done outside the control of the USB controller, correct? (the IOMMU would translate between the address the processor sees and the address the USB controller thinks it's responding to)

Is the problem actually that the USB controller needs to be able to allocate coherent memory in a range much more narrowly defined than the current dma_mask allows?

I didn't want to build another memory pool re-implementation. The mempool API seems to me to be flexible enough for this, is there some reason it won't work?

I did consider wrappering mempool to make it easier, but I couldn't really find a simplifying wrapper that wouldn't lose flexibility.

James

Yes. As noted, it can't work for USB directly. And your suggestion of more "... else if (bus is USB) ... else ... " logic (#including each bus type's headers?) bothers me.

Isn't the goal to make sure that for every kind of "struct device *" it should be possible to use those dma_*() calls, without BUGging out. If that's not true ... then why were they defined?

That's certainly the notion I was talking about when this "generic dma" API notion came up this summer [1]. (That discussion led to the USB DMA APIs, and then the usb_sg_* calls that let usb-storage queue scatterlists directly to disk: performance wins, including DaveM's "USB keyboards don't allocate IOMMU pages", but structures looking ahead to having real generic DMA calls.)

Most of us haven't seen your PARISC code, it's not in Linus' tree. :)

Actually, make that "hardware-specific code".

The USB controller is what does the DMA. But USB device drivers don't talk to USB controllers, at least not directly. Instead they talk to a "struct usb_interface *", or a "struct usb_device *" ... those are more or less software proxies for the real devices with usbcore and some HCD turning proxy i/o requests into USB controller operations.

The indirection is getting from the USB device (or interface) to the object representing the USB controller. All USB calls need that, at least for host-side APIs, since the controller driver is multiplexing up to almost 4000 I/O channels. (127 devices * 31 endpoints, max; and of course typical usage is more like dozens of channels.)

Nope, it's just an indirection issue. Even on a PCI based system, the "struct device" used by a USB driver (likely usb_interface->dev) will never be a USB controller. Since it's the USB controller actually doing the I/O something needs to use that controller to do the DMA mapping(s).

So any generic DMA logic needs to be able to drill down a level or so before doing DMA mappings (or allocations) for a USB "struct device *".

- Dave

[1] http://marc.theaimsgroup.com/?l=linux-kernel&m=102389137402497&w=2

This sounds like a mirror of the problem of finding the IOMMU on parisc (there can be more than one).

The way parisc solves this is to look in dev->platform_data and if that's null walk up the dev->parent until the IOMMU is found and then cache the IOMMU ops in the current dev->platform_data. Obviously, you can't use platform_data, but you could use driver_data for this. The IOMMU's actually lie on a parisc specific bus, so the ability to walk up the device tree without having to know the device types was crucial to implementing this.

James

Wouldn't it be straightforward to package that IOMMU solution using the "call dev->whatsit->dma_op()" approach I mentioned? Storing data in the "whatsit" seems more practical than saying driver_data is no longer available to the device's driver. (I'll be agnostic on platform_data.)

This problem seems to me to be a common layering requirement. All the indirections are known when the device structure is being initted, so it might as well be set up then. True for PARISC (right?), as well as USB, SCSI, and most other driver stacks. I suspect it'd even allow complex voodoo for multi-path I/O too...

- Dave

I didn't notice any way it would track, and return, DMA addresses. It's much like a kmem_cache in that way.

In My Ideal World (tm) Linux would have some kind of memory allocator that'd be configured to use __get_free_pages() or dma_alloc_coherent() as appropriate. Fast, efficient; caching pre-initted objects; etc.

I'm not sure how realistic that is. So long as APIs keep getting written so that drivers _must_ re-invent the "memory allocator" wheel, it's not.

But ... if the generic DMA API includes such stuff, it'd be easy to replace a dumb implementation (have you seen pci_pool, or how usb_buffer_alloc works? :) with something more intelligent than any driver could justify writing for its own use.

- Dave

OK, the attached is a sketch of an implementation of bus_type operations.

It renames all the platform dma_ operations to platform_dma_ and will call only the bus specific operation if it exists. Thus it will be the responsibility of the bus to call the platform_dma_ functions correctly (this one is a large loaded gun).

The answer to error handling in the general case is still no (because I don't want to impact the main line code for a specific problem, and the main line is x86 which effectively has infinite mapping resources), but I don't see why the platforms can't export a set of values they guarantee not to return as dma_addr_t's that you can use for errors in the bus implementations.

Would this solve most of your problems?

James

===== include/linux/device.h 1.70 vs edited =====

@@ -61,6 +61,7 @@
 struct device;
 struct device_driver;
 struct device_class;
+struct bus_dma_ops;
 
 struct bus_type {
 	char			* name;

@@ -75,6 +76,7 @@
 	struct device * (*add)	(struct device * parent, char * bus_id);
 	int		(*hotplug) (struct device *dev, char **envp, 
 				    int num_envp, char *buffer, int buffer_size);
+	struct bus_dma_ops *	dma_ops;
 };
 
 

===== include/linux/dma-mapping.h 1.1 vs edited =====

Quick reaction ....

Those signatures look more or less right, at a quick glance, except that allocating N bytes should pass a __GFP_WAIT flag. (And of course, allocating a mapping needs a failure return.)

That bus_dma_ops is more of a "vtable" approach, and I confess I'd been thinking of hanging some object that had internal state as well as method pointers. (Call it a "whatsit" for the moment.)

That'd make it possible for layered busses like USB and SCSI to just reference the "whatsit" from the parent bus in their layered "struct device" objects. [1]

In many cases that'd just end up being a ref to the "platform whatsit", eliminating a conditional test from the hot path from your sketch as well as an entire set of new "platform_*()" APIs.

- Dave

[1] That is, resembling what Benjamin Herrenschmidt suggested:

http://marc.theaimsgroup.com/?l=linux-kernel&m=102389432006266&w=2

Here's a patch (against 2.5.53) that does it with current allocators:

- adds new dma_pool calls, implemented by inlines using "kmem_cache_t *" (wrapped) or "struct pci_pool *" depending on the arch/config.

struct dma_pool *dma_pool_create(char *, struct device *, size_t) void dma_pool_destroy (struct dma_pool *pool)
void *dma_pool_alloc(struct dma_pool *, int mem_flags, dma_addr_t *) void dma_pool_free(struct dma_pool *, void *, dma_addr_t)

- new kmalloc/kfree style calls, likewise turned by inlines into kmalloc or code that can use dma_pool (but can't yet, needs a device dma state hook), falling back to dma_alloc_coherent for page allocation otherwise.

void *dma_alloc(struct device *, size_t, dma_addr_t *, int mem_flags) void dma_free(struct device *, size_t, void *, dma_addr_t)

- pci_pool is now arch-specific like the rest of the pci dma calls. along with a DMA-API.txt update, this makes 2/3 of this patch. so x86 uses dma_pool instead, through <asm-generic/pci-dma-compat.h>

An immediate effect of this patch is that on x86, both USB and Firewire will do their pci_pool allocations through kmem_cache_t, which is a smarter (and faster) allocator. (So would other drivers using pci_pool calls.)

Comments?

- Dave

@@ -152,5 +152,33 @@
 	BUG();
 }
 
+#ifdef __dma_slab_flags
+/* if we can use the slab allocator, do so.  */
+#include <asm-generic/dma-slab.h>
+
+#else
+
+extern void *
+dma_alloc (struct device *dev, size_t size, dma_addr_t *handle, int mem_flags);
+
+extern void
+dma_free (struct device *dev, size_t size, void *vaddr, dma_addr_t dma);
+
+
+#define dma_pool pci_pool
+
+static inline struct dma_pool *
+dma_pool_create (const char *name, struct device *dev, size_t size)
+{
+	BUG_ON(dev->bus != &pci_bus_type);
+	return pci_pool_create(name, to_pci_dev(dev), size, 0, 0);
+}
+
+#define dma_pool_destroy	pci_pool_destroy
+#define dma_pool_alloc		pci_pool_alloc
+#define dma_pool_free		pci_pool_free
+
+#endif
+
 #endif
 

@@ -134,4 +134,9 @@
 	flush_write_buffers();
 }
 
+/* use the slab allocator to manage dma buffers */
+#define __dma_slab_flags(dev) \
+	((*(dev)->dma_mask <= 0x00ffffff) ? SLAB_DMA : 0)
+#include <asm-generic/dma-slab.h>
+
 #endif

@@ -1 +1,7 @@
+#ifndef CONFIG_NOT_COHERENT_CACHE
+/* use the slab allocator to manage dma buffers */
+#define __dma_slab_flags(dev) \
+	((*(dev)->dma_mask != 0xffffffff) ? SLAB_DMA : 0)
+#endif
+
 #include <asm-generic/dma-mapping.h>

@@ -1 +1,4 @@
+/* use the slab allocator to manage dma buffers */
+#define __dma_slab_flags(dev) 0
+
 #include <asm-generic/dma-mapping.h>

@@ -1 +1,6 @@
+#ifdef	CONFIG_SA1111
+/* we can just use the slab allocator to manage dma buffers */
+#define __dma_slab_flags(dev) SLAB_DMA
+#endif
+
 #include <asm-generic/dma-mapping.h>

@@ -2,7 +2,7 @@
 
 obj-y		:= core.o sys.o interface.o power.o bus.o \
 			driver.o class.o intf.o platform.o \
-			cpu.o firmware.o
+			cpu.o firmware.o dma.o
 
 obj-$(CONFIG_NUMA)	+= node.o  memblk.o
 

@@ -11,4 +11,4 @@
 obj-$(CONFIG_HOTPLUG)	+= hotplug.o
 
 export-objs	:= core.o power.o sys.o bus.o driver.o \
-			class.o intf.o platform.o firmware.o
+			class.o intf.o platform.o firmware.o dma.o

@@ -672,14 +672,6 @@
 struct pci_bus * pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, int busnr);
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass);
 
-/* kmem_cache style wrapper around pci_alloc_consistent() */
-struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
-		size_t size, size_t align, size_t allocation);
-void pci_pool_destroy (struct pci_pool *pool);
-
-void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
-void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
-
 #if defined(CONFIG_ISA) || defined(CONFIG_EISA)
 extern struct pci_dev *isa_bridge;
 #endif

@@ -35,6 +35,39 @@
 	return dma_map_single(&hwdev->dev, ptr, size, (enum dma_data_direction)direction);
 }
 
+static inline struct pci_pool *
+pci_pool_create (const char *name, struct pci_dev *hwdev,
+		size_t size, size_t align, size_t allocation)
+{
+	if (size < align)
+		size = align;
+	if (allocation) {
+		while (align < allocation && (allocation % size) != 0) {
+			align <<= 1;
+			if (size < align)
+				size = align;
+		}
+	}
+	return (struct pci_pool *) dma_pool_create(name, &hwdev->dev, size);
+}
+
+static inline void pci_pool_destroy (struct pci_pool *pool)
+{
+	dma_pool_destroy ((struct dma_pool *) pool);
+}
+
+static inline void *
+pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle)
+{
+	return dma_pool_alloc((struct dma_pool *) pool, flags, handle);
+}
+
+static inline void
+pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr)
+{
+	return dma_pool_free((struct dma_pool *) pool, vaddr, addr);
+}
+
 static inline void
 pci_unmap_single(struct pci_dev *hwdev, dma_addr_t dma_addr,
 		 size_t size, int direction)

@@ -2,6 +2,8 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 
+#ifndef __dma_slab_flags
+
 /*
  * Pool allocator ... wraps the pci_alloc_consistent page allocator, so
  * small blocks are easily used by drivers for bus mastering controllers.

@@ -403,3 +405,5 @@
 EXPORT_SYMBOL (pci_pool_destroy);
 EXPORT_SYMBOL (pci_pool_alloc);
 EXPORT_SYMBOL (pci_pool_free);
+
+#endif /* !__dma_slab_flags */

@@ -81,6 +81,14 @@
 
 extern void pci_free_consistent(struct pci_dev *, size_t, void *, dma_addr_t);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicate size for PCI DMA in streaming
    mode.  The 32-bit PCI bus mastering address to use is returned.
    Once the device is given the dma address, the device owns this memory

@@ -76,6 +76,14 @@
 	consistent_free(vaddr, size, dma_handle);
 }
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *

@@ -58,6 +58,14 @@
 #define sg_dma_address			platform_pci_dma_address
 #define pci_dma_supported		platform_pci_dma_supported
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* pci_unmap_{single,page} is not a nop, thus... */
 #define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)	\
 	dma_addr_t ADDR_NAME;

@@ -77,6 +77,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /*
  * Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.

@@ -70,6 +70,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 
 #ifdef CONFIG_MAPPED_PCI_IO
 

@@ -146,6 +146,14 @@
 	void (*dma_sync_sg)(struct pci_dev *dev, struct scatterlist *sg, int nelems, int direction);
 };
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 
 /*
 ** We could live without the hppa_dma_ops indirection if we didn't want

@@ -91,6 +91,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *

@@ -43,6 +43,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 extern dma_addr_t pci_map_single(struct pci_dev *hwdev, void *ptr,
 				 size_t size, int direction);
 extern void pci_unmap_single(struct pci_dev *hwdev, dma_addr_t dma_addr,

@@ -71,6 +71,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *

@@ -47,6 +47,14 @@
  */
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size, void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *

@@ -55,6 +55,14 @@
  */
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size, void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 /* Map a single buffer of the indicated size for DMA in streaming mode.
  * The 32-bit bus address to use is returned.
  *

@@ -74,4 +74,12 @@
 pci_free_consistent (struct pci_dev *pdev, size_t size, void *cpu_addr,
 		     dma_addr_t dma_addr);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 #endif /* __V850_PCI_H__ */

@@ -69,6 +69,14 @@
 extern void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 				void *vaddr, dma_addr_t dma_handle);
 
+/* kmem_cache style wrapper around pci_alloc_consistent() */
+struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation);
+void pci_pool_destroy (struct pci_pool *pool);
+
+void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
+void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+
 #ifdef CONFIG_GART_IOMMU
 
 /* Map a single buffer of the indicated size for DMA in streaming mode.

I think a much easier way of doing this would be a slight modification to the current pci_pool code: we know it works already. All that really has to change is that it should take a struct device instead of a pci_dev and it should call dma_alloc_coherent to get the source memory. The rest of the pci pool code is mainly about memory management and is well tested and so should remain (much as I'd like to see a mempool implementation).

You have this in your code:

+void *
+dma_alloc (struct device *dev, size_t size, dma_addr_t *handle, int mem_flags) +{
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ BUG_ON(dev->bus != &pci_bus_type);
+
+ if (size >= PAGE_SIZE)
+ return pci_alloc_consistent (pdev, size, handle);

This would work if just passed to dma_alloc_coherent and drop the check for the pci_bus_type (If the dma_ APIs are implemented in terms of the pci_ ones, then they will bug out in the generic implementation of dma_alloc_coherent so there's no need to do this type of transform twice).

This:

+dma_alloc (struct device *dev, size_t size, dma_addr_t *handle, int mem_flags) +{
+ void *ret;
+
+ /* We rely on kmalloc memory being aligned to L1_CACHE_BYTES, to
+ * prevent cacheline sharing during DMA. With dma-incoherent caches,
+ * such sharing causes bugs, not just cache-related slowdown.
+ */
+ ret = kmalloc (size, mem_flags | __dma_slab_flags (dev));
+ if (likely (ret != 0))
+ *handle = virt_to_phys (ret);
+ return ret;

Just can't be done: you can't get consistent memory from kmalloc and you certainly can't use virt_to_phys and expect it to work on IOMMU hardware.

James

The same is true of the slab code, which is a better allocator. Why should kernels require an extra allocator, when pci_pool can safely be eliminated on quite a few systems?

In that patch, the pci_pool code _does_ remain ... only for use on platforms where __get_free_pages() memory, used by the slab allocator, can't be used (at least without a lot of setup work).

You finally got me to look at a the mempool code. I didn't notice any code to actually allocate 1/Nth page units; that's delegated to some other code. (Except for that "Raced" buglet, where it doesn't delegate, and uses kfree instead of calling pool->free().) So that's another reason it'd not be appropriate to try to use it in this area: it doesn't solve the core problem, which is non-wasteful allocation of 1/Nth page units.

True ... though that wouldn't be true on the other branch, where it'd be choosing some pci_pool based on size (when it gets a way to find a per-device array of pools). Do you know for a fact that GCC isn't smart enough to get rid of that duplicate check? (I'm curious.)

Actually it _can_ be done on x86 and several other common platforms. (But "coherent" memory was the agreed terminology change, yes?) Which is where that particular code kicks in ... x86, some ppc, sa1111, and uml for now. No IOMMUs there.

Any system where pci_alloc_consistent() is calling __get_free_pages(), and doing no further setup beyond a virt_to_phys() call, is a candidate for having much simpler allocation of I/O memory. I could even imagine that being true on some systems where an "IOMMU" just accelerates I/O (prefetch, cache bypass, etc) for streaming mappings, and isn't strictly needed otherwise.

- Dave

I agree in principle. But the way to get this to work is to allow the slab allocater to specify its getpages and freepages in the same way as ctor and dtor, so it can be fed by dma_alloc_coherent. Then you can wrapper the slab allocator to be used across all platforms (rather than only those which are coherent and have no IOMMU).

The benefit mempool has over slab is its guaranteed minimum pool size and hence guaranteed non-failing allocations as long as you manage pool resizing correctly. I suppose this is primarily useful for storage devices, which sometimes need memory that cannot be obtained from doing I/O.

However, you can base mempool off slab modified to use dma_alloc_coherent() and get the benefits of everything.

James

Nobody's done that yet, and it's been a couple years now since that point was made as a desirable evolution path for pci_pool. In fact the first (only!) step on that path was the dma_pool update I just posted.

Modifying the slab allocator like that means passing extra parameters around (for dma_addr_t), causing extra register pressure even for non-dma allocations. I have a hard time seeing that not slow things down, even on x86 (etc) where we know we can already get all the benefits of the slab allocator without any changes to that critical code.

Yes: not all drivers need (or want) such a memory-reservation layer.

I'd say instead that drivers which happen to need the protection against failures that mempool provides can safely layer that behavior on top of any allocator they want. (Modulo that buglet, which requires them to be layered on top of kmalloc.)

Which means that the mempool discussion is just a detour, it's not an implementation tool for any kind of dma_pool but is instead an option for drivers (in block i/o paths) to use on top of a dma_pool allocator.

- Dave

I think the attached should do the necessary with no slow down in the slab allocator.

Now all that has to happen for use with the dma pools is to wrapper dma_alloc/free_coherent().

Note that the semantics won't be quite the same as the pci_pool one since you can't guarantee that allocations don't cross the particular boundary parameter. There's also going to be a reliance on the concept that the dma coherent allocators will return a page bounded chunk of memory. Most seem already to be doing this because the page properties are only controllable at that level, so it shouldn't be a problem.

James

===== mm/slab.c 1.52 vs edited =====

@@ -260,6 +260,12 @@
 	/* de-constructor func */
 	void (*dtor)(void *, kmem_cache_t *, unsigned long);
 
+	/* page based allocator */
+	unsigned long FASTCALL((*getpages)(unsigned int gfp_mask, unsigned int order));
+
+	/* release pages allocated above */
+	void FASTCALL((*freepages)(unsigned long addr, unsigned int order));
+
 /* 4) cache creation/removal */
 	const char		*name;
 	struct list_head	next;

@@ -715,7 +721,7 @@
 	 * would be relatively rare and ignorable.
 	 */
 	flags |= cachep->gfpflags;
-	addr = (void*) __get_free_pages(flags, cachep->gfporder);
+	addr = (void*) cachep->getpages(flags, cachep->gfporder);
 	/* Assume that now we have the pages no one else can legally
 	 * messes with the 'struct page's.
 	 * However vm_scan() might try to test the structure to see if

@@ -741,7 +747,7 @@
 		dec_page_state(nr_slab);
 		page++;
 	}
-	free_pages((unsigned long)addr, cachep->gfporder);
+	cachep->freepages((unsigned long)addr, cachep->gfporder);
 }
 
 #if DEBUG

@@ -811,13 +817,15 @@
 }
 
 /**
- * kmem_cache_create - Create a cache.
+ * kmem_cache_create_alloc - Create a cache with a custom allocator.
  * @name: A string which is used in /proc/slabinfo to identify this cache.
  * @size: The size of objects to be created in this cache.
  * @offset: The offset to use within the page.
  * @flags: SLAB flags
  * @ctor: A constructor for the objects.
  * @dtor: A destructor for the objects.
+ * @getpages: A function to get the memory pages
+ * @freepages: A function to free the corresponding pages
  *
  * Returns a ptr to the cache on success, NULL on failure.
  * Cannot be called within a int, but can be interrupted.

@@ -844,9 +852,12 @@
  * as davem.
  */
 kmem_cache_t *
-kmem_cache_create (const char *name, size_t size, size_t offset,
-	unsigned long flags, void (*ctor)(void*, kmem_cache_t *, unsigned long),
-	void (*dtor)(void*, kmem_cache_t *, unsigned long))
+kmem_cache_create_with_alloc (const char *name, size_t size, size_t offset,
+	unsigned long flags,
+	void (*ctor)(void*, kmem_cache_t *, unsigned long),
+	void (*dtor)(void*, kmem_cache_t *, unsigned long),
+	unsigned long FASTCALL((*getpages)(unsigned int gfp_mask, unsigned int order)),
+	void FASTCALL((*freepages)(unsigned long addr, unsigned int order)))
 {
 	const char *func_nm = KERN_ERR "kmem_create: ";
 	size_t left_over, align, slab_size;

@@ -1010,6 +1021,9 @@
 		cachep->slabp_cache = kmem_find_general_cachep(slab_size,0);
 	cachep->ctor = ctor;
 	cachep->dtor = dtor;
+	BUG_ON(getpages == NULL || freepages == NULL);
+	cachep->getpages = getpages;
+	cachep->freepages = freepages;
 	cachep->name = name;
 
 	if (g_cpucache_up == FULL) {

@@ -1072,6 +1086,50 @@
 	up(&cache_chain_sem);
 opps:
 	return cachep;
+}
+
+/**
+ * kmem_cache_create_alloc - Create a cache with a custom allocator.
+ * @name: A string which is used in /proc/slabinfo to identify this cache.
+ * @size: The size of objects to be created in this cache.
+ * @offset: The offset to use within the page.
+ * @flags: SLAB flags
+ * @ctor: A constructor for the objects.
+ * @dtor: A destructor for the objects.
+ * @getpages: A function to get the memory pages
+ * @freepages: A function to free the corresponding pages
+ *
+ * Returns a ptr to the cache on success, NULL on failure.
+ * Cannot be called within a int, but can be interrupted.
+ * The @ctor is run when new pages are allocated by the cache
+ * and the @dtor is run before the pages are handed back.
+ *
+ * @name must be valid until the cache is destroyed. This implies that
+ * the module calling this has to destroy the cache before getting 
+ * unloaded.
+ * 
+ * The flags are
+ *
+ * %SLAB_POISON - Poison the slab with a known test pattern (a5a5a5a5)
+ * to catch references to uninitialised memory.
+ *
+ * %SLAB_RED_ZONE - Insert `Red' zones around the allocated memory to check
+ * for buffer overruns.
+ *
+ * %SLAB_NO_REAP - Don't automatically reap this cache when we're under
+ * memory pressure.
+ *
+ * %SLAB_HWCACHE_ALIGN - Align the objects in this cache to a hardware
+ * cacheline.  This can be beneficial if you're counting cycles as closely
+ * as davem.
+ */
+kmem_cache_t *
+kmem_cache_create (const char *name, size_t size, size_t offset,
+	unsigned long flags,
+	void (*ctor)(void*, kmem_cache_t *, unsigned long),
+	void (*dtor)(void*, kmem_cache_t *, unsigned long))
+{
+	return kmem_cache_create_with_alloc(name, size, offset, flags, ctor, dtor, __get_free_pages, free_pages);
 }
 
 static inline void check_irq_off(void)

You didn't make anything store or return the dma_addr_t ... that was the issue I was referring to, it's got to be either (a) passed up from the very lowest level, like the pci_*() calls assume, or else (b) cheaply derived from the virtual address. My patch added slab support in common cases where (b) applies.

(By the way -- I'm glad we seem to be agreeing on the notion that we should have a dma_pool! Is that also true of kmalloc/kfree analogues?)

Go back and look at the dma_pool patch I posted, which shows how to handle that using extra padding. Only one driver needed that, and it looks like maybe its hardware spec changed so that's not an issue.

That's why the signature of dma_pool_create() is simpler than the one for pci_pool_create() ... I never liked it before, and now I'm fully convinced it _can_ be simpler.

I think that's a desirable way to specify dma_alloc_coherent(), as handling page-and-above. The same layering as "normal" memory allocation. (In fact, that's how I had updated the docs.)

- Dave

That's fairly simply done as part of the wrappers: The allocator stores the vaddr, paddr and size in a hash table. Thus, the paddr can be deduced when kmem_cache_alloc is called by the allocation wrapper using the linearity property.

I've got to say though that the most sensible course of action is still to generalise pci_pool, which can be done easily and safely. I think replacing it with a slab based scheme is probably a 2.7 thing.

James

However it'd be done, it'd be an essential part, and it was missing. In fact, your getpages() didn't have a way to return the dma_addr_t values, and your freepages() didn't provide it as an input. (But it did pass mem_flags in, as I had at some point suggested should be done with dma_alloc_coherent.)

I think the patch I posted "easily and safely" does that, appropriate for 2.5, even though some platforms can't yet collect that win.

I'd agree that morphing drivers/pci/pool.c into drivers/base/pool.c (or whatever) would be appropriate, but I haven't heard any arguments to justify using that allocator on systems where the the slab code can be used (more cheaply).

- Dave

How about the attached as the basis for a generic coherent memory pool implementation. It basically leverages pci/pool.c to be more generic, and thus makes use of well tested code.

Obviously, as a final tidy up, pci/pool.c should probably be moved to base/pool.c with compile options for drivers that want it.

James

diff -Nru a/drivers/base/core.c b/drivers/base/core.c
@@ -146,6 +146,7 @@
 	INIT_LIST_HEAD(&dev->bus_list);
 	INIT_LIST_HEAD(&dev->class_list);
 	INIT_LIST_HEAD(&dev->intf_list);
+	INIT_LIST_HEAD(&dev->pools);
 }
 
 /**

diff -Nru a/drivers/pci/probe.c b/drivers/pci/probe.c
@@ -353,8 +353,6 @@
 
 	sprintf(dev->slot_name, "%02x:%02x.%d", dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
 	sprintf(dev->dev.name, "PCI device %04x:%04x", dev->vendor, dev->device);
-	INIT_LIST_HEAD(&dev->pools);
-	
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class);
 	class >>= 8;				    /* upper 3 bytes */
 	dev->class = class;

diff -Nru a/include/linux/device.h b/include/linux/device.h
@@ -256,6 +256,8 @@
 	struct list_head driver_list;
 	struct list_head children;
 	struct list_head intf_list;
+	struct list_head pools;		/* dma_pools tied to this device */
+
 	struct device 	* parent;
 
 	struct kobject kobj;

diff -Nru a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_LINUX_DMA_MAPPING_H
 #define _ASM_LINUX_DMA_MAPPING_H
 
+#include <linux/dma-pool.h>
+
 /* These definitions mirror those in pci.h, so they can be used
  * interchangeably with their PCI_ counterparts */
 enum dma_data_direction {

diff -Nru a/include/linux/dma-pool.h b/include/linux/dma-pool.h
@@ -0,0 +1,15 @@
+#ifndef _LINUX_DMA_POOL_H
+#define _LINUX_DMA_POOL_H
+
+#include <linux/device.h>
+
+/* dma_pool is an opaque structure pointer */
+struct dma_pool;
+
+struct dma_pool *dma_pool_create(const char *name, struct device *dev,
+				 size_t size, size_t align, size_t allocation);
+void dma_pool_destroy(struct dma_pool *pool);
+void *dma_pool_alloc(struct dma_pool *pool, int flags, dma_addr_t *handle);
+void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t addr);
+
+#endif

diff -Nru a/include/linux/pci.h b/include/linux/pci.h
@@ -388,8 +388,6 @@
 					   0xffffffff.  You only need to change
 					   this if your device has broken DMA
 					   or supports 64-bit transfers.  */
-	struct list_head pools;		/* pci_pools tied to this device */
-
 	u32             current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */

@@ -658,14 +656,38 @@
 unsigned int pci_do_scan_bus(struct pci_bus *bus);
 struct pci_bus * pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, int busnr);
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass);
+#include <linux/dma-pool.h>
 
 /* kmem_cache style wrapper around pci_alloc_consistent() */
-struct pci_pool *pci_pool_create (const char *name, struct pci_dev *dev,
-		size_t size, size_t align, size_t allocation);
-void pci_pool_destroy (struct pci_pool *pool);
 
-void *pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
-void pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr);
+/* struct pci_pool is an alias for struct dma_pool.  However, its contents
+ * are never exposed so just declare it here */
+struct pci_pool;
+
+static inline struct pci_pool *
+pci_pool_create (const char *name, struct pci_dev *dev,
+		size_t size, size_t align, size_t allocation)
+{
+	return (struct pci_pool *)dma_pool_create(name, &dev->dev, size, align, allocation);
+}
+
+static inline void
+pci_pool_destroy(struct pci_pool *pool)
+{
+	dma_pool_destroy((struct dma_pool *)pool);
+}
+
+static inline void *
+pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle)
+{
+	return dma_pool_alloc((struct dma_pool *)pool, flags, handle);
+}
+
+static inline void
+pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t addr)
+{
+	dma_pool_free((struct dma_pool *)pool, vaddr, addr);
+}
 
 #if defined(CONFIG_ISA) || defined(CONFIG_EISA)
 extern struct pci_dev *isa_bridge;

I'd still rather have configuration completely eliminate that particular pool allocator on the platforms (most, by volume) that don't need it, in favor of the slab code ... which is not only well-tested, but also has had a fair amount of cross-platform performance work done on it.

I'd have no problems with making it even more generic, and moving it.

Though "compile options" doesn't sound right, unless you mean letting arch-specific code choose whether to use that or the slab allocator.

- Dave

That's actually an open question. The line of least resistance (which is what I followed) is to do what the pci_ API does (i.e. BUG()).

It's not clear to me that adding error returns rather than BUGging would buy us anything (because now all the drivers have to know about the errors and process them).

That's pretty much an edge case. I'm not opposed to putting edge cases in the api (I did it for dma_alloc_noncoherent() to help parisc), but I don't think the main line should be affected unless there's a good case for it.

Perhaps there is a compromise where the driver flags in the struct device_driver that it wants error returns otherwise it takes the default behaviour (i.e. no error return checking and BUG if there's a problem).

James

Hi,

That might have been appropriate for PCI-mostly APIs, since those tend to be resource-rich. Maybe. (It always seemed like an API bug to me.)

I can't buy that logic in the "generic" case though. Heck, haven't all the address space allocation calls in Linux always exposed ENOMEM type faults ... except PCI? This one is _really_ easy to fix now. Resources are never infinite.

For me, designing any "generic" API to handle common cases (like allocation failures) reasonably (no BUGging!) is a fundamental design requirement.

Robust drivers are aware of things like allocation faults, and handle them. If they do so poorly, that can be fixed like any other driver bug.

Absolutely *any* system can have situations where the relevant address space (or memory) was all in use, or wasn't available to a non-blocking request without blocking, etc. Happens more often on some systems than others; I just chose SA-1111 since your approach would seem to make that unusable.

If that isn't a "good case", why not? And what could ever be a "good case"?

IMO that's the worst of all possible worlds. The error paths would get even less testing than they do today. If there's a fault path defined, use it in all cases: don't just BUG() in some modes, and some drivers.

- Dave