[PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[JianKang Chen]

From: Jiankang Chen <chenjiankang1@huawei.com>

__get_free_pages will return an virtual address, 
but it is not just 32-bit address, for example a 64-bit system. 
And this comment really confuse new bigenner of mm.

reported-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Jiankang Chen <chenjiankang1@huawei.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 77e4d3c..5a7c432 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4240,7 +4240,7 @@ unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 	struct page *page;
 
 	/*
-	 * __get_free_pages() returns a 32-bit address, which cannot represent
+	 * __get_free_pages() returns a virtual address, which cannot represent
 	 * a highmem page
 	 */
 	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-- 
1.7.12.4

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Mon 27-11-17 19:09:24, JianKang Chen wrote:
> From: Jiankang Chen <chenjiankang1@huawei.com>
> 
> __get_free_pages will return an virtual address, 
> but it is not just 32-bit address, for example a 64-bit system. 
> And this comment really confuse new bigenner of mm.

s@bigenner@beginner@

Anyway, do we really need a bug on for this? Has this actually caught
any wrong usage? VM_BUG_ON tends to be enabled these days AFAIK and
panicking the kernel seems like an over-reaction. If there is a real
risk then why don't we simply mask __GFP_HIGHMEM off when calling
alloc_pages?

> reported-by: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: Jiankang Chen <chenjiankang1@huawei.com>
> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 77e4d3c..5a7c432 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4240,7 +4240,7 @@ unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
>  	struct page *page;
>  
>  	/*
> -	 * __get_free_pages() returns a 32-bit address, which cannot represent
> +	 * __get_free_pages() returns a virtual address, which cannot represent
>  	 * a highmem page
>  	 */
>  	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
> -- 
> 1.7.12.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Mon 27-11-17 12:33:41, Michal Hocko wrote:
> On Mon 27-11-17 19:09:24, JianKang Chen wrote:
> > From: Jiankang Chen <chenjiankang1@huawei.com>
> > 
> > __get_free_pages will return an virtual address, 
> > but it is not just 32-bit address, for example a 64-bit system. 
> > And this comment really confuse new bigenner of mm.
> 
> s@bigenner@beginner@
> 
> Anyway, do we really need a bug on for this? Has this actually caught
> any wrong usage? VM_BUG_ON tends to be enabled these days AFAIK and
> panicking the kernel seems like an over-reaction. If there is a real
> risk then why don't we simply mask __GFP_HIGHMEM off when calling
> alloc_pages?

I meant this
---
>From 000bb422fe07adbfa8cd8ed953b18f48647a45d6 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 29 Nov 2017 17:02:33 +0100
Subject: [PATCH] mm: drop VM_BUG_ON from __get_free_pages

There is no real reason to blow up just because the caller doesn't know
that __get_free_pages cannot return highmem pages. Simply fix that up
silently. Even if we have some confused users such a fixup will not be
harmful.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0d518e9b2ee8..3dd960ea8c13 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4284,9 +4284,7 @@ unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 	 * __get_free_pages() returns a virtual address, which cannot represent
 	 * a highmem page
 	 */
-	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-
-	page = alloc_pages(gfp_mask, order);
+	page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order);
 	if (!page)
 		return 0;
 	return (unsigned long) page_address(page);
-- 
2.15.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Andrew Morton]

On Wed, 29 Nov 2017 17:04:46 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> On Mon 27-11-17 12:33:41, Michal Hocko wrote:
> > On Mon 27-11-17 19:09:24, JianKang Chen wrote:
> > > From: Jiankang Chen <chenjiankang1@huawei.com>
> > > 
> > > __get_free_pages will return an virtual address, 
> > > but it is not just 32-bit address, for example a 64-bit system. 
> > > And this comment really confuse new bigenner of mm.
> > 
> > s@bigenner@beginner@
> > 
> > Anyway, do we really need a bug on for this? Has this actually caught
> > any wrong usage? VM_BUG_ON tends to be enabled these days AFAIK and
> > panicking the kernel seems like an over-reaction. If there is a real
> > risk then why don't we simply mask __GFP_HIGHMEM off when calling
> > alloc_pages?
> 
> I meant this
> ---
> >From 000bb422fe07adbfa8cd8ed953b18f48647a45d6 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Wed, 29 Nov 2017 17:02:33 +0100
> Subject: [PATCH] mm: drop VM_BUG_ON from __get_free_pages
> 
> There is no real reason to blow up just because the caller doesn't know
> that __get_free_pages cannot return highmem pages. Simply fix that up
> silently. Even if we have some confused users such a fixup will not be
> harmful.

