[PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Mel Gorman]

This series is based on top of Matthew Wilcox's series "Rationalise
__alloc_pages wrapper" and does not apply to 5.14-rc4. If Andrew's tree
is not the testing baseline then the following git tree will work.

git://git.kernel.org/pub/scm/linux/kernel/git/mel/linux.git mm-bulk-rebase-v6r7

Changelog since v5
o Add micro-optimisations from Jesper
o Add array-based versions of the sunrpc and page_pool users
o Allocate 1 page if local zone watermarks are not met
o Fix statistics
o prep_new_pages as they are allocated. Batching prep_new_pages with
  IRQs enabled limited how the API could be used (e.g. list must be
  empty) and added too much complexity.

Changelog since v4
o Drop users of the API
o Remove free_pages_bulk interface, no users
o Add array interface
o Allocate single page if watermark checks on local zones fail

Changelog since v3
o Rebase on top of Matthew's series consolidating the alloc_pages API
o Rename alloced to allocated
o Split out preparation patch for prepare_alloc_pages
o Defensive check for bulk allocation or <= 0 pages
o Call single page allocation path only if no pages were allocated
o Minor cosmetic cleanups
o Reorder patch dependencies by subsystem. As this is a cross-subsystem
  series, the mm patches have to be merged before the sunrpc and net
  users.

Changelog since v2
o Prep new pages with IRQs enabled
o Minor documentation update

Changelog since v1
o Parenthesise binary and boolean comparisons
o Add reviewed-bys
o Rebase to 5.12-rc2

This series introduces a bulk order-0 page allocator with sunrpc and
the network page pool being the first users. The implementation is not
efficient as semantics needed to be ironed out first. If no other semantic
changes are needed, it can be made more efficient.  Despite that, this
is a performance-related for users that require multiple pages for an
operation without multiple round-trips to the page allocator. Quoting
the last patch for the high-speed networking use-case

            Kernel          XDP stats       CPU     pps           Delta
            Baseline        XDP-RX CPU      total   3,771,046       n/a
            List            XDP-RX CPU      total   3,940,242    +4.49%
            Array           XDP-RX CPU      total   4,249,224   +12.68%

From the SUNRPC traces of svc_alloc_arg()

	Single page: 25.007 us per call over 532,571 calls
	Bulk list:    6.258 us per call over 517,034 calls
	Bulk array:   4.590 us per call over 517,442 calls

Both potential users in this series are corner cases (NFS and high-speed
networks) so it is unlikely that most users will see any benefit in the
short term. Other potential other users are batch allocations for page
cache readahead, fault around and SLUB allocations when high-order pages
are unavailable. It's unknown how much benefit would be seen by converting
multiple page allocation calls to a single batch or what difference it may
make to headline performance.

Light testing of my own running dbench over NFS passed. Chuck and Jesper
conducted their own tests and details are included in the changelogs.

Patch 1 renames a variable name that is particularly unpopular

Patch 2 adds a bulk page allocator

Patch 3 adds an array-based version of the bulk allocator

Patches 4-5 adds micro-optimisations to the implementation

Patches 6-7 SUNRPC user

Patches 8-9 Network page_pool user

 include/linux/gfp.h     |  18 +++++
 include/net/page_pool.h |   2 +-
 mm/page_alloc.c         | 157 ++++++++++++++++++++++++++++++++++++++--
 net/core/page_pool.c    | 111 ++++++++++++++++++----------
 net/sunrpc/svc_xprt.c   |  38 +++++-----
 5 files changed, 263 insertions(+), 63 deletions(-)

-- 
2.26.2

[PATCH 1/9] mm/page_alloc: Rename alloced to allocated

[Mel Gorman]

Review feedback of the bulk allocator twice found problems with "alloced"
being a counter for pages allocated. The naming was based on the API name
"alloc" and was based on the idea that verbal communication about malloc
tends to use the fake word "malloced" instead of the fake word mallocated.
To be consistent, this preparation patch renames alloced to allocated
in rmqueue_bulk so the bulk allocator and per-cpu allocator use similar
names when the bulk allocator is introduced.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index dfa9af064f74..8a3e13277e22 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2908,7 +2908,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 			unsigned long count, struct list_head *list,
 			int migratetype, unsigned int alloc_flags)
 {
-	int i, alloced = 0;
+	int i, allocated = 0;
 
 	spin_lock(&zone->lock);
 	for (i = 0; i < count; ++i) {
@@ -2931,7 +2931,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 		 * pages are ordered properly.
 		 */
 		list_add_tail(&page->lru, list);
-		alloced++;
+		allocated++;
 		if (is_migrate_cma(get_pcppage_migratetype(page)))
 			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
 					      -(1 << order));
@@ -2940,12 +2940,12 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 	/*
 	 * i pages were removed from the buddy list even if some leak due
 	 * to check_pcp_refill failing so adjust NR_FREE_PAGES based
-	 * on i. Do not confuse with 'alloced' which is the number of
+	 * on i. Do not confuse with 'allocated' which is the number of
 	 * pages added to the pcp list.
 	 */
 	__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
 	spin_unlock(&zone->lock);
-	return alloced;
+	return allocated;
 }
 
 #ifdef CONFIG_NUMA
-- 
2.26.2

Re: [PATCH 1/9] mm/page_alloc: Rename alloced to allocated

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 11:42:20AM +0000, Mel Gorman wrote:
> Review feedback of the bulk allocator twice found problems with "alloced"
> being a counter for pages allocated. The naming was based on the API name
> "alloc" and was based on the idea that verbal communication about malloc
> tends to use the fake word "malloced" instead of the fake word mallocated.
> To be consistent, this preparation patch renames alloced to allocated
> in rmqueue_bulk so the bulk allocator and per-cpu allocator use similar
> names when the bulk allocator is introduced.
> 
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH 1/9] mm/page_alloc: Rename alloced to allocated

[Vlastimil Babka]

On 3/25/21 12:42 PM, Mel Gorman wrote:
> Review feedback of the bulk allocator twice found problems with "alloced"
> being a counter for pages allocated. The naming was based on the API name
> "alloc" and was based on the idea that verbal communication about malloc
> tends to use the fake word "malloced" instead of the fake word mallocated.
> To be consistent, this preparation patch renames alloced to allocated
> in rmqueue_bulk so the bulk allocator and per-cpu allocator use similar
> names when the bulk allocator is introduced.
> 
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index dfa9af064f74..8a3e13277e22 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2908,7 +2908,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
>  			unsigned long count, struct list_head *list,
>  			int migratetype, unsigned int alloc_flags)
>  {
> -	int i, alloced = 0;
> +	int i, allocated = 0;
>  
>  	spin_lock(&zone->lock);
>  	for (i = 0; i < count; ++i) {
> @@ -2931,7 +2931,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
>  		 * pages are ordered properly.
>  		 */
>  		list_add_tail(&page->lru, list);
> -		alloced++;
> +		allocated++;
>  		if (is_migrate_cma(get_pcppage_migratetype(page)))
>  			__mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
>  					      -(1 << order));
> @@ -2940,12 +2940,12 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
>  	/*
>  	 * i pages were removed from the buddy list even if some leak due
>  	 * to check_pcp_refill failing so adjust NR_FREE_PAGES based
> -	 * on i. Do not confuse with 'alloced' which is the number of
> +	 * on i. Do not confuse with 'allocated' which is the number of
>  	 * pages added to the pcp list.
>  	 */
>  	__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
>  	spin_unlock(&zone->lock);
> -	return alloced;
> +	return allocated;
>  }
>  
>  #ifdef CONFIG_NUMA
> 

[PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Mel Gorman]

This patch adds a new page allocator interface via alloc_pages_bulk,
and __alloc_pages_bulk_nodemask. A caller requests a number of pages
to be allocated and added to a list.

The API is not guaranteed to return the requested number of pages and
may fail if the preferred allocation zone has limited free memory, the
cpuset changes during the allocation or page debugging decides to fail
an allocation. It's up to the caller to request more pages in batch
if necessary.

Note that this implementation is not very efficient and could be improved
but it would require refactoring. The intent is to make it available early
to determine what semantics are required by different callers. Once the
full semantics are nailed down, it can be refactored.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/gfp.h |  11 +++++
 mm/page_alloc.c     | 118 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0a88f84b08f4..4a304fd39916 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -518,6 +518,17 @@ static inline int arch_make_page_accessible(struct page *page)
 struct page *__alloc_pages(gfp_t gfp, unsigned int order, int preferred_nid,
 		nodemask_t *nodemask);
 
