Re: [PATCH 3/9] mempool: add mempool_{alloc,free}_bulk

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Oct 31, 2025 at 10:34:33AM +0100, Christoph Hellwig wrote:
>  /**
> - * mempool_alloc - allocate an element from a memory pool
> + * mempool_alloc_bulk - allocate multiple elements from a memory pool
>   * @pool:	pointer to the memory pool
> + * @elem:	partially or fully populated elements array
> + * @count:	size (in entries) of @elem
>   * @gfp_mask:	GFP_* flags.
>   *
> + * Allocate elements for each slot in @elem that is non-%NULL.

elem => elems (and likewise for mempool_free_bulk())

>   * Note: This function only sleeps if the alloc_fn callback sleeps or returns
>   * %NULL.  Using __GFP_ZERO is not supported.
>   *
> - * Return: pointer to the allocated element or %NULL on error. This function
> - * never returns %NULL when @gfp_mask allows sleeping.
> + * Return: 0 if successful, else -ENOMEM.  This function never returns -ENOMEM
> + * when @gfp_mask allows sleeping.
>   */
> -void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask)
> +int mempool_alloc_bulk_noprof(struct mempool *pool, void **elem,
> +		unsigned int count, gfp_t gfp_mask, unsigned long caller_ip)

What exactly is the behavior on partial failures?  Is the return value 0
or is it -ENOMEM, and is the array restored to its original state or
might some elements have been allocated?

> +/**
> + * mempool_alloc - allocate an element from a memory pool
> + * @pool:	pointer to the memory pool
> + * @gfp_mask:	GFP_* flags.
> + *
> + * Note: This function only sleeps if the alloc_fn callback sleeps or returns
> + * %NULL.  Using __GFP_ZERO is not supported.
> + *
> + * Return: pointer to the allocated element or %NULL on error. This function
> + * never returns %NULL when @gfp_mask allows sleeping.
> + */
> +void *mempool_alloc_noprof(struct mempool *pool, gfp_t gfp_mask)
> +{
> +	void *elem[1] = { };
> +
> +	if (mempool_alloc_bulk_noprof(pool, elem, 1, gfp_mask, _RET_IP_) < 0)
> +		return NULL;
> +	return elem[0];
> +}
>  EXPORT_SYMBOL(mempool_alloc_noprof);

How much overhead does this add to mempool_alloc(), which will continue
to be the common case?  I wonder if it would be worthwhile to
force-inline the bulk allocation function into it, so that it will get
generate about the same code as before.

>  	if (unlikely(READ_ONCE(pool->curr_nr) < pool->min_nr)) {
>  		spin_lock_irqsave(&pool->lock, flags);
> -		if (likely(pool->curr_nr < pool->min_nr)) {
> -			add_element(pool, element);
> -			spin_unlock_irqrestore(&pool->lock, flags);
> -			if (wq_has_sleeper(&pool->wait))
> -				wake_up(&pool->wait);
> -			return;
> +		while (pool->curr_nr < pool->min_nr && freed < count) {
> +			add_element(pool, elem[freed++]);
> +			added = true;
>  		}
>  		spin_unlock_irqrestore(&pool->lock, flags);
> -	}
>  
>  	/*
>  	 * Handle the min_nr = 0 edge case:
> @@ -572,20 +614,41 @@ void mempool_free(void *element, mempool_t *pool)
>  	 * allocation of element when both min_nr and curr_nr are 0, and
>  	 * any active waiters are properly awakened.
>  	 */

The above comment has a weird position now.  Maybe move it into the
'else if' block below.

> -	if (unlikely(pool->min_nr == 0 &&
> +	} else if (unlikely(pool->min_nr == 0 &&
>  		     READ_ONCE(pool->curr_nr) == 0)) {
>  		spin_lock_irqsave(&pool->lock, flags);
>  		if (likely(pool->curr_nr == 0)) {
> -			add_element(pool, element);
> -			spin_unlock_irqrestore(&pool->lock, flags);
> -			if (wq_has_sleeper(&pool->wait))
> -				wake_up(&pool->wait);
> -			return;
> +			add_element(pool, elem[freed++]);
> +			added = true;
>  		}
>  		spin_unlock_irqrestore(&pool->lock, flags);
>  	}
>  
> -	pool->free(element, pool->pool_data);
> +	if (unlikely(added) && wq_has_sleeper(&pool->wait))
> +		wake_up(&pool->wait);
> +
> +	return freed;
> +}
> +EXPORT_SYMBOL_GPL(mempool_free_bulk);

- Eric