Re: [PATCH RFC v2 04/14] mm: create common code from request allocation based from blk-mq code

[Andrew Morton]

On Thu, 12 Dec 2019 11:24:34 -0700 Dave Jiang <dave.jiang@xxxxxxxxx> wrote:

> Move the allocation of requests from compound pages to a common function
> to allow usages by other callers.

What other callers are expected?

> Since the routine has more to do with
> memory allocation and management, it is moved to be exported by the
> mempool.h and be part of mm subsystem.
> 

Hm, this move doesn't seem to fit very well.  But perhaps it's close
enough.

> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -42,7 +42,7 @@ obj-y			:= filemap.o mempool.o oom_kill.o fadvise.o \
>  			   mm_init.o mmu_context.o percpu.o slab_common.o \
>  			   compaction.o vmacache.o \
>  			   interval_tree.o list_lru.o workingset.o \
> -			   debug.o gup.o $(mmu-y)
> +			   debug.o gup.o request_alloc.o $(mmu-y)

Now there's a regression.  We're adding a bunch of unused code to a
CONFIG_BLOCK=n kernel.

>
> ...
>
> +void request_from_pages_free(struct list_head *page_list)
>
> ...
>
> +int request_from_pages_alloc(void *ctx, unsigned int depth, size_t rq_size,
> +			     struct list_head *page_list, int max_order,
> +			     int node,
> +			     void (*assign)(void *ctx, void *req, int idx))

I find these function names hard to understand.  Are they well chosen?

Some documentation would help.  These are global, exported-to-modules
API functions and they really should be fully documented.

> +{
> +	size_t left;
> +	unsigned int i, j, entries_per_page;
> +
> +	left = rq_size * depth;
> +
> +	for (i = 0; i < depth; ) {

"depth" of what?

> +		int this_order = max_order;
> +		struct page *page;
> +		int to_do;
> +		void *p;
> +
> +		while (this_order && left < order_to_size(this_order - 1))
> +			this_order--;
> +
> +		do {
> +			page = alloc_pages_node(node,
> +						GFP_NOIO | __GFP_NOWARN |
> +						__GFP_NORETRY | __GFP_ZERO,
> +						this_order);
> +			if (page)
> +				break;
> +			if (!this_order--)
> +				break;
> +			if (order_to_size(this_order) < rq_size)
> +				break;
> +		} while (1);

What the heck is all the above trying to do?  Some explanatory comments
are needed, methinks.

> +		if (!page)
> +			goto fail;
> +
> +		page->private = this_order;
> +		list_add_tail(&page->lru, page_list);
> +
> +		p = page_address(page);
> +		/*
> +		 * Allow kmemleak to scan these pages as they contain pointers
> +		 * to additional allocations like via ops->init_request().
> +		 */
> +		kmemleak_alloc(p, order_to_size(this_order), 1, GFP_NOIO);
> +		entries_per_page = order_to_size(this_order) / rq_size;
> +		to_do = min(entries_per_page, depth - i);
> +		left -= to_do * rq_size;
> +		for (j = 0; j < to_do; j++) {
> +			assign((void *)ctx, p, i);
> +			p += rq_size;
> +			i++;
> +		}
> +	}
> +
> +	return i;
> +
> +fail:
> +	request_from_pages_free(page_list);
> +	return -ENOMEM;
> +}
> +EXPORT_SYMBOL_GPL(request_from_pages_alloc);