+int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
+				nodemask_t *nodemask, int nr_pages,
+				struct list_head *list);
+
+/* Bulk allocate order-0 pages */
+static inline unsigned long
+alloc_pages_bulk(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
+{
+	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list);
+}
+
 /*
  * Allocate pages, preferring the node given as nid. The node must be valid and
  * online. For more general interface, see alloc_pages_node().
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8a3e13277e22..eb547470a7e4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4965,6 +4965,124 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 	return true;
 }
 
+/*
+ * __alloc_pages_bulk - Allocate a number of order-0 pages to a list
+ * @gfp: GFP flags for the allocation
+ * @preferred_nid: The preferred NUMA node ID to allocate from
+ * @nodemask: Set of nodes to allocate from, may be NULL
+ * @nr_pages: The number of pages desired on the list
+ * @page_list: List to store the allocated pages
+ *
+ * This is a batched version of the page allocator that attempts to
+ * allocate nr_pages quickly and add them to a list.
+ *
+ * Returns the number of pages on the list.
+ */
+int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
+			nodemask_t *nodemask, int nr_pages,
+			struct list_head *page_list)
+{
+	struct page *page;
+	unsigned long flags;
+	struct zone *zone;
+	struct zoneref *z;
+	struct per_cpu_pages *pcp;
+	struct list_head *pcp_list;
+	struct alloc_context ac;
+	gfp_t alloc_gfp;
+	unsigned int alloc_flags;
+	int allocated = 0;
+
+	if (WARN_ON_ONCE(nr_pages <= 0))
+		return 0;
+
+	/* Use the single page allocator for one page. */
+	if (nr_pages == 1)
+		goto failed;
+
+	/* May set ALLOC_NOFRAGMENT, fragmentation will return 1 page. */
+	gfp &= gfp_allowed_mask;
+	alloc_gfp = gfp;
+	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &alloc_gfp, &alloc_flags))
+		return 0;
+	gfp = alloc_gfp;
+
+	/* Find an allowed local zone that meets the high watermark. */
+	for_each_zone_zonelist_nodemask(zone, z, ac.zonelist, ac.highest_zoneidx, ac.nodemask) {
+		unsigned long mark;
+
+		if (cpusets_enabled() && (alloc_flags & ALLOC_CPUSET) &&
+		    !__cpuset_zone_allowed(zone, gfp)) {
+			continue;
+		}
+
+		if (nr_online_nodes > 1 && zone != ac.preferred_zoneref->zone &&
+		    zone_to_nid(zone) != zone_to_nid(ac.preferred_zoneref->zone)) {
+			goto failed;
+		}
+
+		mark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK) + nr_pages;
+		if (zone_watermark_fast(zone, 0,  mark,
+				zonelist_zone_idx(ac.preferred_zoneref),
+				alloc_flags, gfp)) {
+			break;
+		}
+	}
+
+	/*
+	 * If there are no allowed local zones that meets the watermarks then
+	 * try to allocate a single page and reclaim if necessary.
+	 */
+	if (!zone)
+		goto failed;
+
+	/* Attempt the batch allocation */
+	local_irq_save(flags);
+	pcp = &this_cpu_ptr(zone->pageset)->pcp;
+	pcp_list = &pcp->lists[ac.migratetype];
+
+	while (allocated < nr_pages) {
+		page = __rmqueue_pcplist(zone, ac.migratetype, alloc_flags,
+								pcp, pcp_list);
+		if (!page) {
+			/* Try and get at least one page */
+			if (!allocated)
+				goto failed_irq;
+			break;
+		}
+
+		/*
+		 * Ideally this would be batched but the best way to do
+		 * that cheaply is to first convert zone_statistics to
+		 * be inaccurate per-cpu counter like vm_events to avoid
+		 * a RMW cycle then do the accounting with IRQs enabled.
+		 */
+		__count_zid_vm_events(PGALLOC, zone_idx(zone), 1);
+		zone_statistics(ac.preferred_zoneref->zone, zone);
+
+		prep_new_page(page, 0, gfp, 0);
+		list_add(&page->lru, page_list);
+		allocated++;
+	}
+
+	local_irq_restore(flags);
+
+	return allocated;
+
+failed_irq:
+	local_irq_restore(flags);
+
+failed:
+	page = __alloc_pages(gfp, 0, preferred_nid, nodemask);
+	if (page) {
+		list_add(&page->lru, page_list);
+		allocated = 1;
+	}
+
+	return allocated;
+}
+EXPORT_SYMBOL_GPL(__alloc_pages_bulk);
+
 /*
  * This is the 'heart' of the zoned buddy allocator.
  */
-- 
2.26.2

Re: [PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 11:42:21AM +0000, Mel Gorman wrote:
> +int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> +				nodemask_t *nodemask, int nr_pages,
> +				struct list_head *list);
> +
> +/* Bulk allocate order-0 pages */
> +static inline unsigned long
> +alloc_pages_bulk(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
> +{
> +	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list);

Discrepancy in the two return types here.  Suspect they should both
be 'unsigned int' so there's no question about "can it return an errno".

>  
> +/*

If you could make that "/**" instead ...

Re: [PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Mel Gorman]

On Thu, Mar 25, 2021 at 12:05:25PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 25, 2021 at 11:42:21AM +0000, Mel Gorman wrote:
> > +int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> > +				nodemask_t *nodemask, int nr_pages,
> > +				struct list_head *list);
> > +
> > +/* Bulk allocate order-0 pages */
> > +static inline unsigned long
> > +alloc_pages_bulk(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
> > +{
> > +	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list);
> 
> Discrepancy in the two return types here.  Suspect they should both
> be 'unsigned int' so there's no question about "can it return an errno".
> 

I'll make it unsigned long as the nr_pages parameter is unsigned long.
It's a silly range to have for pages but it matches alloc_contig_range
even though free_contig_range takes unsigned int *sigh*

> >  
> > +/*
> 
> If you could make that "/**" instead ...
> 

I decided not to until we're reasonably sure the semantics are not going
to change.

---8<---
mm/page_alloc: Add a bulk page allocator -fix

Matthew Wilcox pointed out that the return type for alloc_pages_bulk()
and __alloc_pages_bulk() is inconsistent. Fix it.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 include/linux/gfp.h | 2 +-
 mm/page_alloc.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 4a304fd39916..a2be8f4174a9 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -518,7 +518,7 @@ static inline int arch_make_page_accessible(struct page *page)
 struct page *__alloc_pages(gfp_t gfp, unsigned int order, int preferred_nid,
 		nodemask_t *nodemask);
 
-int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
+unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 				nodemask_t *nodemask, int nr_pages,
 				struct list_head *list);
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eb547470a7e4..92d55f80c289 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4978,7 +4978,7 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
  *
  * Returns the number of pages on the list.
  */
-int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
+unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 			nodemask_t *nodemask, int nr_pages,
 			struct list_head *page_list)
 {

Re: [PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Vlastimil Babka]

On 3/25/21 12:42 PM, Mel Gorman wrote:
> This patch adds a new page allocator interface via alloc_pages_bulk,
> and __alloc_pages_bulk_nodemask. A caller requests a number of pages
> to be allocated and added to a list.
> 
> The API is not guaranteed to return the requested number of pages and
> may fail if the preferred allocation zone has limited free memory, the
> cpuset changes during the allocation or page debugging decides to fail
> an allocation. It's up to the caller to request more pages in batch
> if necessary.
> 
> Note that this implementation is not very efficient and could be improved
> but it would require refactoring. The intent is to make it available early
> to determine what semantics are required by different callers. Once the
> full semantics are nailed down, it can be refactored.
> 
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/gfp.h |  11 +++++
>  mm/page_alloc.c     | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 129 insertions(+)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0a88f84b08f4..4a304fd39916 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -518,6 +518,17 @@ static inline int arch_make_page_accessible(struct page *page)
>  struct page *__alloc_pages(gfp_t gfp, unsigned int order, int preferred_nid,
>  		nodemask_t *nodemask);
>  
> +int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> +				nodemask_t *nodemask, int nr_pages,
> +				struct list_head *list);
> +
> +/* Bulk allocate order-0 pages */
> +static inline unsigned long
> +alloc_pages_bulk(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
> +{
> +	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list);
> +}
> +
>  /*
>   * Allocate pages, preferring the node given as nid. The node must be valid and
>   * online. For more general interface, see alloc_pages_node().
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 8a3e13277e22..eb547470a7e4 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4965,6 +4965,124 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
>  	return true;
>  }
>  
> +/*
> + * __alloc_pages_bulk - Allocate a number of order-0 pages to a list
> + * @gfp: GFP flags for the allocation
> + * @preferred_nid: The preferred NUMA node ID to allocate from
> + * @nodemask: Set of nodes to allocate from, may be NULL
> + * @nr_pages: The number of pages desired on the list
> + * @page_list: List to store the allocated pages
> + *
> + * This is a batched version of the page allocator that attempts to
> + * allocate nr_pages quickly and add them to a list.
> + *
> + * Returns the number of pages on the list.
> + */
> +int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> +			nodemask_t *nodemask, int nr_pages,
> +			struct list_head *page_list)
> +{
> +	struct page *page;
> +	unsigned long flags;
> +	struct zone *zone;
> +	struct zoneref *z;
> +	struct per_cpu_pages *pcp;
> +	struct list_head *pcp_list;
> +	struct alloc_context ac;
> +	gfp_t alloc_gfp;
> +	unsigned int alloc_flags;

Was going to complain that this is not set to ALLOC_WMARK_LOW. Must be faster
next time...

> +	int allocated = 0;
> +
> +	if (WARN_ON_ONCE(nr_pages <= 0))
> +		return 0;
> +
> +	/* Use the single page allocator for one page. */
> +	if (nr_pages == 1)
> +		goto failed;
> +
> +	/* May set ALLOC_NOFRAGMENT, fragmentation will return 1 page. */

I don't understand this comment. Only alloc_flags_nofragment() sets this flag
and we don't use it here?

> +	gfp &= gfp_allowed_mask;
> +	alloc_gfp = gfp;
> +	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &alloc_gfp, &alloc_flags))
> +		return 0;
> +	gfp = alloc_gfp;
> +
> +	/* Find an allowed local zone that meets the high watermark. */

