On 12/5/22 09:59, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> The allocated dmapool pages are never freed for the lifetime of the
> pool. There is no need for the two level list+stack lookup for finding a
> free block since nothing is ever removed from the list. Just use a
> simple stack, reducing time complexity to constant.
>
> The implementation inserts the stack linking elements and the dma handle
> of the block within itself when freed. This means the smallest possible
> dmapool block is increased to at most 16 bytes to accomodate these
> fields, but there are no exisiting users requesting a dma pool smaller
> than that anyway.
Great work!
I notice that the comment at the top of dmapool.c describes the old
design ("Free blocks are tracked in an unsorted singly-linked
list of free blocks within the page."), so you need to delete or update
that part of the comment.
> struct dma_pool { /* the pool */
> struct list_head page_list;
> spinlock_t lock;
> struct device *dev;
> + struct dma_block *next_block;
> unsigned int size;
> unsigned int allocation;
> unsigned int boundary;
> + unsigned int nr_blocks;
> + unsigned int nr_active;
> + unsigned int nr_pages;
I think nr_blocks, nr_active, and nr_pages should be size_t rather than
unsigned int since they count the number of objects in the entire pool,
and it would be theoretically possible to allocate more than 2^32 objects.
> @@ -199,22 +217,24 @@ EXPORT_SYMBOL(dma_pool_create);
>
> static void pool_initialise_page(struct dma_pool *pool, struct dma_page *page)
> {
> - unsigned int offset = 0;
> - unsigned int next_boundary = pool->boundary;
> -
> - page->in_use = 0;
> - page->offset = 0;
> - do {
> - unsigned int next = offset + pool->size;
> - if (unlikely((next + pool->size) >= next_boundary)) {
> - next = next_boundary;
> + unsigned int next_boundary = pool->boundary, offset = 0;
> + struct dma_block *block;
> +
> + while (offset < pool->allocation) {
> + if (offset > next_boundary) {
This is incorrect. I believe the correct comparison should be:
+ while (offset + pool->size <= pool->allocation) {
+ if (offset + pool->size > next_boundary) {
That should handle all the weird possible combinations of size,
boundary, and allocation.
Tony Battersby
Cybernetics
