| Message ID | 162490397938.1485.7782934829743772831.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/page_alloc: Return nr_populated when the array is full | expand |
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?
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
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.
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);
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(-)