linux-nfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm/page_alloc: Return nr_populated when the array is full
@ 2021-06-28 18:12 Chuck Lever
  2021-06-28 18:17 ` Matthew Wilcox
  2021-06-29  8:14 ` Mel Gorman
  0 siblings, 2 replies; 4+ messages in thread
From: Chuck Lever @ 2021-06-28 18:12 UTC (permalink / raw)
  To: mgorman; +Cc: linux-nfs, linux-mm

The SUNRPC consumer of __alloc_bulk_pages() legitimately calls it
with a full array sometimes. In that case, the correct return code,
according to the API contract, is to return the number of pages
already in the array/list.

Let's clean up the return logic to make it clear that the returned
value is always the total number of pages in the array/list, not the
number of pages that were allocated during this call.

Fixes: b3b64ebd3822 ("mm/page_alloc: do bulk array bounds check after checking populated elements")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 mm/page_alloc.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ef2265f86b91..270719898b47 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5047,7 +5047,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	int nr_populated = 0;
 
 	if (unlikely(nr_pages <= 0))
-		return 0;
+		goto out;
 
 	/*
 	 * Skip populated array elements to determine if any pages need
@@ -5058,7 +5058,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 
 	/* Already populated array? */
 	if (unlikely(page_array && nr_pages - nr_populated == 0))
-		return 0;
+		goto out;
 
 	/* Use the single page allocator for one page. */
 	if (nr_pages - nr_populated == 1)
@@ -5068,7 +5068,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 	gfp &= gfp_allowed_mask;
 	alloc_gfp = gfp;
 	if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, &ac, &alloc_gfp, &alloc_flags))
-		return 0;
+		goto out;
 	gfp = alloc_gfp;
 
 	/* Find an allowed local zone that meets the low watermark. */
@@ -5141,6 +5141,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 
 	local_irq_restore(flags);
 
+out:
 	return nr_populated;
 
 failed_irq:
@@ -5156,7 +5157,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 		nr_populated++;
 	}
 
-	return nr_populated;
+	goto out;
 }
 EXPORT_SYMBOL_GPL(__alloc_pages_bulk);
 



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/page_alloc: Return nr_populated when the array is full
  2021-06-28 18:12 [PATCH v2] mm/page_alloc: Return nr_populated when the array is full Chuck Lever
@ 2021-06-28 18:17 ` Matthew Wilcox
  2021-06-28 20:06   ` Chuck Lever III
  2021-06-29  8:14 ` Mel Gorman
  1 sibling, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2021-06-28 18:17 UTC (permalink / raw)
  To: Chuck Lever; +Cc: mgorman, linux-nfs, linux-mm

On Mon, Jun 28, 2021 at 02:12:59PM -0400, Chuck Lever wrote:
> The SUNRPC consumer of __alloc_bulk_pages() legitimately calls it
> with a full array sometimes. In that case, the correct return code,
> according to the API contract, is to return the number of pages
> already in the array/list.
> 
> Let's clean up the return logic to make it clear that the returned
> value is always the total number of pages in the array/list, not the
> number of pages that were allocated during this call.

This is more complicated than either v1 or the version that Mel sent
earlier today.  Is it worth it?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/page_alloc: Return nr_populated when the array is full
  2021-06-28 18:17 ` Matthew Wilcox
@ 2021-06-28 20:06   ` Chuck Lever III
  0 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever III @ 2021-06-28 20:06 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Mel Gorman, Linux NFS Mailing List, linux-mm

Hi-

> On Jun 28, 2021, at 2:17 PM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Mon, Jun 28, 2021 at 02:12:59PM -0400, Chuck Lever wrote:
>> The SUNRPC consumer of __alloc_bulk_pages() legitimately calls it
>> with a full array sometimes. In that case, the correct return code,
>> according to the API contract, is to return the number of pages
>> already in the array/list.
>> 
>> Let's clean up the return logic to make it clear that the returned
>> value is always the total number of pages in the array/list, not the
>> number of pages that were allocated during this call.
> 
> This is more complicated than either v1 or the version that Mel sent
> earlier today.  Is it worth it?

Yes.

My v2 addresses the reason the bug was introduced in the first place: The
code currently does not reflect the API contract described in the documenting
comment.

A human reading the function as it is currently written might easily expect
that a zero return code is proper if something failed. However, the API
contract does not list zero as a unique return value with a special meaning.
The contract merely states: "Returns the number of pages on the list or
array." Therefore zero is a plausible return value only if @nr_pages is
zero or less.

Note that the value returned if prepare_alloc_pages() fails is also incorrect,
by my reading, and my v2 addresses that.

The only other call site is __page_pool_alloc_pages_slow(), and that looks
incorrect to me -- it does not agree with either the API contract or the
SUNRPC call site. I did not fix that, but I think it should be looked into
by someone familiar with that code.

I haven't seen the patch Mel sent earlier today. I was not cc'd on that one
or on b3b64ebd3822.

--
Chuck Lever




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/page_alloc: Return nr_populated when the array is full
  2021-06-28 18:12 [PATCH v2] mm/page_alloc: Return nr_populated when the array is full Chuck Lever
  2021-06-28 18:17 ` Matthew Wilcox
@ 2021-06-29  8:14 ` Mel Gorman
  1 sibling, 0 replies; 4+ messages in thread
From: Mel Gorman @ 2021-06-29  8:14 UTC (permalink / raw)
  To: Chuck Lever; +Cc: linux-nfs, linux-mm, Matthew Wilcox

On Mon, Jun 28, 2021 at 02:12:59PM -0400, Chuck Lever wrote:
> The SUNRPC consumer of __alloc_bulk_pages() legitimately calls it
> with a full array sometimes. In that case, the correct return code,
> according to the API contract, is to return the number of pages
> already in the array/list.
> 
> Let's clean up the return logic to make it clear that the returned
> value is always the total number of pages in the array/list, not the
> number of pages that were allocated during this call.
> 
> Fixes: b3b64ebd3822 ("mm/page_alloc: do bulk array bounds check after checking populated elements")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Commit 66d9282523b3 ("mm/page_alloc: Correct return value of populated
elements if bulk array is populated") has since been merged as it was
the minimal obvious for the problem introduced but I have no objection
to your patch being rebased on top and sent as a cleanup.

Thanks.

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-06-29  8:24 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28 18:12 [PATCH v2] mm/page_alloc: Return nr_populated when the array is full Chuck Lever
2021-06-28 18:17 ` Matthew Wilcox
2021-06-28 20:06   ` Chuck Lever III
2021-06-29  8:14 ` Mel Gorman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).