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.
