Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Dec 2022 08:58:15 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Keith Busch <kbusch@meta.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>,
        Tony Battersby <tonyb@cybernetics.com>,
        Kernel Team <kernel-team@meta.com>,
        Keith Busch <kbusch@kernel.org>
Subject: Re: [PATCHv2 11/11] dmapool: link blocks across pages
Message-ID: <Y6XeJ2mzd8p73J93@infradead.org>
References: <20221216201625.2362737-1-kbusch@meta.com>
 <20221216201625.2362737-12-kbusch@meta.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221216201625.2362737-12-kbusch@meta.com>
Precedence: bulk

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.