Re: [PATCH 11/11] dmapool: link blocks across pages

[Tony Battersby <tonyb@cybernetics.com>]

On 12/5/22 09:59, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> 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