mm...  So we have a caller which hopes to be getting highmem pages but
isn't.  Caller then proceeds to pointlessly kmap the page and wonders
why it isn't getting as much memory as it would like on 32-bit systems,
etc.

I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
would suffice.

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Wed 29-11-17 13:41:59, Andrew Morton wrote:
> On Wed, 29 Nov 2017 17:04:46 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Mon 27-11-17 12:33:41, Michal Hocko wrote:
> > > On Mon 27-11-17 19:09:24, JianKang Chen wrote:
> > > > From: Jiankang Chen <chenjiankang1@huawei.com>
> > > > 
> > > > __get_free_pages will return an virtual address, 
> > > > but it is not just 32-bit address, for example a 64-bit system. 
> > > > And this comment really confuse new bigenner of mm.
> > > 
> > > s@bigenner@beginner@
> > > 
> > > Anyway, do we really need a bug on for this? Has this actually caught
> > > any wrong usage? VM_BUG_ON tends to be enabled these days AFAIK and
> > > panicking the kernel seems like an over-reaction. If there is a real
> > > risk then why don't we simply mask __GFP_HIGHMEM off when calling
> > > alloc_pages?
> > 
> > I meant this
> > ---
> > >From 000bb422fe07adbfa8cd8ed953b18f48647a45d6 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Wed, 29 Nov 2017 17:02:33 +0100
> > Subject: [PATCH] mm: drop VM_BUG_ON from __get_free_pages
> > 
> > There is no real reason to blow up just because the caller doesn't know
> > that __get_free_pages cannot return highmem pages. Simply fix that up
> > silently. Even if we have some confused users such a fixup will not be
> > harmful.
> 
> mm...  So we have a caller which hopes to be getting highmem pages but
> isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> why it isn't getting as much memory as it would like on 32-bit systems,
> etc.

How he can kmap the page when he gets a _virtual_ address?

> I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> would suffice.

This function has always been about lowmem pages. I seriously doubt we
have anybody confused and asking for a highmem page in the kernel. I
haven't checked that but it would already blow up as VM_BUG_ON tends to
be enabled on many setups.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Andrew Morton]

On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> > mm...  So we have a caller which hopes to be getting highmem pages but
> > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > why it isn't getting as much memory as it would like on 32-bit systems,
> > etc.
> 
> How he can kmap the page when he gets a _virtual_ address?

doh.

> > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > would suffice.
> 
> This function has always been about lowmem pages. I seriously doubt we
> have anybody confused and asking for a highmem page in the kernel. I
> haven't checked that but it would already blow up as VM_BUG_ON tends to
> be enabled on many setups.

OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
shouldn't be doing that in the first place.

I wonder what happens if we just remove the WARN_ON and pass any
__GFP_HIGHMEM straight through.  The caller gets a weird address from
page_to_virt(highmem page) and usually goes splat?  Good enough
treatment for something which never happens anyway?

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Thu 30-11-17 13:17:06, Andrew Morton wrote:
> On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > > mm...  So we have a caller which hopes to be getting highmem pages but
> > > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > > why it isn't getting as much memory as it would like on 32-bit systems,
> > > etc.
> > 
> > How he can kmap the page when he gets a _virtual_ address?
> 
> doh.
> 
> > > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > > would suffice.
> > 
> > This function has always been about lowmem pages. I seriously doubt we
> > have anybody confused and asking for a highmem page in the kernel. I
> > haven't checked that but it would already blow up as VM_BUG_ON tends to
> > be enabled on many setups.
> 
> OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
> shouldn't be doing that in the first place.

Yes, they shouldn't be.

