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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y6XeJ2mzd8p73J93@infradead.org>
Date:   Fri, 23 Dec 2022 08:58:15 -0800
From:   Christoph Hellwig <hch@...radead.org>
To:     Keith Busch <kbusch@...a.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>,
        Tony Battersby <tonyb@...ernetics.com>,
        Kernel Team <kernel-team@...a.com>,
        Keith Busch <kbusch@...nel.org>
Subject: Re: [PATCHv2 11/11] dmapool: link blocks across pages

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.