As mentioned in [1], it seems odd to check NULL elements in
the middle of page bulk allocating, and it seems caller can
do a better job of bulk allocating pages into a whole array
sequentially without checking NULL elements first before
doing the page bulk allocation for most of existing users
by passing 'page_array + allocated' and 'nr_pages - allocated'
when calling subsequent page bulk alloc API so that NULL
checking can be avoided, see the pattern in mm/mempolicy.c.
Through analyzing of existing bulk allocation API users, it
seems only the fs users are depending on the assumption of
populating only NULL elements, see:
commit 91d6ac1d62c3 ("btrfs: allocate page arrays using bulk page allocator")
commit d6db47e571dc ("erofs: do not use pagepool in z_erofs_gbuf_growsize()")
commit f6e70aab9dfe ("SUNRPC: refresh rq_pages using a bulk page allocator")
commit 88e4d41a264d ("SUNRPC: Use __alloc_bulk_pages() in svc_init_buffer()")
The current API adds a mental burden for most users. For most
users, their code would be much cleaner if the interface accepts
an uninitialised array with length, and were told how many pages
had been stored in that array, so support one simple and one
full-featured to meet the above different use cases as below:
- alloc_pages_bulk() would be given an uninitialised array of page
pointers and a required count and would return the number of
pages that were allocated.
- alloc_pages_bulk_refill() would be given an initialised array
of page pointers some of which might be NULL. It would attempt
to allocate pages for the non-NULL pointers, return 0 if all
pages are allocated, -EAGAIN if at least one page allocated,
ok to try again immediately or -ENOMEM if don't bother trying
again soon, which provides a more consistent semantics than the
current API as mentioned in [2], at the cost of the pages might
be getting re-ordered to make the implementation simpler.
Change the existing fs users to use the full-featured API, except
for the one for svc_init_buffer() in net/sunrpc/svc.c. Other
existing callers can use the simple API as they seems to be passing
all NULL elements via memset, kzalloc, etc, only remove unnecessary
memset for existing users calling the simple API in this patch.
The test result for xfstests full test:
Before this patch:
btrfs/default: 1061 tests, 3 failures, 290 skipped, 13152 seconds
Failures: btrfs/012 btrfs/226
Flaky: generic/301: 60% (3/5)
Totals: 1073 tests, 290 skipped, 13 failures, 0 errors, 12540s
nfs/loopback: 530 tests, 3 failures, 392 skipped, 3942 seconds
Failures: generic/464 generic/551
Flaky: generic/650: 40% (2/5)
Totals: 542 tests, 392 skipped, 12 failures, 0 errors, 3799s
After this patch:
btrfs/default: 1061 tests, 2 failures, 290 skipped, 13446 seconds
Failures: btrfs/012 btrfs/226
Totals: 1069 tests, 290 skipped, 10 failures, 0 errors, 12853s
nfs/loopback: 530 tests, 3 failures, 392 skipped, 4103 seconds
Failures: generic/464 generic/551
Flaky: generic/650: 60% (3/5)
Totals: 542 tests, 392 skipped, 13 failures, 0 errors, 3933s
The stress test also suggest there is no regression for the erofs
too.
Using the simple API also enable the caller to not zero the array
before calling the page bulk allocating API, which has about 1~2 ns
performance improvement for time_bench_page_pool03_slow() test case
of page_pool in a x86 vm system, this reduces some performance impact
of fixing the DMA API misuse problem in [3], performance improves
from 87.886 ns to 86.429 ns.
Also a temporary patch to enable the using of full-featured API in
page_pool suggests that the new full-featured API doesn't seem to have
noticeable performance impact for the existing users, like SUNRPC, btrfs
and erofs.
1. https://lore.kernel.org/all/bd8c2f5c-464d-44ab-b607-390a87ea4...@huawei.com/
2. https://lore.kernel.org/all/180818a1-b906-4a0b-89d3-34cb71cc2...@huawei.com/
3. https://lore.kernel.org/all/20250212092552.1779679-1-linyunsh...@huawei.com/
CC: Jesper Dangaard Brouer <h...@kernel.org>
CC: Luiz Capitulino <luiz...@redhat.com>
CC: Mel Gorman <mgor...@techsingularity.net>
Suggested-by: Neil Brown <ne...@suse.de>
Acked-by: Jeff Layton <jlay...@kernel.org>
Signed-off-by: Yunsheng Lin <linyunsh...@huawei.com>
---
V3:
1. Provide both simple and full-featured API as suggested by NeilBrown.
2. Do the fs testing as suggested in V2.
V2:
1. Drop RFC tag.
2. Fix a compile error for xfs.
3. Defragmemt the page_array for SUNRPC and btrfs.
---
drivers/vfio/pci/mlx5/cmd.c | 2 --
drivers/vfio/pci/virtio/migrate.c | 2 --
fs/btrfs/extent_io.c | 21 +++++++++---------
fs/erofs/zutil.c | 11 +++++----
include/linux/gfp.h | 37 +++++++++++++++++++++++++++++++
include/trace/events/sunrpc.h | 12 +++++-----
kernel/bpf/arena.c | 1 -
mm/page_alloc.c | 32 +++++---------------------
net/core/page_pool.c | 3 ---
net/sunrpc/svc_xprt.c | 12 ++++++----
10 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
index 11eda6b207f1..fb094527715f 100644
--- a/drivers/vfio/pci/mlx5/cmd.c
+++ b/drivers/vfio/pci/mlx5/cmd.c
@@ -446,8 +446,6 @@ static int mlx5vf_add_migration_pages(struct
mlx5_vhca_data_buffer *buf,
if (ret)
goto err_append;
buf->allocated_length += filled * PAGE_SIZE;
- /* clean input for another bulk allocation */
- memset(page_list, 0, filled * sizeof(*page_list));
to_fill = min_t(unsigned int, to_alloc,
PAGE_SIZE / sizeof(*page_list));
} while (to_alloc > 0);
diff --git a/drivers/vfio/pci/virtio/migrate.c
b/drivers/vfio/pci/virtio/migrate.c
index ba92bb4e9af9..9f003a237dec 100644
--- a/drivers/vfio/pci/virtio/migrate.c
+++ b/drivers/vfio/pci/virtio/migrate.c
@@ -91,8 +91,6 @@ static int virtiovf_add_migration_pages(struct
virtiovf_data_buffer *buf,
if (ret)
goto err_append;
buf->allocated_length += filled * PAGE_SIZE;
- /* clean input for another bulk allocation */
- memset(page_list, 0, filled * sizeof(*page_list));
to_fill = min_t(unsigned int, to_alloc,
PAGE_SIZE / sizeof(*page_list));
} while (to_alloc > 0);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 197f5e51c474..51ef15703900 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -623,21 +623,22 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct
page **page_array,
bool nofail)
{
const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS;
- unsigned int allocated;
-
- for (allocated = 0; allocated < nr_pages;) {
- unsigned int last = allocated;
+ int ret;
- allocated = alloc_pages_bulk(gfp, nr_pages, page_array);
- if (unlikely(allocated == last)) {
+ do {
+ ret = alloc_pages_bulk_refill(gfp, nr_pages, page_array);
+ if (unlikely(ret == -ENOMEM)) {
/* No progress, fail and do cleanup. */
- for (int i = 0; i < allocated; i++) {
- __free_page(page_array[i]);
- page_array[i] = NULL;
+ for (int i = 0; i < nr_pages; i++) {
+ if (page_array[i]) {
+ __free_page(page_array[i]);
+ page_array[i] = NULL;
+ }
}
return -ENOMEM;
}
- }
+ } while (ret == -EAGAIN);
+
return 0;
}
diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
index 55ff2ab5128e..6ce11a8a261c 100644
--- a/fs/erofs/zutil.c
+++ b/fs/erofs/zutil.c
@@ -68,7 +68,7 @@ int z_erofs_gbuf_growsize(unsigned int nrpages)
struct page **tmp_pages = NULL;
struct z_erofs_gbuf *gbuf;
void *ptr, *old_ptr;
- int last, i, j;
+ int ret, i, j;
mutex_lock(&gbuf_resize_mutex);
/* avoid shrinking gbufs, since no idea how many fses rely on */
@@ -86,12 +86,11 @@ int z_erofs_gbuf_growsize(unsigned int nrpages)
for (j = 0; j < gbuf->nrpages; ++j)
tmp_pages[j] = gbuf->pages[j];
do {
- last = j;
- j = alloc_pages_bulk(GFP_KERNEL, nrpages,
- tmp_pages);
- if (last == j)
+ ret = alloc_pages_bulk_refill(GFP_KERNEL, nrpages,
+ tmp_pages);
+ if (ret == -ENOMEM)
goto out;
- } while (j != nrpages);
+ } while (ret == -EAGAIN);
ptr = vmap(tmp_pages, nrpages, VM_MAP, PAGE_KERNEL);
if (!ptr)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index c9fa6309c903..cf6100981fd6 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -244,6 +244,43 @@ unsigned long alloc_pages_bulk_mempolicy_noprof(gfp_t gfp,
#define alloc_pages_bulk(_gfp, _nr_pages, _page_array) \
__alloc_pages_bulk(_gfp, numa_mem_id(), NULL, _nr_pages, _page_array)
+/*
+ * alloc_pages_bulk_refill_noprof - Refill order-0 pages to an array
+ * @gfp: GFP flags for the allocation when refilling
+ * @nr_pages: The size of refilling array
+ * @page_array: The array to refill order-0 pages
+ *
+ * Note that only NULL elements are populated with pages and the pages might
+ * get re-ordered.
+ *
+ * Return 0 if all pages are refilled, -EAGAIN if at least one page is
refilled,
+ * ok to try again immediately or -ENOMEM if no page is refilled and don't
+ * bother trying again soon.
+ */
+static inline int alloc_pages_bulk_refill_noprof(gfp_t gfp, int nr_pages,
+ struct page **page_array)
+{
+ int allocated = 0, i;
+
+ for (i = 0; i < nr_pages; i++) {
+ if (page_array[i]) {
+ swap(page_array[allocated], page_array[i]);
+ allocated++;
+ }
+ }
+
+ i = alloc_pages_bulk_noprof(gfp, numa_mem_id(), NULL,
+ nr_pages - allocated,
+ page_array + allocated);
+ if (likely(allocated + i == nr_pages))
+ return 0;
+
+ return i ? -EAGAIN : -ENOMEM;
+}
+
+#define alloc_pages_bulk_refill(...) \
+ alloc_hooks(alloc_pages_bulk_refill_noprof(__VA_ARGS__))
+
static inline unsigned long
alloc_pages_bulk_node_noprof(gfp_t gfp, int nid, unsigned long nr_pages,
struct page **page_array)
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 5d331383047b..cb8899f1cbdc 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -2143,23 +2143,23 @@ TRACE_EVENT(svc_wake_up,
TRACE_EVENT(svc_alloc_arg_err,
TP_PROTO(
unsigned int requested,
- unsigned int allocated
+ int ret
),
- TP_ARGS(requested, allocated),
+ TP_ARGS(requested, ret),
TP_STRUCT__entry(
__field(unsigned int, requested)
- __field(unsigned int, allocated)
+ __field(int, ret)
),
TP_fast_assign(
__entry->requested = requested;
- __entry->allocated = allocated;
+ __entry->ret = ret;
),
- TP_printk("requested=%u allocated=%u",
- __entry->requested, __entry->allocated)
+ TP_printk("requested=%u ret=%d",
+ __entry->requested, __entry->ret)
);
DECLARE_EVENT_CLASS(svc_deferred_event,
diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index 0d56cea71602..9022c4440814 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -445,7 +445,6 @@ static long arena_alloc_pages(struct bpf_arena *arena, long
uaddr, long page_cnt
return 0;
}
- /* zeroing is needed, since alloc_pages_bulk() only fills in non-zero
entries */
pages = kvcalloc(page_cnt, sizeof(struct page *), GFP_KERNEL);
if (!pages)
return 0;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d7cfcfa2b077..59a4fe23e62a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4784,9 +4784,6 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask,
unsigned int order,
* This is a batched version of the page allocator that attempts to
* allocate nr_pages quickly. Pages are added to the page_array.
*
- * Note that only NULL elements are populated with pages and nr_pages
- * is the maximum number of pages that will be stored in the array.
- *
* Returns the number of pages in the array.
*/
unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
@@ -4802,29 +4799,18 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int
preferred_nid,
struct alloc_context ac;
gfp_t alloc_gfp;
unsigned int alloc_flags = ALLOC_WMARK_LOW;
- int nr_populated = 0, nr_account = 0;
-
- /*
- * Skip populated array elements to determine if any pages need
- * to be allocated before disabling IRQs.
- */
- while (nr_populated < nr_pages && page_array[nr_populated])
- nr_populated++;
+ int nr_populated = 0;
/* No pages requested? */
if (unlikely(nr_pages <= 0))
goto out;
- /* Already populated array? */
- if (unlikely(nr_pages - nr_populated == 0))
- goto out;
-
/* Bulk allocator does not support memcg accounting. */
if (memcg_kmem_online() && (gfp & __GFP_ACCOUNT))
goto failed;
/* Use the single page allocator for one page. */
- if (nr_pages - nr_populated == 1)
+ if (nr_pages == 1)
goto failed;
#ifdef CONFIG_PAGE_OWNER
@@ -4896,24 +4882,16 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int
preferred_nid,
/* Attempt the batch allocation */
pcp_list = &pcp->lists[order_to_pindex(ac.migratetype, 0)];
while (nr_populated < nr_pages) {
-
- /* Skip existing pages */
- if (page_array[nr_populated]) {
- nr_populated++;
- continue;
- }
-
page = __rmqueue_pcplist(zone, 0, ac.migratetype, alloc_flags,
pcp, pcp_list);
if (unlikely(!page)) {
/* Try and allocate at least one page */
- if (!nr_account) {
+ if (!nr_populated) {
pcp_spin_unlock(pcp);
goto failed_irq;
}
break;
}
- nr_account++;
prep_new_page(page, 0, gfp, 0);
set_page_refcounted(page);
@@ -4923,8 +4901,8 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int
preferred_nid,
pcp_spin_unlock(pcp);
pcp_trylock_finish(UP_flags);
- __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
- zone_statistics(zonelist_zone(ac.preferred_zoneref), zone, nr_account);
+ __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_populated);
+ zone_statistics(zonelist_zone(ac.preferred_zoneref), zone,
nr_populated);
out:
return nr_populated;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 7745ad924ae2..2431d2f6d610 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -541,9 +541,6 @@ static noinline netmem_ref
__page_pool_alloc_pages_slow(struct page_pool *pool,
if (unlikely(pool->alloc.count > 0))
return pool->alloc.cache[--pool->alloc.count];
- /* Mark empty alloc.cache slots "empty" for alloc_pages_bulk */
- memset(&pool->alloc.cache, 0, sizeof(void *) * bulk);
-
nr_pages = alloc_pages_bulk_node(gfp, pool->p.nid, bulk,
(struct page **)pool->alloc.cache);
if (unlikely(!nr_pages))
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index ae25405d8bd2..1191686fc0af 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -653,7 +653,8 @@ static bool svc_alloc_arg(struct svc_rqst *rqstp)
{
struct svc_serv *serv = rqstp->rq_server;
struct xdr_buf *arg = &rqstp->rq_arg;
- unsigned long pages, filled, ret;
+ unsigned long pages;
+ int ret;
pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
if (pages > RPCSVC_MAXPAGES) {
@@ -663,9 +664,12 @@ static bool svc_alloc_arg(struct svc_rqst *rqstp)
pages = RPCSVC_MAXPAGES;
}
- for (filled = 0; filled < pages; filled = ret) {
- ret = alloc_pages_bulk(GFP_KERNEL, pages, rqstp->rq_pages);
- if (ret > filled)
+ while (true) {
+ ret = alloc_pages_bulk_refill(GFP_KERNEL, pages,
rqstp->rq_pages);
+ if (!ret)
+ break;
+
+ if (ret == -EAGAIN)
/* Made progress, don't sleep yet */
continue;
--
2.33.0