Should it say "low watermark"?

Vlastimil

Re: [PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Mel Gorman]

On Mon, Apr 12, 2021 at 12:21:42PM +0200, Vlastimil Babka wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 8a3e13277e22..eb547470a7e4 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4965,6 +4965,124 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
> >  	return true;
> >  }
> >  
> > +/*
> > + * __alloc_pages_bulk - Allocate a number of order-0 pages to a list
> > + * @gfp: GFP flags for the allocation
> > + * @preferred_nid: The preferred NUMA node ID to allocate from
> > + * @nodemask: Set of nodes to allocate from, may be NULL
> > + * @nr_pages: The number of pages desired on the list
> > + * @page_list: List to store the allocated pages
> > + *
> > + * This is a batched version of the page allocator that attempts to
> > + * allocate nr_pages quickly and add them to a list.
> > + *
> > + * Returns the number of pages on the list.
> > + */
> > +int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> > +			nodemask_t *nodemask, int nr_pages,
> > +			struct list_head *page_list)
> > +{
> > +	struct page *page;
> > +	unsigned long flags;
> > +	struct zone *zone;
> > +	struct zoneref *z;
> > +	struct per_cpu_pages *pcp;
> > +	struct list_head *pcp_list;
> > +	struct alloc_context ac;
> > +	gfp_t alloc_gfp;
> > +	unsigned int alloc_flags;
> 
> Was going to complain that this is not set to ALLOC_WMARK_LOW. Must be faster
> next time...
> 

Good that you caught it anyway!

> > +	int allocated = 0;
> > +
> > +	if (WARN_ON_ONCE(nr_pages <= 0))
> > +		return 0;
> > +
> > +	/* Use the single page allocator for one page. */
> > +	if (nr_pages == 1)
> > +		goto failed;
> > +
> > +	/* May set ALLOC_NOFRAGMENT, fragmentation will return 1 page. */
> 
> I don't understand this comment. Only alloc_flags_nofragment() sets this flag
> and we don't use it here?
> 

It's there as a reminder that there are non-obvious consequences
to ALLOC_NOFRAGMENT that may affect the bulk allocation success
rate. __rmqueue_fallback will only select pageblock_order pages and if that
fails, we fall into the slow path that allocates a single page. I didn't
deal with it because it was not obvious that it's even relevant but I bet
in 6 months time, I'll forget that ALLOC_NOFRAGMENT may affect success
rates without the comment. I'm waiting for a bug that can trivially trigger
a case with a meaningful workload where the success rate is poor enough to
affect latency before adding complexity. Ideally by then, the allocation
paths would be unified a bit better.

> > +	gfp &= gfp_allowed_mask;
> > +	alloc_gfp = gfp;
> > +	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &alloc_gfp, &alloc_flags))
> > +		return 0;
> > +	gfp = alloc_gfp;
> > +
> > +	/* Find an allowed local zone that meets the high watermark. */
> 
> Should it say "low watermark"?
> 

Yeah, that's leftover from an earlier prototype :(

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/9] mm/page_alloc: Add a bulk page allocator

[Mel Gorman]

On Mon, Apr 12, 2021 at 11:59:38AM +0100, Mel Gorman wrote:
> > I don't understand this comment. Only alloc_flags_nofragment() sets this flag
> > and we don't use it here?
> > 
> 
> It's there as a reminder that there are non-obvious consequences
> to ALLOC_NOFRAGMENT that may affect the bulk allocation success
> rate. __rmqueue_fallback will only select pageblock_order pages and if that
> fails, we fall into the slow path that allocates a single page. I didn't
> deal with it because it was not obvious that it's even relevant but I bet
> in 6 months time, I'll forget that ALLOC_NOFRAGMENT may affect success
> rates without the comment. I'm waiting for a bug that can trivially trigger
> a case with a meaningful workload where the success rate is poor enough to
> affect latency before adding complexity. Ideally by then, the allocation
> paths would be unified a bit better.
> 

So this needs better clarification. ALLOC_NOFRAGMENT is not a
problem at the moment but at one point during development, it was a
non-obvious potential problem. If the paths are unified, ALLOC_NOFRAGMENT
*potentially* becomes a problem depending on how it's done and it needs
careful consideration. For example, it could be part unified by moving
the alloc_flags_nofragment() call into prepare_alloc_pages because in
__alloc_pages, it always happens and it looks like an obvious partial
unification. Hence the comment "May set ALLOC_NOFRAGMENT" because I wanted
a reminder in case I "fixed" this in 6 months time and forgot the downside.

-- 
Mel Gorman
SUSE Labs

[PATCH 3/9] mm/page_alloc: Add an array-based interface to the bulk page allocator

[Mel Gorman]

The proposed callers for the bulk allocator store pages from the bulk
allocator in an array. This patch adds an array-based interface to the API
to avoid multiple list iterations. The page list interface is preserved
to avoid requiring all users of the bulk API to allocate and manage enough
storage to store the pages.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 include/linux/gfp.h | 13 +++++++---
 mm/page_alloc.c     | 60 +++++++++++++++++++++++++++++++++------------
 2 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 4a304fd39916..fb6234e1fe59 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -520,13 +520,20 @@ struct page *__alloc_pages(gfp_t gfp, unsigned int order, int preferred_nid,
 
 int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 				nodemask_t *nodemask, int nr_pages,
-				struct list_head *list);
+				struct list_head *page_list,
+				struct page **page_array);
 
 /* Bulk allocate order-0 pages */
 static inline unsigned long
-alloc_pages_bulk(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
+alloc_pages_bulk_list(gfp_t gfp, unsigned long nr_pages, struct list_head *list)
 {
-	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list);
+	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, list, NULL);
+}
+
+static inline unsigned long
+alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
+{
+	return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
 }
 
 /*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eb547470a7e4..be1e33a4df39 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4966,21 +4966,29 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 }
 
 /*
- * __alloc_pages_bulk - Allocate a number of order-0 pages to a list
+ * __alloc_pages_bulk - Allocate a number of order-0 pages to a list or array
  * @gfp: GFP flags for the allocation
  * @preferred_nid: The preferred NUMA node ID to allocate from
  * @nodemask: Set of nodes to allocate from, may be NULL
- * @nr_pages: The number of pages desired on the list
- * @page_list: List to store the allocated pages
+ * @nr_pages: The number of pages desired on the list or array
+ * @page_list: Optional list to store the allocated pages
+ * @page_array: Optional array to store the pages
  *
  * This is a batched version of the page allocator that attempts to
- * allocate nr_pages quickly and add them to a list.
+ * allocate nr_pages quickly. Pages are added to page_list if page_list
+ * is not NULL, otherwise it is assumed that the page_array is valid.
  *
- * Returns the number of pages on the list.
+ * For lists, nr_pages is the number of pages that should be allocated.
+ *
+ * For arrays, 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 on the list or array.
  */
 int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 			nodemask_t *nodemask, int nr_pages,