> I wonder what happens if we just remove the WARN_ON and pass any
> __GFP_HIGHMEM straight through.  The caller gets a weird address from
> page_to_virt(highmem page) and usually goes splat?  Good enough
> treatment for something which never happens anyway?

page_address will return NULL so they will blow up and leak the freshly
allocated memory. I do not think this is really desirable. We _could_
handle this case but I am not really sure this is a win. A silent fixup
sounds like the most simply protection.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Fri 01-12-17 08:24:14, Michal Hocko wrote:
> On Thu 30-11-17 13:17:06, Andrew Morton wrote:
> > On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> > 
> > > > mm...  So we have a caller which hopes to be getting highmem pages but
> > > > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > > > why it isn't getting as much memory as it would like on 32-bit systems,
> > > > etc.
> > > 
> > > How he can kmap the page when he gets a _virtual_ address?
> > 
> > doh.
> > 
> > > > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > > > would suffice.
> > > 
> > > This function has always been about lowmem pages. I seriously doubt we
> > > have anybody confused and asking for a highmem page in the kernel. I
> > > haven't checked that but it would already blow up as VM_BUG_ON tends to
> > > be enabled on many setups.
> > 
> > OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
> > shouldn't be doing that in the first place.
> 
> Yes, they shouldn't be.
> 
> > I wonder what happens if we just remove the WARN_ON and pass any
> > __GFP_HIGHMEM straight through.  The caller gets a weird address from
> > page_to_virt(highmem page) and usually goes splat?  Good enough
> > treatment for something which never happens anyway?
> 
> page_address will return NULL so they will blow up and leak the freshly
> allocated memory.

let me be more specific. They will blow up and leak if the returned
address is not checked. If it is then we just leak. None of that sounds
good to me.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Fri 01-12-17 12:18:45, Michal Hocko wrote:
> On Fri 01-12-17 08:24:14, Michal Hocko wrote:
> > On Thu 30-11-17 13:17:06, Andrew Morton wrote:
> > > On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> > > 
> > > > > mm...  So we have a caller which hopes to be getting highmem pages but
> > > > > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > > > > why it isn't getting as much memory as it would like on 32-bit systems,
> > > > > etc.
> > > > 
> > > > How he can kmap the page when he gets a _virtual_ address?
> > > 
> > > doh.
> > > 
> > > > > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > > > > would suffice.
> > > > 
> > > > This function has always been about lowmem pages. I seriously doubt we
> > > > have anybody confused and asking for a highmem page in the kernel. I
> > > > haven't checked that but it would already blow up as VM_BUG_ON tends to
> > > > be enabled on many setups.
> > > 
> > > OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
> > > shouldn't be doing that in the first place.
> > 
> > Yes, they shouldn't be.
> > 
> > > I wonder what happens if we just remove the WARN_ON and pass any
> > > __GFP_HIGHMEM straight through.  The caller gets a weird address from
> > > page_to_virt(highmem page) and usually goes splat?  Good enough
> > > treatment for something which never happens anyway?
> > 
> > page_address will return NULL so they will blow up and leak the freshly
> > allocated memory.
> 
> let me be more specific. They will blow up and leak if the returned
> address is not checked. If it is then we just leak. None of that sounds
> good to me.

So do we care and I will resend the patch in that case or I just drop
this from my patch queue?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Andrew Morton]

