lkml.org 
[lkml]   [2019]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC v2 04/14] mm: create common code from request allocation based from blk-mq code
On Thu, 12 Dec 2019 11:24:34 -0700 Dave Jiang <dave.jiang@intel.com> 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);

\
 
 \ /
  Last update: 2019-12-13 01:43    [W:0.129 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site