From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 55C95C4332F for ; Tue, 18 Oct 2022 17:51:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 86DD98E0001; Tue, 18 Oct 2022 13:51:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 81DA66B0075; Tue, 18 Oct 2022 13:51:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6E5918E0001; Tue, 18 Oct 2022 13:51:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 5EBE76B0072 for ; Tue, 18 Oct 2022 13:51:48 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 35F4FA10EB for ; Tue, 18 Oct 2022 17:51:48 +0000 (UTC) X-FDA: 80034813096.01.7362A8B Received: from mail-pf1-f169.google.com (mail-pf1-f169.google.com [209.85.210.169]) by imf03.hostedemail.com (Postfix) with ESMTP id CED2B2000C for ; Tue, 18 Oct 2022 17:51:47 +0000 (UTC) Received: by mail-pf1-f169.google.com with SMTP id 3so14823584pfw.4 for ; Tue, 18 Oct 2022 10:51:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=TMLVsDeOxaJfLSB7Nf4Fo7/C7oXeEsIQmvCMFgmnqE0=; b=XrvzkWmtESC1AgLqiZGVRrwjW89MUpmcQKu6gVg44kf11WOU8V7DM2d88EDorXDboR 0m4Rm0iIXkHvqq4hiEEEwRfU6ptCbPDqYg4Mh8gbQg58JO/+LRpAh2wa0FBNjYCJ4+VJ Ob5C3WJlYjZI0MSPWRPw7MGArfz+OTeEvm2pOxfrZ1Dhjg73/OUF6Qrm4B/nA8RfUeO6 FOy2KwdcG6z7jZXQ7ZoIY0nZQ1WxknQLfRFPx73EVKBJeF9vfQ6PmxXSjR/GP/sz/WLN 2eAh8IAhABBa/gpC+CpzE1nnYcAgmnyxay4JarxAniEnzmXHRdwY9CCusm6F6/fjoiuo g5SA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=TMLVsDeOxaJfLSB7Nf4Fo7/C7oXeEsIQmvCMFgmnqE0=; b=q0ntzVKL5lDDjuMJjs4EJ42g661/oI16u9eg9ZLLEPy71zx43Krk0eZiJxIQeAzNJ7 CogcnXgZ/aSMEhDvjbFSRRmLGndFpjOD8O9aRifXLcoy3v8rQFfyO8WGjxZSyR1OACoB l+uNMo+dhprSjvwO/iiaP6lYDnwV8ihxSBlTm4wtzoGVOxmbcI6j4cSuEABrij8r3kcO To7AWYiPnrNb/fmoBWSZo3D3qwSjR829APuu0DS3iva/esCl43lUtpHsBkqm5XIpBgj7 YtmbjWNPa3qZTRrPD9Ujpvp7tGyIW2lB1GN/xLnffbfzOVMDxr7DcbS4WZGY7dv2SHuu vc2Q== X-Gm-Message-State: ACrzQf2HooABjUzMoyuGlne/sGUjdI86iawMq8XL6e4tzs4rd4NMdbOL JqZoqQf5ho7DnuIMgzcEzW1dvN6cpU7kI6De/14= X-Google-Smtp-Source: AMsMyM4flMKuRrdtA9zB35sp9o8LErRNHsL0X4chitFUdIByl4v05gzOWyNaZHWfOr8VW14oX7WFvHNqg6LQTY6JEzM= X-Received: by 2002:a05:6a00:24c2:b0:52e:7181:a8a0 with SMTP id d2-20020a056a0024c200b0052e7181a8a0mr4417808pfv.57.1666115506399; Tue, 18 Oct 2022 10:51:46 -0700 (PDT) MIME-Version: 1.0 References: <20221005180341.1738796-1-shy828301@gmail.com> <20221005180341.1738796-3-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Tue, 18 Oct 2022 10:51:33 -0700 Message-ID: Subject: Re: [PATCH 2/4] mm: mempool: introduce page bulk allocator To: Brian Foster Cc: mgorman@techsingularity.net, agk@redhat.com, snitzer@kernel.org, dm-devel@redhat.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666115507; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=TMLVsDeOxaJfLSB7Nf4Fo7/C7oXeEsIQmvCMFgmnqE0=; b=YRGabbDnqUsafeil50vGg0upKg1aQkG8dfgl2KXe7jirJM7iL/F09XPHTWOYcb0lQh4TmC pqnY25QtkMQltJCYdB4PjKNVOhKYQYNCNIKIJSUq1Oh4oHHiZVpG32o0KqRbu+twJRLkX5 a1paSTyKGi8FpC/zwYgF4pg19xEher0= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=XrvzkWmt; spf=pass (imf03.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.169 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666115507; a=rsa-sha256; cv=none; b=jYgv1+qV82DbE8N/UuHkXer9wwoFouNlSpZSv3UUHTiSyqpGQ5SIQEDMAucU5iuttwnKsb mEDsKyo3pZ8yZQue5IgCHTP+QhiBek76o3MI79GmqRl4Jdr/zqhp3qIuSz4xWmfeE2R6YM HkJf4mi6+mxFLmFNgD2TiA7oWqGdg6g= X-Rspamd-Server: rspam12 X-Rspam-User: Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=XrvzkWmt; spf=pass (imf03.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.169 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Stat-Signature: dfw3dpsagsec7rznh6f8d6595idya47f X-Rspamd-Queue-Id: CED2B2000C X-HE-Tag: 1666115507-920842 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Oct 14, 2022 at 5:03 AM Brian Foster wrote: > > On Thu, Oct 06, 2022 at 11:43:21AM -0700, Yang Shi wrote: > > On Thu, Oct 6, 2022 at 7:47 AM Brian Foster wrote: > > > > > > On Wed, Oct 05, 2022 at 11:03:39AM -0700, Yang Shi wrote: > > > > Since v5.13 the page bulk allocator was introduced to allocate order-0 > > > > pages in bulk. There are a few mempool allocator callers which does > > > > order-0 page allocation in a loop, for example, dm-crypt, f2fs compress, > > > > etc. A mempool page bulk allocator seems useful. So introduce the > > > > mempool page bulk allocator. > > > > > > > > It introduces the below APIs: > > > > - mempool_init_pages_bulk() > > > > - mempool_create_pages_bulk() > > > > They initialize the mempool for page bulk allocator. The pool is filled > > > > by alloc_page() in a loop. > > > > > > > > - mempool_alloc_pages_bulk_list() > > > > - mempool_alloc_pages_bulk_array() > > > > They do bulk allocation from mempool. > > > > They do the below conceptually: > > > > 1. Call bulk page allocator > > > > 2. If the allocation is fulfilled then return otherwise try to > > > > allocate the remaining pages from the mempool > > > > 3. If it is fulfilled then return otherwise retry from #1 with sleepable > > > > gfp > > > > 4. If it is still failed, sleep for a while to wait for the mempool is > > > > refilled, then retry from #1 > > > > The populated pages will stay on the list or array until the callers > > > > consume them or free them. > > > > Since mempool allocator is guaranteed to success in the sleepable context, > > > > so the two APIs return true for success or false for fail. It is the > > > > caller's responsibility to handle failure case (partial allocation), just > > > > like the page bulk allocator. > > > > > > > > The mempool typically is an object agnostic allocator, but bulk allocation > > > > is only supported by pages, so the mempool bulk allocator is for page > > > > allocation only as well. > > > > > > > > Signed-off-by: Yang Shi > > > > --- > > > > > > Hi Yang, > > > > > > I'm not terribly familiar with either component so I'm probably missing > > > context/details, but just a couple high level thoughts when reading your > > > patches... > > > > > > > include/linux/mempool.h | 19 ++++ > > > > mm/mempool.c | 188 +++++++++++++++++++++++++++++++++++++--- > > > > 2 files changed, 197 insertions(+), 10 deletions(-) > > > > > > > ... > > > > diff --git a/mm/mempool.c b/mm/mempool.c > > > > index ba32151f3843..7711ca2e6d66 100644 > > > > --- a/mm/mempool.c > > > > +++ b/mm/mempool.c > > > > @@ -177,6 +177,7 @@ void mempool_destroy(mempool_t *pool) > > > > EXPORT_SYMBOL(mempool_destroy); > > > > > > > > static inline int __mempool_init(mempool_t *pool, int min_nr, > > > > + mempool_alloc_pages_bulk_t *alloc_pages_bulk_fn, > > > > mempool_alloc_t *alloc_fn, > > > > mempool_free_t *free_fn, void *pool_data, > > > > gfp_t gfp_mask, int node_id) > > > > @@ -186,8 +187,11 @@ static inline int __mempool_init(mempool_t *pool, int min_nr, > > > > pool->pool_data = pool_data; > > > > pool->alloc = alloc_fn; > > > > pool->free = free_fn; > > > > + pool->alloc_pages_bulk = alloc_pages_bulk_fn; > > > > init_waitqueue_head(&pool->wait); > > > > > > > > + WARN_ON_ONCE(alloc_pages_bulk_fn && alloc_fn); > > > > + > > > > pool->elements = kmalloc_array_node(min_nr, sizeof(void *), > > > > gfp_mask, node_id); > > > > if (!pool->elements) > > > > @@ -199,7 +203,10 @@ static inline int __mempool_init(mempool_t *pool, int min_nr, > > > > while (pool->curr_nr < pool->min_nr) { > > > > void *element; > > > > > > > > - element = pool->alloc(gfp_mask, pool->pool_data); > > > > + if (pool->alloc_pages_bulk) > > > > + element = alloc_page(gfp_mask); > > > > > > Any reason to not use the callback from the caller for the bulk variant > > > here? It looks like some users might expect consistency between the > > > alloc / free callbacks for the pool. I.e., I'm not familiar with > > > dm-crypt, but the code modified in the subsequent patches looks like it > > > keeps an allocated page count. Will that still work with this, assuming > > > these pages are freed through free_fn? > > > > No special reason, this implementation just end up with fewer code > > otherwise we should need to define a list, and manipulate the list, > > seems like a little bit overkilling for initialization code. > > > > Yes, that allocated page count works, just the pages in the pool are > > not counted in the count anymore, 256 pages should be not a big deal > > IMHO. > > > > Ok. I defer to dm-crypt folks on whether/how much it might care about > pages being hidden from the accounting. My concern was partly that, but > also partly whether it was possible to break consistency between the > number of alloc and free callbacks to be expected. For example, wouldn't > these counters underflow if the mempool is torn down or shrunk (via > mempool_resize()), and thus the caller gets ->free() callbacks for pages > it never accounted for in the first place? For graceful tear down, all the pages should be freed before tear down IMHO. For mempool resize, it may be possible, but dm-crypt doesn't resize mempool IIRC. Anyway this counter is driver specific, it could be inc'ed or dec'ed when the mempool is created or resized. > > Brian > > > > > > > > + else > > > > + element = pool->alloc(gfp_mask, pool->pool_data); > > > > if (unlikely(!element)) { > > > > mempool_exit(pool); > > > > return -ENOMEM; > > > ... > > > > @@ -457,6 +499,132 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask) > > > > } > > > > EXPORT_SYMBOL(mempool_alloc); > > > > > > > > +/** > > > > + * mempool_alloc_pages_bulk - allocate a bulk of pagesfrom a specific > > > > + * memory pool > > > > + * @pool: pointer to the memory pool which was allocated via > > > > + * mempool_create(). > > > > + * @gfp_mask: the usual allocation bitmask. > > > > + * @nr: the number of requested pages. > > > > + * @page_list: the list the pages will be added to. > > > > + * @page_array: the array the pages will be added to. > > > > + * > > > > + * this function only sleeps if the alloc_pages_bulk_fn() function sleeps > > > > + * or the allocation can not be satisfied even though the mempool is depleted. > > > > + * Note that due to preallocation, this function *never* fails when called > > > > + * from process contexts. (it might fail if called from an IRQ context.) > > > > + * Note: using __GFP_ZERO is not supported. And the caller should not pass > > > > + * in both valid page_list and page_array. > > > > + * > > > > + * Return: true when nr pages are allocated or false if not. It is the > > > > + * caller's responsibility to free the partial allocated pages. > > > > + */ > > > > +static bool mempool_alloc_pages_bulk(mempool_t *pool, gfp_t gfp_mask, > > > > + unsigned int nr, > > > > + struct list_head *page_list, > > > > + struct page **page_array) > > > > +{ > > > > + unsigned long flags; > > > > + wait_queue_entry_t wait; > > > > + gfp_t gfp_temp; > > > > + int i; > > > > + unsigned int ret, nr_remaining; > > > > + struct page *page; > > > > + > > > > > > This looks like a lot of duplicate boilerplate from mempool_alloc(). > > > Could this instead do something like: rename the former to > > > __mempool_alloc() and add a count parameter, implement bulk alloc > > > support in there for count > 1, then let traditional (i.e., non-bulk) > > > mempool_alloc() callers pass a count of 1? > > > > Thanks for the suggestion. Yeah, the duplicate code is not perfect. I > > thought about this way too, but it may need to have a lot of "if > > (count > 0)" of "if (is_bulk_alloc) " if a flag is used in the code to > > handle the bulk allocation, for example, calculate remaining nr, loop > > to remove element from the pool, manipulate list or array, etc. Seems > > not that readable IMHO. > > > > We may be able to extract some common code into shared helpers, for > > example, the gfp sanitization and wait logic. > > > > > > > > Along the same lines, I also wonder if there's any value in generic bulk > > > alloc support for mempool. For example, I suppose technically this could > > > be implemented via one change to support a pool->alloc_bulk() callback > > > that any user could implement via a loop if they wanted > > > mempool_alloc_bulk() support backed by a preallocated pool. The page > > > based user could then just use that to call alloc_pages_bulk_*() as an > > > optimization without the mempool layer needing to know or care about > > > whether the underlying elements are pages or not. Hm? > > > > Thanks for the suggestion. Actually I thought about this too. But the > > memory space overhead, particularly stack space seems like a > > showstopper to me. We just can put the pointers into an array, but > > this may consume a significant amount of stack memory. One pointer is > > 8 bytes, 256 objects imply 2K stack space. Of course the users could > > move the array into a dynamic allocated data structure, but this may > > need the users modify their driver. Bulk kmalloc via kmalloc_array() > > may be fine, this is the only usercase other than pages I could think > > of. > > > > > > > > Brian > > > > > > > + VM_WARN_ON_ONCE(gfp_mask & __GFP_ZERO); > > > > + might_alloc(gfp_mask); > > > > + > > > > + gfp_mask |= __GFP_NOMEMALLOC; /* don't allocate emergency reserves */ > > > > + gfp_mask |= __GFP_NORETRY; /* don't loop in __alloc_pages */ > > > > + gfp_mask |= __GFP_NOWARN; /* failures are OK */ > > > > + > > > > + gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO); > > > > + > > > > +repeat_alloc: > > > > + i = 0; > > > > + ret = pool->alloc_pages_bulk(gfp_temp, nr, pool->pool_data, page_list, > > > > + page_array); > > > > + > > > > + if (ret == nr) > > > > + return true; > > > > + > > > > + nr_remaining = nr - ret; > > > > + > > > > + spin_lock_irqsave(&pool->lock, flags); > > > > + /* Allocate page from the pool and add to the list or array */ > > > > + while (pool->curr_nr && (nr_remaining > 0)) { > > > > + page = remove_element(pool); > > > > + spin_unlock_irqrestore(&pool->lock, flags); > > > > + smp_wmb(); > > > > + > > > > + kmemleak_update_trace((void *)page); > > > > + > > > > + if (page_list) > > > > + list_add(&page->lru, page_list); > > > > + else > > > > + page_array[ret + i] = page; > > > > + > > > > + i++; > > > > + nr_remaining--; > > > > + > > > > + spin_lock_irqsave(&pool->lock, flags); > > > > + } > > > > + > > > > + spin_unlock_irqrestore(&pool->lock, flags); > > > > + > > > > + if (!nr_remaining) > > > > + return true; > > > > + > > > > + /* > > > > + * The bulk allocator counts in the populated pages for array, > > > > + * but don't do it for list. > > > > + */ > > > > + if (page_list) > > > > + nr = nr_remaining; > > > > + > > > > + /* > > > > + * We use gfp mask w/o direct reclaim or IO for the first round. If > > > > + * alloc failed with that and @pool was empty, retry immediately. > > > > + */ > > > > + if (gfp_temp != gfp_mask) { > > > > + gfp_temp = gfp_mask; > > > > + goto repeat_alloc; > > > > + } > > > > + > > > > + /* We must not sleep if !__GFP_DIRECT_RECLAIM */ > > > > + if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) > > > > + return false; > > > > + > > > > + /* Let's wait for someone else to return an element to @pool */ > > > > + init_wait(&wait); > > > > + prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE); > > > > + > > > > + /* > > > > + * FIXME: this should be io_schedule(). The timeout is there as a > > > > + * workaround for some DM problems in 2.6.18. > > > > + */ > > > > + io_schedule_timeout(5*HZ); > > > > + > > > > + finish_wait(&pool->wait, &wait); > > > > + goto repeat_alloc; > > > > +} > > > > + > > > > +bool mempool_alloc_pages_bulk_list(mempool_t *pool, gfp_t gfp_mask, > > > > + unsigned int nr, > > > > + struct list_head *page_list) > > > > +{ > > > > + return mempool_alloc_pages_bulk(pool, gfp_mask, nr, page_list, NULL); > > > > +} > > > > +EXPORT_SYMBOL(mempool_alloc_pages_bulk_list); > > > > + > > > > +bool mempool_alloc_pages_bulk_array(mempool_t *pool, gfp_t gfp_mask, > > > > + unsigned int nr, > > > > + struct page **page_array) > > > > +{ > > > > + return mempool_alloc_pages_bulk(pool, gfp_mask, nr, NULL, page_array); > > > > +} > > > > +EXPORT_SYMBOL(mempool_alloc_pages_bulk_array); > > > > + > > > > /** > > > > * mempool_free - return an element to the pool. > > > > * @element: pool element pointer. > > > > -- > > > > 2.26.3 > > > > > > > > > >