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

[Christoph Hellwig]

On Fri, Dec 16, 2022 at 12:16:25PM -0800, Keith Busch wrote:
>  	unsigned int size;
>  	unsigned int allocation;
>  	unsigned int boundary;
> +	size_t nr_blocks;
> +	size_t nr_active;
> +	size_t nr_pages;

Should these be unsigned int like the counters above?

> +static inline struct dma_block *pool_block_pop(struct dma_pool *pool)
> +{
> +	struct dma_block *block = pool->next_block;
> +
> +	if (block) {
> +		pool->next_block = block->next_block;
> +		pool->nr_active++;
> +	}
> +	return block;
> +}
> +
> +static inline void pool_block_push(struct dma_pool *pool, struct dma_block *block,
> +				 dma_addr_t dma)
> +{
> +	block->dma = dma;
> +	block->next_block = pool->next_block;
> +	pool->next_block = block;
> +}

Any point in marking these inline vs just letting the ocmpile do
it's job?

> @@ -162,6 +176,10 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	retval->size = size;
>  	retval->boundary = boundary;
>  	retval->allocation = allocation;
> +	retval->nr_blocks = 0;
> +	retval->nr_active = 0;
> +	retval->nr_pages = 0;
> +	retval->next_block = NULL;

Maybe just switch to kzmalloc so that you don't have to bother
initializing invdividual fields.  It's not like dma_pool_create is
called from anything near a fast path.

>  static void pool_initialise_page(struct dma_pool *pool, struct dma_page *page)
>  {
> +	unsigned int next_boundary = pool->boundary, offset = 0;
> +	struct dma_block *block;
> +
> +	while (offset + pool->size <= pool->allocation) {
> +		if (offset + pool->size > next_boundary) {
> +			offset = next_boundary;
>  			next_boundary += pool->boundary;
> +			continue;
>  		}
> +
> +		block = page->vaddr + offset;
> +		pool_block_push(pool, block, page->dma + offset);

So I guess with this pool_initialise_page needs to be called under
the lock anyway, but just doing it silently in the previous patch
seems a bit odd.

> +static inline void pool_check_block(struct dma_pool *pool, struct dma_block *block,
> +				    gfp_t mem_flags)

I didn't spot this earlier, but inline on a relatively expensive debug
helper is a bit silly.

Otherwise this looks like a nice improvement by using a better and
simpler data structure.