Re: [PATCH 0/3] mm: treewide: make get_free_pages() and return void *

[Mike Rapoport <rppt@kernel.org>]

On Mon, Oct 20, 2025 at 10:54:27AM +0200, Vlastimil Babka wrote:
> On 10/19/25 16:25, Mike Rapoport wrote:
> > On Sun, Oct 19, 2025 at 01:30:47AM +0100, Matthew Wilcox wrote:
> >> On Sat, Oct 18, 2025 at 12:29:59PM +0300, Mike Rapoport wrote:
> >> > Vast majority of allocations that use get_free_pages() and its derivatives
> >> > cast the returned unsigned long to a pointer and then cast it back to
> >> > unsigned long when freeing the memory.
> >> > 
> >> > These castings are useless and only obfuscate the code.
> >> > 
> >> > Make get_free_pages() and friends return 'void *' and free_pages() accept
> >> > 'void *' as its address parameter.
> >> 
> >> No.  Linus has rejected this change before.  I can't find it now, it was
> >> a long time ago. 
> 
> Here's a lore link
> https://lore.kernel.org/all/CA+55aFwp4iy4rtX2gE2WjBGFL=NxMVnoFeHqYa2j1dYOMMGqxg@mail.gmail.com/ 
> > If it was a long time ago, he might not object it now.
> 
> Did the circumstances change in a positive way? Using a semantic patch might
> make it less painfull to apply in a flag day manner, although depends on how
> much is that "a bit of manual tweaking" you mention.

Semantic patch missed a handful of places, other than that tweaking was for
formatting, e.g

diff --git spatch/arch/s390/mm/cmm.c manual/arch/s390/mm/cmm.c
index 980d2b302937..7212ab4f0eaa 100644
--- spatch/arch/s390/mm/cmm.c
+++ manual/arch/s390/mm/cmm.c
@@ -74,7 +74,8 @@ static long cmm_alloc_pages(long nr, long *counter,
 		if (!pa || pa->index >= CMM_NR_PAGES) {
 			/* Need a new page for the page list. */
 			spin_unlock(&cmm_lock);
-			npa =__get_free_page(GFP_NOIO);
+			npa =
+				__get_free_page(GFP_NOIO);
 			if (!npa) {
 				free_page(addr);
 				break;
 
> >> Most of them shouldn't be using get_free_pages() at all, they should be
> >> using kmalloc().
> 
> Changing to kmalloc() would have to be careful, what if the callers rely on
> doing e.g. get_page() later. It would however be useful to dintinguish "I
> want a page-sized buffer" (note that it's guaranteed to be aligned by
> kmalloc() these days, which it wasn't in 2015) from "I really want a page".
> But many of the latter cases maybe want a struct page then and are using
> alloc_pages()? 

alloc_pages() users also not necessarily want a page, there are quite a few
places where we have 

	struct page *page = alloc_pages();
	some_type *ptr = page_address(page);

So ideally those also should use an API that returns void *. But again, as
converting get_free_pages to kmalloc, it's a case-by-case audit.

> > Don't know if most but some of them could. Still, we'd have a bunch of
> > get_free_pages() users with needless castings.
> > And converting callers that should use kmalloc() is a long and tedious
> > process, while here we get an API improvement in a single automated change.

> Maybe a more feasible way would be to rename to something more coherent,
> while keeping the old interfaces alive for a while for easier backporting.
> because __get_free_pages() / free_pages() is not really great naming.
> If possible it would be nice to also make __GFP_COMP implicit in the new API.

If we shorten "page-sized-buffer" to "p" we can do something like:

void *__palloc(gfp_t flags, unsigned int order);
void *palloc(gfp_t flags);
void *pzalloc(gfp_t flags);
void __pfree(void *ptr, unsigned int order);
void pfree(void *ptr);

I'd keep the order in __whatever_free() for the first step, because I'm not
100% sure we can use __GFP_COMP for every existing caller of
get_free_pages.
 
Do we also want to rename gfp flags to something page-sized-buffer based? :)

-- 
Sincerely yours,
Mike.