On Thu, 14 Dec 2017 15:06:08 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> On Fri 01-12-17 12:18:45, Michal Hocko wrote:
> > On Fri 01-12-17 08:24:14, Michal Hocko wrote:
> > > On Thu 30-11-17 13:17:06, Andrew Morton wrote:
> > > > On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> > > > 
> > > > > > mm...  So we have a caller which hopes to be getting highmem pages but
> > > > > > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > > > > > why it isn't getting as much memory as it would like on 32-bit systems,
> > > > > > etc.
> > > > > 
> > > > > How he can kmap the page when he gets a _virtual_ address?
> > > > 
> > > > doh.
> > > > 
> > > > > > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > > > > > would suffice.
> > > > > 
> > > > > This function has always been about lowmem pages. I seriously doubt we
> > > > > have anybody confused and asking for a highmem page in the kernel. I
> > > > > haven't checked that but it would already blow up as VM_BUG_ON tends to
> > > > > be enabled on many setups.
> > > > 
> > > > OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
> > > > shouldn't be doing that in the first place.
> > > 
> > > Yes, they shouldn't be.
> > > 
> > > > I wonder what happens if we just remove the WARN_ON and pass any
> > > > __GFP_HIGHMEM straight through.  The caller gets a weird address from
> > > > page_to_virt(highmem page) and usually goes splat?  Good enough
> > > > treatment for something which never happens anyway?
> > > 
> > > page_address will return NULL so they will blow up and leak the freshly
> > > allocated memory.
> > 
> > let me be more specific. They will blow up and leak if the returned
> > address is not checked. If it is then we just leak. None of that sounds
> > good to me.
> 
> So do we care and I will resend the patch in that case or I just drop
> this from my patch queue?

Well..  I still think that silently accepting bad input would be bad
practice.  If we can just delete the assertion and have such a caller
reliably blow up later on then that's good enough.  Otherwise let's
leave the code as-is?

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Thu 14-12-17 12:33:09, Andrew Morton wrote:
> On Thu, 14 Dec 2017 15:06:08 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Fri 01-12-17 12:18:45, Michal Hocko wrote:
> > > On Fri 01-12-17 08:24:14, Michal Hocko wrote:
> > > > On Thu 30-11-17 13:17:06, Andrew Morton wrote:
> > > > > On Thu, 30 Nov 2017 07:53:35 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> > > > > 
> > > > > > > mm...  So we have a caller which hopes to be getting highmem pages but
> > > > > > > isn't.  Caller then proceeds to pointlessly kmap the page and wonders
> > > > > > > why it isn't getting as much memory as it would like on 32-bit systems,
> > > > > > > etc.
> > > > > > 
> > > > > > How he can kmap the page when he gets a _virtual_ address?
> > > > > 
> > > > > doh.
> > > > > 
> > > > > > > I do think we should help ferret out such bogosity.  A WARN_ON_ONCE
> > > > > > > would suffice.
> > > > > > 
> > > > > > This function has always been about lowmem pages. I seriously doubt we
> > > > > > have anybody confused and asking for a highmem page in the kernel. I
> > > > > > haven't checked that but it would already blow up as VM_BUG_ON tends to
> > > > > > be enabled on many setups.
> > > > > 
> > > > > OK.  But silently accepting __GFP_HIGHMEM is a bit weird - callers
> > > > > shouldn't be doing that in the first place.
> > > > 
> > > > Yes, they shouldn't be.
> > > > 
> > > > > I wonder what happens if we just remove the WARN_ON and pass any
> > > > > __GFP_HIGHMEM straight through.  The caller gets a weird address from
> > > > > page_to_virt(highmem page) and usually goes splat?  Good enough
> > > > > treatment for something which never happens anyway?
> > > > 
> > > > page_address will return NULL so they will blow up and leak the freshly
> > > > allocated memory.
> > > 
> > > let me be more specific. They will blow up and leak if the returned
> > > address is not checked. If it is then we just leak. None of that sounds
> > > good to me.
> > 
> > So do we care and I will resend the patch in that case or I just drop
> > this from my patch queue?
> 
> Well..  I still think that silently accepting bad input would be bad
> practice.  If we can just delete the assertion and have such a caller
> reliably blow up later on then that's good enough.

The point is that if the caller checks for the failed allocation then
the result is a memory leak.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Andrew Morton]

On Fri, 15 Dec 2017 10:36:18 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> > > 
> > > So do we care and I will resend the patch in that case or I just drop
> > > this from my patch queue?
> > 
> > Well..  I still think that silently accepting bad input would be bad
> > practice.  If we can just delete the assertion and have such a caller
> > reliably blow up later on then that's good enough.
> 
> The point is that if the caller checks for the failed allocation then
> the result is a memory leak.

That's if page_address(highmem page) returns NULL.  I'm not sure what
it returns, really - so many different implementations across so many
different architectures.

