Skip to contentSkip to conversation

Pull request: DMA pool updates

DISCUSSION Matthew Wilcox 2 participants 2 messages Feb 6, 2008 Memory management

lore
Share & tools
lore
#0 lore raw

Hi Linus,

Could I ask you to pull the DMA Pool changes detailed below?

All the patches have been posted to linux-kernel before, and various comments (and acks) have been taken into account. (see
http://thread.gmane.org/gmane.linux.kernel/609943)

It's a fairly nice performance improvement, so would be good to get in. It's survived a few hours of *mumble* high-stress database benchmark, so I have high confidence in its stability.

The following changes since commit 21511abd0a248a3f225d3b611cfabb93124605a7:
Linus Torvalds (1):

        Merge branch 'release' of git://git.kernel.org/.../aegl/linux-2.6

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/willy/misc.git dmapool

Matthew Wilcox (7):
Move dmapool.c to mm/ directory
dmapool: Fix style problems
Avoid taking waitqueue lock in dmapool
dmapool: Validate parameters to dma_pool_create
dmapool: Tidy up includes and add comments
Change dmapool free block management
pool: Improve memory usage for devices which can't cross boundaries

 drivers/base/Makefile  |    2 +-
 drivers/base/dmapool.c |  481 ----------------------------------------------
 mm/Makefile            |    1 +
 mm/dmapool.c           |  500 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 502 insertions(+), 482 deletions(-)
 delete mode 100644 drivers/base/dmapool.c
 create mode 100644 mm/dmapool.c
Signature · 10 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#1 lore raw
Quoted 10 lines · On Tue, 5 Feb 2008 19:52:47 -0700 Matthew Wilcox <[email protected]> wrote:

Could I ask you to pull the DMA Pool changes detailed below?

All the patches have been posted to linux-kernel before, and various comments (and acks) have been taken into account. (see
http://thread.gmane.org/gmane.linux.kernel/609943)

It's a fairly nice performance improvement, so would be good to get in. It's survived a few hours of *mumble* high-stress database benchmark, so I have high confidence in its stability.

Quoted 7 lines

The following changes since commit 21511abd0a248a3f225d3b611cfabb93124605a7:
Linus Torvalds (1):

        Merge branch 'release' of git://git.kernel.org/.../aegl/linux-2.6

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/willy/misc.git dmapool

Looks OK to me - I think I reviewed these a while back? We really should have had this tree in -mm for general tyre-kicking.

What's with the #ifdef CONFIG_SLAB stuff in the dmapool code? Are we just overloading a convenient Kconfig label here, or is it required for some reason? Why not CONFIG_SLUB_DEBUG too?

For the future:

This code:

+struct dma_pool *dma_pool_create(const char *name, struct device *dev,
+ size_t size, size_t align, size_t boundary)
+{
+ struct dma_pool *retval;
+ size_t allocation;
+
+ if (align == 0) {
+ align = 1;
+ } else if (align & (align - 1)) {
+ return NULL;
+ }
+
+ if (size == 0) {
+ return NULL;
+ } else if (size < 4) {
+ size = 4;
+ }
+
+ if ((size % align) != 0)
+ size = ALIGN(size, align);
+
+ allocation = max_t(size_t, size, PAGE_SIZE);
+
+ if (!boundary) {
+ boundary = allocation;
+ } else if ((boundary < size) || (boundary & (boundary - 1))) {
+ return NULL;
+ }

could do with some relief from its brace fetish and some education about is_power_of_2(). And the `if ((size % align) != 0)' can just go away, which will save code and is likely faster.

It's a separate thing I guess, but I don't see any reason why dma_pool_alloc() _has_ to use GFP_ATOMIC. Looks like it can do the allocation outside spin_lock_irqsave() and use mem_flags instead. If that has __GFP_WAIT then we're in much better shape.

I wish all this code wouldn't do

	struct dma_page *page;

because one very much expects a local variable called "page" to be of type `struct page *'.