-			struct list_head *page_list)
+			struct list_head *page_list,
+			struct page **page_array)
 {
 	struct page *page;
 	unsigned long flags;
@@ -4991,13 +4999,20 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	struct alloc_context ac;
 	gfp_t alloc_gfp;
 	unsigned int alloc_flags;
-	int allocated = 0;
+	int nr_populated = 0;
 
 	if (WARN_ON_ONCE(nr_pages <= 0))
 		return 0;
 
+	/*
+	 * Skip populated array elements to determine if any pages need
+	 * to be allocated before disabling IRQs.
+	 */
+	while (page_array && page_array[nr_populated] && nr_populated < nr_pages)
+		nr_populated++;
+
 	/* Use the single page allocator for one page. */
-	if (nr_pages == 1)
+	if (nr_pages - nr_populated == 1)
 		goto failed;
 
 	/* May set ALLOC_NOFRAGMENT, fragmentation will return 1 page. */
@@ -5041,12 +5056,19 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	pcp = &this_cpu_ptr(zone->pageset)->pcp;
 	pcp_list = &pcp->lists[ac.migratetype];
 
-	while (allocated < nr_pages) {
+	while (nr_populated < nr_pages) {
+
+		/* Skip existing pages */
+		if (page_array && page_array[nr_populated]) {
+			nr_populated++;
+			continue;
+		}
+
 		page = __rmqueue_pcplist(zone, ac.migratetype, alloc_flags,
 								pcp, pcp_list);
 		if (!page) {
 			/* Try and get at least one page */
-			if (!allocated)
+			if (!nr_populated)
 				goto failed_irq;
 			break;
 		}
@@ -5061,13 +5083,16 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 		zone_statistics(ac.preferred_zoneref->zone, zone);
 
 		prep_new_page(page, 0, gfp, 0);
-		list_add(&page->lru, page_list);
-		allocated++;
+		if (page_list)
+			list_add(&page->lru, page_list);
+		else
+			page_array[nr_populated] = page;
+		nr_populated++;
 	}
 
 	local_irq_restore(flags);
 
-	return allocated;
+	return nr_populated;
 
 failed_irq:
 	local_irq_restore(flags);
@@ -5075,11 +5100,14 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 failed:
 	page = __alloc_pages(gfp, 0, preferred_nid, nodemask);
 	if (page) {
-		list_add(&page->lru, page_list);
-		allocated = 1;
+		if (page_list)
+			list_add(&page->lru, page_list);
+		else
+			page_array[nr_populated] = page;
+		nr_populated++;
 	}
 
-	return allocated;
+	return nr_populated;
 }
 EXPORT_SYMBOL_GPL(__alloc_pages_bulk);
 
-- 
2.26.2

Re: [PATCH 3/9] mm/page_alloc: Add an array-based interface to the bulk page allocator

[Vlastimil Babka]

On 3/25/21 12:42 PM, Mel Gorman wrote:
> The proposed callers for the bulk allocator store pages from the bulk
> allocator in an array. This patch adds an array-based interface to the API
> to avoid multiple list iterations. The page list interface is preserved
> to avoid requiring all users of the bulk API to allocate and manage enough
> storage to store the pages.
> 
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[PATCH 4/9] mm/page_alloc: optimize code layout for __alloc_pages_bulk

[Mel Gorman]

From: Jesper Dangaard Brouer <brouer@redhat.com>

Looking at perf-report and ASM-code for __alloc_pages_bulk() it is clear
that the code activated is suboptimal. The compiler guesses wrong and
places unlikely code at the beginning. Due to the use of WARN_ON_ONCE()
macro the UD2 asm instruction is added to the code, which confuse the
I-cache prefetcher in the CPU.

[mgorman: Minor changes and rebasing]
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index be1e33a4df39..1ec18121268b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5001,7 +5001,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	unsigned int alloc_flags;
 	int nr_populated = 0;
 
-	if (WARN_ON_ONCE(nr_pages <= 0))
+	if (unlikely(nr_pages <= 0))
 		return 0;
 
 	/*
@@ -5048,7 +5048,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	 * If there are no allowed local zones that meets the watermarks then
 	 * try to allocate a single page and reclaim if necessary.
 	 */
-	if (!zone)
+	if (unlikely(!zone))
 		goto failed;
 
 	/* Attempt the batch allocation */
@@ -5066,7 +5066,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 
 		page = __rmqueue_pcplist(zone, ac.migratetype, alloc_flags,
 								pcp, pcp_list);
-		if (!page) {
+		if (unlikely(!page)) {
 			/* Try and get at least one page */
 			if (!nr_populated)
 				goto failed_irq;
-- 
2.26.2

Re: [PATCH 4/9] mm/page_alloc: optimize code layout for __alloc_pages_bulk

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 11:42:23AM +0000, Mel Gorman wrote:
>  
> -	if (WARN_ON_ONCE(nr_pages <= 0))
> +	if (unlikely(nr_pages <= 0))
>  		return 0;

If we made nr_pages unsigned, we wouldn't need this check at all (ok,
we'd still need to figure out what to do with 0).  But then, if a user
inadvertently passes in -ENOMEM, we'll try to allocate 4 billion pages.
So maybe we should check it.  Gah, API design is hard.

Re: [PATCH 4/9] mm/page_alloc: optimize code layout for __alloc_pages_bulk

[Mel Gorman]

On Thu, Mar 25, 2021 at 12:12:17PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 25, 2021 at 11:42:23AM +0000, Mel Gorman wrote:
> >  
> > -	if (WARN_ON_ONCE(nr_pages <= 0))
> > +	if (unlikely(nr_pages <= 0))
> >  		return 0;
> 
> If we made nr_pages unsigned, we wouldn't need this check at all (ok,
> we'd still need to figure out what to do with 0).  But then, if a user
> inadvertently passes in -ENOMEM, we'll try to allocate 4 billion pages.

This is exactly why nr_pages is signed. An error in accounting by the
caller potentially puts the system under severe memory pressure. This
*should* only be a problem when a new caller of the API is being
implemented. The warning goes away in a later patch for reasons explained
in the changelog.

> So maybe we should check it.  Gah, API design is hard.

Yep.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 4/9] mm/page_alloc: optimize code layout for __alloc_pages_bulk

[Vlastimil Babka]

On 3/25/21 12:42 PM, Mel Gorman wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> Looking at perf-report and ASM-code for __alloc_pages_bulk() it is clear
> that the code activated is suboptimal. The compiler guesses wrong and
> places unlikely code at the beginning. Due to the use of WARN_ON_ONCE()
> macro the UD2 asm instruction is added to the code, which confuse the
> I-cache prefetcher in the CPU.

Hm that's weird, WARN_ON_ONCE() uses unlikely() too, so the UD2 should end up in
the out-of-fast-path part?
But anyway.

> [mgorman: Minor changes and rebasing]
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-By: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index be1e33a4df39..1ec18121268b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5001,7 +5001,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>  	unsigned int alloc_flags;
>  	int nr_populated = 0;
>  
> -	if (WARN_ON_ONCE(nr_pages <= 0))
> +	if (unlikely(nr_pages <= 0))
>  		return 0;
>  
>  	/*
> @@ -5048,7 +5048,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>  	 * If there are no allowed local zones that meets the watermarks then
>  	 * try to allocate a single page and reclaim if necessary.
>  	 */
> -	if (!zone)
> +	if (unlikely(!zone))
>  		goto failed;
>  
>  	/* Attempt the batch allocation */
> @@ -5066,7 +5066,7 @@ int __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>  
>  		page = __rmqueue_pcplist(zone, ac.migratetype, alloc_flags,
>  								pcp, pcp_list);
> -		if (!page) {
> +		if (unlikely(!page)) {
>  			/* Try and get at least one page */
>  			if (!nr_populated)
>  				goto failed_irq;
> 

[PATCH 5/9] mm/page_alloc: inline __rmqueue_pcplist

[Mel Gorman]

From: Jesper Dangaard Brouer <brouer@redhat.com>

When __alloc_pages_bulk() got introduced two callers of __rmqueue_pcplist
exist and the compiler chooses to not inline this function.

 ./scripts/bloat-o-meter vmlinux-before vmlinux-inline__rmqueue_pcplist
add/remove: 0/1 grow/shrink: 2/0 up/down: 164/-125 (39)
Function                                     old     new   delta
rmqueue                                     2197    2296     +99
__alloc_pages_bulk                          1921    1986     +65
__rmqueue_pcplist                            125       -    -125
Total: Before=19374127, After=19374166, chg +0.00%

modprobe page_bench04_bulk loops=$((10**7))

Type:time_bulk_page_alloc_free_array
 -  Per elem: 106 cycles(tsc) 29.595 ns (step:64)
 - (measurement period time:0.295955434 sec time_interval:295955434)
 - (invoke count:10000000 tsc_interval:1065447105)

Before:
 - Per elem: 110 cycles(tsc) 30.633 ns (step:64)

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1ec18121268b..d900e92884b2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3415,7 +3415,8 @@ static inline void zone_statistics(struct zone *preferred_zone, struct zone *z)
 }
 
 /* Remove page from the per-cpu list, caller must protect the list */
-static struct page *__rmqueue_pcplist(struct zone *zone, int migratetype,
+static inline
+struct page *__rmqueue_pcplist(struct zone *zone, int migratetype,
 			unsigned int alloc_flags,
 			struct per_cpu_pages *pcp,
 			struct list_head *list)
-- 
2.26.2

Re: [PATCH 5/9] mm/page_alloc: inline __rmqueue_pcplist

[Vlastimil Babka]

On 3/25/21 12:42 PM, Mel Gorman wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> When __alloc_pages_bulk() got introduced two callers of __rmqueue_pcplist
> exist and the compiler chooses to not inline this function.
> 
>  ./scripts/bloat-o-meter vmlinux-before vmlinux-inline__rmqueue_pcplist
> add/remove: 0/1 grow/shrink: 2/0 up/down: 164/-125 (39)
> Function                                     old     new   delta
> rmqueue                                     2197    2296     +99
> __alloc_pages_bulk                          1921    1986     +65
> __rmqueue_pcplist                            125       -    -125
> Total: Before=19374127, After=19374166, chg +0.00%
> 
> modprobe page_bench04_bulk loops=$((10**7))
> 
> Type:time_bulk_page_alloc_free_array
>  -  Per elem: 106 cycles(tsc) 29.595 ns (step:64)
>  - (measurement period time:0.295955434 sec time_interval:295955434)
>  - (invoke count:10000000 tsc_interval:1065447105)
> 
> Before:
>  - Per elem: 110 cycles(tsc) 30.633 ns (step:64)
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/page_alloc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1ec18121268b..d900e92884b2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3415,7 +3415,8 @@ static inline void zone_statistics(struct zone *preferred_zone, struct zone *z)
>  }
>  
>  /* Remove page from the per-cpu list, caller must protect the list */
> -static struct page *__rmqueue_pcplist(struct zone *zone, int migratetype,
> +static inline
> +struct page *__rmqueue_pcplist(struct zone *zone, int migratetype,
>  			unsigned int alloc_flags,
>  			struct per_cpu_pages *pcp,
>  			struct list_head *list)
> 

[PATCH 6/9] SUNRPC: Set rq_page_end differently

[Mel Gorman]

From: Chuck Lever <chuck.lever@oracle.com>

Patch series "SUNRPC consumer for the bulk page allocator"

This patch set and the measurements below are based on yesterday's
bulk allocator series:

git://git.kernel.org/pub/scm/linux/kernel/git/mel/linux.git mm-bulk-rebase-v5r9

The patches change SUNRPC to invoke the array-based bulk allocator
instead of alloc_page().

The micro-benchmark results are promising. I ran a mixture of 256KB
reads and writes over NFSv3. The server's kernel is built with KASAN
enabled, so the comparison is exaggerated but I believe it is still
valid.

I instrumented svc_recv() to measure the latency of each call to
svc_alloc_arg() and report it via a trace point. The following
results are averages across the trace events.

Single page: 25.007 us per call over 532,571 calls
Bulk list:    6.258 us per call over 517,034 calls
Bulk array:   4.590 us per call over 517,442 calls

This patch (of 2)

Refactor:

I'm about to use the loop variable @i for something else.

As far as the "i++" is concerned, that is a post-increment. The
value of @i is not used subsequently, so the increment operator
is unnecessary and can be removed.

Also note that nfsd_read_actor() was renamed nfsd_splice_actor()
by commit cf8208d0eabd ("sendfile: convert nfsd to
splice_direct_to_actor()").

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 net/sunrpc/svc_xprt.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 3cdd71a8df1e..609bda97d4ae 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -642,7 +642,7 @@ static void svc_check_conn_limits(struct svc_serv *serv)
 static int svc_alloc_arg(struct svc_rqst *rqstp)
 {
 	struct svc_serv *serv = rqstp->rq_server;
-	struct xdr_buf *arg;
+	struct xdr_buf *arg = &rqstp->rq_arg;
 	int pages;
 	int i;
 
@@ -667,11 +667,10 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
 			}
 			rqstp->rq_pages[i] = p;
 		}
-	rqstp->rq_page_end = &rqstp->rq_pages[i];
-	rqstp->rq_pages[i++] = NULL; /* this might be seen in nfs_read_actor */
+	rqstp->rq_page_end = &rqstp->rq_pages[pages];
+	rqstp->rq_pages[pages] = NULL; /* this might be seen in nfsd_splice_actor() */
 
 	/* Make arg->head point to first page and arg->pages point to rest */
-	arg = &rqstp->rq_arg;
 	arg->head[0].iov_base = page_address(rqstp->rq_pages[0]);
 	arg->head[0].iov_len = PAGE_SIZE;
 	arg->pages = rqstp->rq_pages + 1;
-- 
2.26.2

[PATCH 7/9] SUNRPC: Refresh rq_pages using a bulk page allocator

[Mel Gorman]

From: Chuck Lever <chuck.lever@oracle.com>

Reduce the rate at which nfsd threads hammer on the page allocator.
This improves throughput scalability by enabling the threads to run
more independently of each other.

[mgorman: Update interpretation of alloc_pages_bulk return value]
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 net/sunrpc/svc_xprt.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 609bda97d4ae..0c27c3291ca1 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -643,30 +643,29 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
 {
 	struct svc_serv *serv = rqstp->rq_server;
 	struct xdr_buf *arg = &rqstp->rq_arg;
-	int pages;
-	int i;
+	unsigned long pages, filled;
 
-	/* now allocate needed pages.  If we get a failure, sleep briefly */
 	pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
 	if (pages > RPCSVC_MAXPAGES) {
-		pr_warn_once("svc: warning: pages=%u > RPCSVC_MAXPAGES=%lu\n",
+		pr_warn_once("svc: warning: pages=%lu > RPCSVC_MAXPAGES=%lu\n",
 			     pages, RPCSVC_MAXPAGES);
 		/* use as many pages as possible */
 		pages = RPCSVC_MAXPAGES;
 	}
-	for (i = 0; i < pages ; i++)
-		while (rqstp->rq_pages[i] == NULL) {
-			struct page *p = alloc_page(GFP_KERNEL);
-			if (!p) {
-				set_current_state(TASK_INTERRUPTIBLE);
-				if (signalled() || kthread_should_stop()) {
-					set_current_state(TASK_RUNNING);
-					return -EINTR;
-				}
-				schedule_timeout(msecs_to_jiffies(500));
-			}
-			rqstp->rq_pages[i] = p;
+
+	for (;;) {
+		filled = alloc_pages_bulk_array(GFP_KERNEL, pages,
+						rqstp->rq_pages);
+		if (filled == pages)
+			break;
+
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (signalled() || kthread_should_stop()) {
+			set_current_state(TASK_RUNNING);
+			return -EINTR;
 		}
+		schedule_timeout(msecs_to_jiffies(500));
+	}
 	rqstp->rq_page_end = &rqstp->rq_pages[pages];
 	rqstp->rq_pages[pages] = NULL; /* this might be seen in nfsd_splice_actor() */
 
-- 
2.26.2

[PATCH 8/9] net: page_pool: refactor dma_map into own function page_pool_dma_map

[Mel Gorman]

From: Jesper Dangaard Brouer <brouer@redhat.com>

In preparation for next patch, move the dma mapping into its own
function, as this will make it easier to follow the changes.

[ilias.apalodimas: make page_pool_dma_map return boolean]
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 net/core/page_pool.c | 45 +++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index ad8b0707af04..40e1b2beaa6c 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -180,14 +180,37 @@ static void page_pool_dma_sync_for_device(struct page_pool *pool,
 					 pool->p.dma_dir);
 }
 
+static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
+{
+	dma_addr_t dma;
+
+	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
+	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
+	 * into page private data (i.e 32bit cpu with 64bit DMA caps)
+	 * This mapping is kept for lifetime of page, until leaving pool.
+	 */
+	dma = dma_map_page_attrs(pool->p.dev, page, 0,
+				 (PAGE_SIZE << pool->p.order),
+				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
+	if (dma_mapping_error(pool->p.dev, dma))
+		return false;
+
+	page->dma_addr = dma;
+
+	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
+
+	return true;
+}
+
 /* slow path */
 noinline
 static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 						 gfp_t _gfp)
 {
+	unsigned int pp_flags = pool->p.flags;
 	struct page *page;
 	gfp_t gfp = _gfp;
-	dma_addr_t dma;
 
 	/* We could always set __GFP_COMP, and avoid this branch, as
 	 * prep_new_page() can handle order-0 with __GFP_COMP.
@@ -211,30 +234,14 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	if (!page)
 		return NULL;
 
-	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
-		goto skip_dma_map;
-
-	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
-	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
-	 * into page private data (i.e 32bit cpu with 64bit DMA caps)
-	 * This mapping is kept for lifetime of page, until leaving pool.
-	 */
-	dma = dma_map_page_attrs(pool->p.dev, page, 0,
-				 (PAGE_SIZE << pool->p.order),
-				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
-	if (dma_mapping_error(pool->p.dev, dma)) {
+	if ((pp_flags & PP_FLAG_DMA_MAP) &&
+	    unlikely(!page_pool_dma_map(pool, page))) {
 		put_page(page);
 		return NULL;
 	}
-	page->dma_addr = dma;
 
-	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
-		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
-
-skip_dma_map:
 	/* Track how many pages are held 'in-flight' */
 	pool->pages_state_hold_cnt++;
-
 	trace_page_pool_state_hold(pool, page, pool->pages_state_hold_cnt);
 
 	/* When page just alloc'ed is should/must have refcnt 1. */
-- 
2.26.2

[PATCH 9/9] net: page_pool: use alloc_pages_bulk in refill code path

[Mel Gorman]

From: Jesper Dangaard Brouer <brouer@redhat.com>

There are cases where the page_pool need to refill with pages from the
page allocator. Some workloads cause the page_pool to release pages
instead of recycling these pages.

For these workload it can improve performance to bulk alloc pages from
the page-allocator to refill the alloc cache.

For XDP-redirect workload with 100G mlx5 driver (that use page_pool)
redirecting xdp_frame packets into a veth, that does XDP_PASS to create
an SKB from the xdp_frame, which then cannot return the page to the
page_pool.

Performance results under GitHub xdp-project[1]:
 [1] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/page_pool06_alloc_pages_bulk.org

Mel: The patch "net: page_pool: convert to use alloc_pages_bulk_array
variant" was squashed with this patch. From the test page, the array
variant was superior with one of the test results as follows.

	Kernel		XDP stats       CPU     pps           Delta
	Baseline	XDP-RX CPU      total   3,771,046       n/a
	List		XDP-RX CPU      total   3,940,242    +4.49%
	Array		XDP-RX CPU      total   4,249,224   +12.68%

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 include/net/page_pool.h |  2 +-
 net/core/page_pool.c    | 82 ++++++++++++++++++++++++++++-------------
 2 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index b5b195305346..6d517a37c18b 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -65,7 +65,7 @@
 #define PP_ALLOC_CACHE_REFILL	64
 struct pp_alloc_cache {
 	u32 count;
-	void *cache[PP_ALLOC_CACHE_SIZE];
+	struct page *cache[PP_ALLOC_CACHE_SIZE];
 };
 
 struct page_pool_params {
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 40e1b2beaa6c..9ec1aa9640ad 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -203,38 +203,17 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
 	return true;
 }
 
-/* slow path */
-noinline
-static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
-						 gfp_t _gfp)
+static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
+						 gfp_t gfp)
 {
-	unsigned int pp_flags = pool->p.flags;
 	struct page *page;
-	gfp_t gfp = _gfp;
-
-	/* We could always set __GFP_COMP, and avoid this branch, as
-	 * prep_new_page() can handle order-0 with __GFP_COMP.
-	 */
-	if (pool->p.order)
-		gfp |= __GFP_COMP;
-
-	/* FUTURE development:
-	 *
-	 * Current slow-path essentially falls back to single page
-	 * allocations, which doesn't improve performance.  This code
-	 * need bulk allocation support from the page allocator code.
-	 */
 
-	/* Cache was empty, do real allocation */
-#ifdef CONFIG_NUMA
+	gfp |= __GFP_COMP;
 	page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
-#else
-	page = alloc_pages(gfp, pool->p.order);
-#endif
-	if (!page)
+	if (unlikely(!page))
 		return NULL;
 
-	if ((pp_flags & PP_FLAG_DMA_MAP) &&
+	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
 	    unlikely(!page_pool_dma_map(pool, page))) {
 		put_page(page);
 		return NULL;
@@ -243,6 +222,57 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	/* Track how many pages are held 'in-flight' */
 	pool->pages_state_hold_cnt++;
 	trace_page_pool_state_hold(pool, page, pool->pages_state_hold_cnt);
+	return page;
+}
+
+/* slow path */
+noinline
+static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
+						 gfp_t gfp)
+{
+	const int bulk = PP_ALLOC_CACHE_REFILL;
+	unsigned int pp_flags = pool->p.flags;
+	unsigned int pp_order = pool->p.order;
+	struct page *page;
+	int i, nr_pages;
+
+	/* Don't support bulk alloc for high-order pages */
+	if (unlikely(pp_order))
+		return __page_pool_alloc_page_order(pool, gfp);
+
+	/* Unnecessary as alloc cache is empty, but guarantees zero count */
+	if (unlikely(pool->alloc.count > 0))
+		return pool->alloc.cache[--pool->alloc.count];
+
+	/* Mark empty alloc.cache slots "empty" for alloc_pages_bulk_array */
+	memset(&pool->alloc.cache, 0, sizeof(void *) * bulk);
+
+	nr_pages = alloc_pages_bulk_array(gfp, bulk, pool->alloc.cache);
+	if (unlikely(!nr_pages))
+		return NULL;
+
+	/* Pages have been filled into alloc.cache array, but count is zero and
+	 * page element have not been (possibly) DMA mapped.
+	 */
+	for (i = 0; i < nr_pages; i++) {
+		page = pool->alloc.cache[i];
+		if ((pp_flags & PP_FLAG_DMA_MAP) &&
+		    unlikely(!page_pool_dma_map(pool, page))) {
+			put_page(page);
+			continue;
+		}
+		pool->alloc.cache[pool->alloc.count++] = page;
+		/* Track how many pages are held 'in-flight' */
+		pool->pages_state_hold_cnt++;
+		trace_page_pool_state_hold(pool, page,
+					   pool->pages_state_hold_cnt);
+	}
+
+	/* Return last page */
+	if (likely(pool->alloc.count > 0))
+		page = pool->alloc.cache[--pool->alloc.count];
+	else
+		page = NULL;
 
 	/* When page just alloc'ed is should/must have refcnt 1. */
 	return page;
-- 
2.26.2

Re: [PATCH 9/9] net: page_pool: use alloc_pages_bulk in refill code path

[Alexander Lobakin]

From: Mel Gorman <mgorman@techsingularity.net>
Date: Thu, 25 Mar 2021 11:42:28 +0000

> From: Jesper Dangaard Brouer <brouer@redhat.com>
>
> There are cases where the page_pool need to refill with pages from the
> page allocator. Some workloads cause the page_pool to release pages
> instead of recycling these pages.
>
> For these workload it can improve performance to bulk alloc pages from
> the page-allocator to refill the alloc cache.
>
> For XDP-redirect workload with 100G mlx5 driver (that use page_pool)
> redirecting xdp_frame packets into a veth, that does XDP_PASS to create
> an SKB from the xdp_frame, which then cannot return the page to the
> page_pool.
>
> Performance results under GitHub xdp-project[1]:
>  [1] https://github.com/xdp-project/xdp-project/blob/master/areas/mem/page_pool06_alloc_pages_bulk.org
>
> Mel: The patch "net: page_pool: convert to use alloc_pages_bulk_array
> variant" was squashed with this patch. From the test page, the array
> variant was superior with one of the test results as follows.
>
> 	Kernel		XDP stats       CPU     pps           Delta
> 	Baseline	XDP-RX CPU      total   3,771,046       n/a
> 	List		XDP-RX CPU      total   3,940,242    +4.49%
> 	Array		XDP-RX CPU      total   4,249,224   +12.68%
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

I tested it a lot for past two weeks and I'm very satisfied with
the results, especially the new array-based version.
Haven't had a chance to test this particular set yet, but still.

Reviewed-by: Alexander Lobakin <alobakin@pm.me>

Great work, thank you all guys!

> ---
>  include/net/page_pool.h |  2 +-
>  net/core/page_pool.c    | 82 ++++++++++++++++++++++++++++-------------
>  2 files changed, 57 insertions(+), 27 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..6d517a37c18b 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -65,7 +65,7 @@
>  #define PP_ALLOC_CACHE_REFILL	64
>  struct pp_alloc_cache {
>  	u32 count;
> -	void *cache[PP_ALLOC_CACHE_SIZE];
> +	struct page *cache[PP_ALLOC_CACHE_SIZE];
>  };
>
>  struct page_pool_params {
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 40e1b2beaa6c..9ec1aa9640ad 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -203,38 +203,17 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>  	return true;
>  }
>
> -/* slow path */
> -noinline
> -static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
> -						 gfp_t _gfp)
> +static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
> +						 gfp_t gfp)
>  {
> -	unsigned int pp_flags = pool->p.flags;
>  	struct page *page;
> -	gfp_t gfp = _gfp;
> -
> -	/* We could always set __GFP_COMP, and avoid this branch, as
> -	 * prep_new_page() can handle order-0 with __GFP_COMP.
> -	 */
> -	if (pool->p.order)
> -		gfp |= __GFP_COMP;
> -
> -	/* FUTURE development:
> -	 *
> -	 * Current slow-path essentially falls back to single page
> -	 * allocations, which doesn't improve performance.  This code
> -	 * need bulk allocation support from the page allocator code.
> -	 */
>
> -	/* Cache was empty, do real allocation */
> -#ifdef CONFIG_NUMA
> +	gfp |= __GFP_COMP;
>  	page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
> -#else
> -	page = alloc_pages(gfp, pool->p.order);
> -#endif
> -	if (!page)
> +	if (unlikely(!page))
>  		return NULL;
>
> -	if ((pp_flags & PP_FLAG_DMA_MAP) &&
> +	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
>  	    unlikely(!page_pool_dma_map(pool, page))) {
>  		put_page(page);
>  		return NULL;
> @@ -243,6 +222,57 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>  	/* Track how many pages are held 'in-flight' */
>  	pool->pages_state_hold_cnt++;
>  	trace_page_pool_state_hold(pool, page, pool->pages_state_hold_cnt);
> +	return page;
> +}
> +
> +/* slow path */
> +noinline
> +static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
> +						 gfp_t gfp)
> +{
> +	const int bulk = PP_ALLOC_CACHE_REFILL;
> +	unsigned int pp_flags = pool->p.flags;
> +	unsigned int pp_order = pool->p.order;
> +	struct page *page;
> +	int i, nr_pages;
> +
> +	/* Don't support bulk alloc for high-order pages */
> +	if (unlikely(pp_order))
> +		return __page_pool_alloc_page_order(pool, gfp);
> +
> +	/* Unnecessary as alloc cache is empty, but guarantees zero count */
> +	if (unlikely(pool->alloc.count > 0))
> +		return pool->alloc.cache[--pool->alloc.count];
> +
> +	/* Mark empty alloc.cache slots "empty" for alloc_pages_bulk_array */
> +	memset(&pool->alloc.cache, 0, sizeof(void *) * bulk);
> +
> +	nr_pages = alloc_pages_bulk_array(gfp, bulk, pool->alloc.cache);
> +	if (unlikely(!nr_pages))
> +		return NULL;
> +
> +	/* Pages have been filled into alloc.cache array, but count is zero and
> +	 * page element have not been (possibly) DMA mapped.
> +	 */
> +	for (i = 0; i < nr_pages; i++) {
> +		page = pool->alloc.cache[i];
> +		if ((pp_flags & PP_FLAG_DMA_MAP) &&
> +		    unlikely(!page_pool_dma_map(pool, page))) {
> +			put_page(page);
> +			continue;
> +		}
> +		pool->alloc.cache[pool->alloc.count++] = page;
> +		/* Track how many pages are held 'in-flight' */
> +		pool->pages_state_hold_cnt++;
> +		trace_page_pool_state_hold(pool, page,
> +					   pool->pages_state_hold_cnt);
> +	}
> +
> +	/* Return last page */
> +	if (likely(pool->alloc.count > 0))
> +		page = pool->alloc.cache[--pool->alloc.count];
> +	else
> +		page = NULL;
>
>  	/* When page just alloc'ed is should/must have refcnt 1. */
>  	return page;
> --
> 2.26.2

Al

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 11:42:19AM +0000, Mel Gorman wrote:
> This series introduces a bulk order-0 page allocator with sunrpc and
> the network page pool being the first users. The implementation is not
> efficient as semantics needed to be ironed out first. If no other semantic
> changes are needed, it can be made more efficient.  Despite that, this
> is a performance-related for users that require multiple pages for an
> operation without multiple round-trips to the page allocator. Quoting
> the last patch for the high-speed networking use-case
> 
>             Kernel          XDP stats       CPU     pps           Delta
>             Baseline        XDP-RX CPU      total   3,771,046       n/a
>             List            XDP-RX CPU      total   3,940,242    +4.49%
>             Array           XDP-RX CPU      total   4,249,224   +12.68%
> 
> >From the SUNRPC traces of svc_alloc_arg()
> 
> 	Single page: 25.007 us per call over 532,571 calls
> 	Bulk list:    6.258 us per call over 517,034 calls
> 	Bulk array:   4.590 us per call over 517,442 calls
> 
> Both potential users in this series are corner cases (NFS and high-speed
> networks) so it is unlikely that most users will see any benefit in the
> short term. Other potential other users are batch allocations for page
> cache readahead, fault around and SLUB allocations when high-order pages
> are unavailable. It's unknown how much benefit would be seen by converting
> multiple page allocation calls to a single batch or what difference it may
> make to headline performance.

We have a third user, vmalloc(), with a 16% perf improvement.  I know the
email says 21% but that includes the 5% improvement from switching to
kvmalloc() to allocate area->pages.

https://lore.kernel.org/linux-mm/20210323133948.GA10046@pc638.lan/

I don't know how many _frequent_ vmalloc users we have that will benefit
from this, but it's probably more than will benefit from improvements
to 200Gbit networking performance.

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Mel Gorman]

On Thu, Mar 25, 2021 at 12:50:01PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 25, 2021 at 11:42:19AM +0000, Mel Gorman wrote:
> > This series introduces a bulk order-0 page allocator with sunrpc and
> > the network page pool being the first users. The implementation is not
> > efficient as semantics needed to be ironed out first. If no other semantic
> > changes are needed, it can be made more efficient.  Despite that, this
> > is a performance-related for users that require multiple pages for an
> > operation without multiple round-trips to the page allocator. Quoting
> > the last patch for the high-speed networking use-case
> > 
> >             Kernel          XDP stats       CPU     pps           Delta
> >             Baseline        XDP-RX CPU      total   3,771,046       n/a
> >             List            XDP-RX CPU      total   3,940,242    +4.49%
> >             Array           XDP-RX CPU      total   4,249,224   +12.68%
> > 
> > >From the SUNRPC traces of svc_alloc_arg()
> > 
> > 	Single page: 25.007 us per call over 532,571 calls
> > 	Bulk list:    6.258 us per call over 517,034 calls
> > 	Bulk array:   4.590 us per call over 517,442 calls
> > 
> > Both potential users in this series are corner cases (NFS and high-speed
> > networks) so it is unlikely that most users will see any benefit in the
> > short term. Other potential other users are batch allocations for page
> > cache readahead, fault around and SLUB allocations when high-order pages
> > are unavailable. It's unknown how much benefit would be seen by converting
> > multiple page allocation calls to a single batch or what difference it may
> > make to headline performance.
> 
> We have a third user, vmalloc(), with a 16% perf improvement.  I know the
> email says 21% but that includes the 5% improvement from switching to
> kvmalloc() to allocate area->pages.
> 
> https://lore.kernel.org/linux-mm/20210323133948.GA10046@pc638.lan/
> 

That's fairly promising. Assuming the bulk allocator gets merged, it would
make sense to add vmalloc on top. That's for bringing it to my attention
because it's far more relevant than my imaginary potential use cases.

> I don't know how many _frequent_ vmalloc users we have that will benefit
> from this, but it's probably more than will benefit from improvements
> to 200Gbit networking performance.

I think it was 100Gbit being looked at but your point is still valid and
there is no harm in incrementally improving over time.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Uladzislau Rezki]

> On Thu, Mar 25, 2021 at 12:50:01PM +0000, Matthew Wilcox wrote:
> > On Thu, Mar 25, 2021 at 11:42:19AM +0000, Mel Gorman wrote:
> > > This series introduces a bulk order-0 page allocator with sunrpc and
> > > the network page pool being the first users. The implementation is not
> > > efficient as semantics needed to be ironed out first. If no other semantic
> > > changes are needed, it can be made more efficient.  Despite that, this
> > > is a performance-related for users that require multiple pages for an
> > > operation without multiple round-trips to the page allocator. Quoting
> > > the last patch for the high-speed networking use-case
> > > 
> > >             Kernel          XDP stats       CPU     pps           Delta
> > >             Baseline        XDP-RX CPU      total   3,771,046       n/a
> > >             List            XDP-RX CPU      total   3,940,242    +4.49%
> > >             Array           XDP-RX CPU      total   4,249,224   +12.68%
> > > 
> > > >From the SUNRPC traces of svc_alloc_arg()
> > > 
> > > 	Single page: 25.007 us per call over 532,571 calls
> > > 	Bulk list:    6.258 us per call over 517,034 calls
> > > 	Bulk array:   4.590 us per call over 517,442 calls
> > > 
> > > Both potential users in this series are corner cases (NFS and high-speed
> > > networks) so it is unlikely that most users will see any benefit in the
> > > short term. Other potential other users are batch allocations for page
> > > cache readahead, fault around and SLUB allocations when high-order pages
> > > are unavailable. It's unknown how much benefit would be seen by converting
> > > multiple page allocation calls to a single batch or what difference it may
> > > make to headline performance.
> > 
> > We have a third user, vmalloc(), with a 16% perf improvement.  I know the
> > email says 21% but that includes the 5% improvement from switching to
> > kvmalloc() to allocate area->pages.
> > 
> > https://lore.kernel.org/linux-mm/20210323133948.GA10046@pc638.lan/
> > 
> 
> That's fairly promising. Assuming the bulk allocator gets merged, it would
> make sense to add vmalloc on top. That's for bringing it to my attention
> because it's far more relevant than my imaginary potential use cases.
> 
For the vmalloc we should be able to allocating on a specific NUMA node,
at least the current interface takes it into account. As far as i see
the current interface allocate on a current node:

static inline unsigned long
alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
{
    return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
}

Or am i missing something?

--
Vlad Rezki

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 03:06:57PM +0100, Uladzislau Rezki wrote:
> For the vmalloc we should be able to allocating on a specific NUMA node,
> at least the current interface takes it into account. As far as i see
> the current interface allocate on a current node:
> 
> static inline unsigned long
> alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
> {
>     return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
> }
> 
> Or am i missing something?

You can call __alloc_pages_bulk() directly; there's no need to indirect
through alloc_pages_bulk_array().

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Uladzislau Rezki]

On Thu, Mar 25, 2021 at 02:09:27PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 25, 2021 at 03:06:57PM +0100, Uladzislau Rezki wrote:
> > For the vmalloc we should be able to allocating on a specific NUMA node,
> > at least the current interface takes it into account. As far as i see
> > the current interface allocate on a current node:
> > 
> > static inline unsigned long
> > alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
> > {
> >     return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
> > }
> > 
> > Or am i missing something?
> 
> You can call __alloc_pages_bulk() directly; there's no need to indirect
> through alloc_pages_bulk_array().
>
OK. It is accessible then.

--
Vlad Rezki

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Mel Gorman]

On Thu, Mar 25, 2021 at 03:06:57PM +0100, Uladzislau Rezki wrote:
> > On Thu, Mar 25, 2021 at 12:50:01PM +0000, Matthew Wilcox wrote:
> > > On Thu, Mar 25, 2021 at 11:42:19AM +0000, Mel Gorman wrote:
> > > > This series introduces a bulk order-0 page allocator with sunrpc and
> > > > the network page pool being the first users. The implementation is not
> > > > efficient as semantics needed to be ironed out first. If no other semantic
> > > > changes are needed, it can be made more efficient.  Despite that, this
> > > > is a performance-related for users that require multiple pages for an
> > > > operation without multiple round-trips to the page allocator. Quoting
> > > > the last patch for the high-speed networking use-case
> > > > 
> > > >             Kernel          XDP stats       CPU     pps           Delta
> > > >             Baseline        XDP-RX CPU      total   3,771,046       n/a
> > > >             List            XDP-RX CPU      total   3,940,242    +4.49%
> > > >             Array           XDP-RX CPU      total   4,249,224   +12.68%
> > > > 
> > > > >From the SUNRPC traces of svc_alloc_arg()
> > > > 
> > > > 	Single page: 25.007 us per call over 532,571 calls
> > > > 	Bulk list:    6.258 us per call over 517,034 calls
> > > > 	Bulk array:   4.590 us per call over 517,442 calls
> > > > 
> > > > Both potential users in this series are corner cases (NFS and high-speed
> > > > networks) so it is unlikely that most users will see any benefit in the
> > > > short term. Other potential other users are batch allocations for page
> > > > cache readahead, fault around and SLUB allocations when high-order pages
> > > > are unavailable. It's unknown how much benefit would be seen by converting
> > > > multiple page allocation calls to a single batch or what difference it may
> > > > make to headline performance.
> > > 
> > > We have a third user, vmalloc(), with a 16% perf improvement.  I know the
> > > email says 21% but that includes the 5% improvement from switching to
> > > kvmalloc() to allocate area->pages.
> > > 
> > > https://lore.kernel.org/linux-mm/20210323133948.GA10046@pc638.lan/
> > > 
> > 
> > That's fairly promising. Assuming the bulk allocator gets merged, it would
> > make sense to add vmalloc on top. That's for bringing it to my attention
> > because it's far more relevant than my imaginary potential use cases.
> > 
> For the vmalloc we should be able to allocating on a specific NUMA node,
> at least the current interface takes it into account. As far as i see
> the current interface allocate on a current node:
> 
> static inline unsigned long
> alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
> {
>     return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
> }
> 
> Or am i missing something?
> 

No, you're not missing anything. Options would be to add a helper similar
alloc_pages_node or to directly call __alloc_pages_bulk specifying a node
and using GFP_THISNODE. prepare_alloc_pages() should pick the correct
zonelist containing only the required node.

> --
> Vlad Rezki

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 0/9 v6] Introduce a bulk order-0 page allocator with two in-tree users

[Uladzislau Rezki]

On Thu, Mar 25, 2021 at 02:26:24PM +0000, Mel Gorman wrote:
> On Thu, Mar 25, 2021 at 03:06:57PM +0100, Uladzislau Rezki wrote:
> > > On Thu, Mar 25, 2021 at 12:50:01PM +0000, Matthew Wilcox wrote:
> > > > On Thu, Mar 25, 2021 at 11:42:19AM +0000, Mel Gorman wrote:
> > > > > This series introduces a bulk order-0 page allocator with sunrpc and
> > > > > the network page pool being the first users. The implementation is not
> > > > > efficient as semantics needed to be ironed out first. If no other semantic
> > > > > changes are needed, it can be made more efficient.  Despite that, this
> > > > > is a performance-related for users that require multiple pages for an
> > > > > operation without multiple round-trips to the page allocator. Quoting
> > > > > the last patch for the high-speed networking use-case
> > > > > 
> > > > >             Kernel          XDP stats       CPU     pps           Delta
> > > > >             Baseline        XDP-RX CPU      total   3,771,046       n/a
> > > > >             List            XDP-RX CPU      total   3,940,242    +4.49%
> > > > >             Array           XDP-RX CPU      total   4,249,224   +12.68%
> > > > > 
> > > > > >From the SUNRPC traces of svc_alloc_arg()
> > > > > 
> > > > > 	Single page: 25.007 us per call over 532,571 calls
> > > > > 	Bulk list:    6.258 us per call over 517,034 calls
> > > > > 	Bulk array:   4.590 us per call over 517,442 calls
> > > > > 
> > > > > Both potential users in this series are corner cases (NFS and high-speed
> > > > > networks) so it is unlikely that most users will see any benefit in the
> > > > > short term. Other potential other users are batch allocations for page
> > > > > cache readahead, fault around and SLUB allocations when high-order pages
> > > > > are unavailable. It's unknown how much benefit would be seen by converting
> > > > > multiple page allocation calls to a single batch or what difference it may
> > > > > make to headline performance.
> > > > 
> > > > We have a third user, vmalloc(), with a 16% perf improvement.  I know the
> > > > email says 21% but that includes the 5% improvement from switching to
> > > > kvmalloc() to allocate area->pages.
> > > > 
> > > > https://lore.kernel.org/linux-mm/20210323133948.GA10046@pc638.lan/
> > > > 
> > > 
> > > That's fairly promising. Assuming the bulk allocator gets merged, it would
> > > make sense to add vmalloc on top. That's for bringing it to my attention
> > > because it's far more relevant than my imaginary potential use cases.
> > > 
> > For the vmalloc we should be able to allocating on a specific NUMA node,
> > at least the current interface takes it into account. As far as i see
> > the current interface allocate on a current node:
> > 
> > static inline unsigned long
> > alloc_pages_bulk_array(gfp_t gfp, unsigned long nr_pages, struct page **page_array)
> > {
> >     return __alloc_pages_bulk(gfp, numa_mem_id(), NULL, nr_pages, NULL, page_array);
> > }
> > 
> > Or am i missing something?
> > 
> 
> No, you're not missing anything. Options would be to add a helper similar
> alloc_pages_node or to directly call __alloc_pages_bulk specifying a node
> and using GFP_THISNODE. prepare_alloc_pages() should pick the correct
> zonelist containing only the required node.
> 
IMHO, a helper something like *_node() would be reasonable. I see that many
functions in "mm" have its own variants which explicitly add "_node()" prefix
to signal to users that it is a NUMA aware calls.

As for __alloc_pages_bulk(), i got it.

Thanks!

--
Vlad Rezki