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;
Message-ID: <8fc56daf-7f8d-b62b-b6bf-4da4ca87ea20@cybernetics.com>
Date:   Mon, 5 Dec 2022 13:07:15 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH 11/11] dmapool: link blocks across pages
Content-Language: en-US
To:     Keith Busch <kbusch@meta.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Matthew Wilcox <willy@infradead.org>
Cc:     Keith Busch <kbusch@kernel.org>
References: <20221205145937.54367-1-kbusch@meta.com>
 <20221205145937.54367-12-kbusch@meta.com>
From:   Tony Battersby <tonyb@cybernetics.com>
In-Reply-To: <20221205145937.54367-12-kbusch@meta.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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