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);