Oh well, it would have been nice to remove that VM_BUG_ON().  Why not
just leave the code as it is now?  

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Fri 15-12-17 12:57:35, Andrew Morton wrote:
> On Fri, 15 Dec 2017 10:36:18 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > > > 
> > > > So do we care and I will resend the patch in that case or I just drop
> > > > this from my patch queue?
> > > 
> > > Well..  I still think that silently accepting bad input would be bad
> > > practice.  If we can just delete the assertion and have such a caller
> > > reliably blow up later on then that's good enough.
> > 
> > The point is that if the caller checks for the failed allocation then
> > the result is a memory leak.
> 
> That's if page_address(highmem page) returns NULL.  I'm not sure what
> it returns, really - so many different implementations across so many
> different architectures.

I am not sure I follow. We only do care for HIGHMEM, right? And that one
returns NULL unless the high mem page is not kmaped.

> Oh well, it would have been nice to remove that VM_BUG_ON().  Why not
> just leave the code as it is now?  

BUGing on a bogus usage is not popular anymore. Also checking for
something nobody actually does is a bit pointless. I will not insist
though.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Michal Hocko]

On Wed 29-11-17 17:04:46, Michal Hocko wrote:
[...]
> From 000bb422fe07adbfa8cd8ed953b18f48647a45d6 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Wed, 29 Nov 2017 17:02:33 +0100
> Subject: [PATCH] mm: drop VM_BUG_ON from __get_free_pages
> 
> There is no real reason to blow up just because the caller doesn't know
> that __get_free_pages cannot return highmem pages. Simply fix that up
> silently. Even if we have some confused users such a fixup will not be
> harmful.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Andrew, have we reached any conclusion for this? Should I repost or drop
it on the floor?

> ---
>  mm/page_alloc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0d518e9b2ee8..3dd960ea8c13 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4284,9 +4284,7 @@ unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
>  	 * __get_free_pages() returns a virtual address, which cannot represent
>  	 * a highmem page
>  	 */
> -	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
> -
> -	page = alloc_pages(gfp_mask, order);
> +	page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order);
>  	if (!page)
>  		return 0;
>  	return (unsigned long) page_address(page);
> -- 
> 2.15.0
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Andrew Morton]

On Fri, 6 Apr 2018 12:02:36 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> On Wed 29-11-17 17:04:46, Michal Hocko wrote:
> [...]
> > From 000bb422fe07adbfa8cd8ed953b18f48647a45d6 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Wed, 29 Nov 2017 17:02:33 +0100
> > Subject: [PATCH] mm: drop VM_BUG_ON from __get_free_pages
> > 
> > There is no real reason to blow up just because the caller doesn't know
> > that __get_free_pages cannot return highmem pages. Simply fix that up
> > silently. Even if we have some confused users such a fixup will not be
> > harmful.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Andrew, have we reached any conclusion for this? Should I repost or drop
> it on the floor?

I actually thought we'd settled on something and merged it.  hrm.

Please send a fresh patch sometime?

Re: [PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[Anshuman Khandual]

On 11/25/2017 12:50 PM, JianKang Chen wrote:
> From: Jiankang Chen <chenjiankang1@huawei.com>
> 
> __get_free_pages will return an 64bit address in 64bit System
> like arm64 or x86_64. And this comment really confuse new bigenner of
> mm.

Normally its not 64 bit virtual address though CPU architecture supports
64 bits. But yes, specifying it as general virtual address without number
of bits is better.

[PATCH resend] mm/page_alloc: fix comment is __get_free_pages

[JianKang Chen]

From: Jiankang Chen <chenjiankang1@huawei.com>

__get_free_pages will return an 64bit address in 64bit System
like arm64 or x86_64. And this comment really confuse new bigenner of
mm.

Signed-off-by: Jiankang Chen <chenjiankang1@huawei.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 77e4d3c..5a7c432 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4240,7 +4240,7 @@ unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 	struct page *page;
 
 	/*
-	 * __get_free_pages() returns a 32-bit address, which cannot represent
+	 * __get_free_pages() returns a virtual address, which cannot represent
 	 * a highmem page
 	 */
 	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-- 
1.7.12.4