[PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Huang Pei]

+. According to Documentation/vm/split_page_table_lock, handle failure
of pgtable_pmd_page_ctor

+. use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT

Reported-by: Joshua Kinard <kumba@gentoo.org>
Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 arch/mips/include/asm/pgalloc.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
index d0cf997b4ba8..5c9597a6c60c 100644
--- a/arch/mips/include/asm/pgalloc.h
+++ b/arch/mips/include/asm/pgalloc.h
@@ -62,11 +62,15 @@ static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
 	pmd_t *pmd = NULL;
 	struct page *pg;
 
-	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
+	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
 	if (pg) {
-		pgtable_pmd_page_ctor(pg);
-		pmd = (pmd_t *)page_address(pg);
-		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
+		if(pgtable_pmd_page_ctor(pg)) {
+			pmd = (pmd_t *)page_address(pg);
+			pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
+		} else {
+			__free_pages(pg, PMD_ORDER);
+		}
+
 	}
 	return pmd;
 }
-- 
2.25.1

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Joshua Kinard]

On 7/20/2021 23:43, Huang Pei wrote:
> +. According to Documentation/vm/split_page_table_lock, handle failure
> of pgtable_pmd_page_ctor
> 
> +. use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
> 
> Reported-by: Joshua Kinard <kumba@gentoo.org>
> Signed-off-by: Huang Pei <huangpei@loongson.cn>
> ---
>  arch/mips/include/asm/pgalloc.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
> index d0cf997b4ba8..5c9597a6c60c 100644
> --- a/arch/mips/include/asm/pgalloc.h
> +++ b/arch/mips/include/asm/pgalloc.h
> @@ -62,11 +62,15 @@ static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
>  	pmd_t *pmd = NULL;
>  	struct page *pg;
>  
> -	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
> +	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
>  	if (pg) {
> -		pgtable_pmd_page_ctor(pg);
> -		pmd = (pmd_t *)page_address(pg);
> -		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> +		if(pgtable_pmd_page_ctor(pg)) {
> +			pmd = (pmd_t *)page_address(pg);
> +			pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> +		} else {
> +			__free_pages(pg, PMD_ORDER);
> +		}
> +
>  	}
>  	return pmd;
>  }
> 
Instead of the nested if statements, why not go with something that looks more
like what is in arch/x86/include/asm/pgalloc.h?

Note, I don't have a full kernel tree in front of me at the moment, so this is
just the refactored function of pmd_alloc_one:

static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
{
	pmd_t *pmd;
	struct page *page;

	page = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
	if (!page)
		return NULL;

	if (!pgtable_pmd_page_ctor(page)) {
		__free_pages(page, PMD_ORDER);
		return NULL;
	}

	pmd = (pmd_t *)page_address(page);
	pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);

	return pmd;
}

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
rsa6144/5C63F4E3F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943

"The past tempts us, the present confuses us, the future frightens us.  And
our lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Huang Pei]

On Wed, Jul 21, 2021 at 12:23:39AM -0400, Joshua Kinard wrote:
> On 7/20/2021 23:43, Huang Pei wrote:
> > +. According to Documentation/vm/split_page_table_lock, handle failure
> > of pgtable_pmd_page_ctor
> > 
> > +. use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
> > 
> > Reported-by: Joshua Kinard <kumba@gentoo.org>
> > Signed-off-by: Huang Pei <huangpei@loongson.cn>
> > ---
> >  arch/mips/include/asm/pgalloc.h | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
> > index d0cf997b4ba8..5c9597a6c60c 100644
> > --- a/arch/mips/include/asm/pgalloc.h
> > +++ b/arch/mips/include/asm/pgalloc.h
> > @@ -62,11 +62,15 @@ static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
> >  	pmd_t *pmd = NULL;
> >  	struct page *pg;
> >  
> > -	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
> > +	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
> >  	if (pg) {
> > -		pgtable_pmd_page_ctor(pg);
> > -		pmd = (pmd_t *)page_address(pg);
> > -		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> > +		if(pgtable_pmd_page_ctor(pg)) {
> > +			pmd = (pmd_t *)page_address(pg);
> > +			pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> > +		} else {
> > +			__free_pages(pg, PMD_ORDER);
> > +		}
> > +
> >  	}
> >  	return pmd;
> >  }
> > 
> Instead of the nested if statements, why not go with something that looks more
> like what is in arch/x86/include/asm/pgalloc.h?
> 
> Note, I don't have a full kernel tree in front of me at the moment, so this is
> just the refactored function of pmd_alloc_one:
> 
> static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
> {
> 	pmd_t *pmd;
> 	struct page *page;
> 
> 	page = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
> 	if (!page)
> 		return NULL;
> 
> 	if (!pgtable_pmd_page_ctor(page)) {
> 		__free_pages(page, PMD_ORDER);
> 		return NULL;
> 	}
> 
> 	pmd = (pmd_t *)page_address(page);
> 	pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> 
> 	return pmd;
> }
> 

Much more readable, V2 just resend, thank you!

PS:

Latest Gentoo/MIPS stage3 is only available for big endian, is there any
little endian stage3 available? 
> -- 
> Joshua Kinard
> Gentoo/MIPS
> kumba@gentoo.org
> rsa6144/5C63F4E3F5C6C943 2015-04-27
> 177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
> 
> "The past tempts us, the present confuses us, the future frightens us.  And
> our lives slip away, moment by moment, lost in that vast, terrible in-between."
> 
> --Emperor Turhan, Centauri Republic

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Joshua Kinard]

On 7/21/2021 04:13, Huang Pei wrote:
> On Wed, Jul 21, 2021 at 12:23:39AM -0400, Joshua Kinard wrote:
>> On 7/20/2021 23:43, Huang Pei wrote:
>>> +. According to Documentation/vm/split_page_table_lock, handle failure
>>> of pgtable_pmd_page_ctor
>>>
>>> +. use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
>>>
>>> Reported-by: Joshua Kinard <kumba@gentoo.org>
>>> Signed-off-by: Huang Pei <huangpei@loongson.cn>

[snip]

> PS:
> 
> Latest Gentoo/MIPS stage3 is only available for big endian, is there any
> little endian stage3 available? 

At this time from us, unfortunately nothing recent.  I only focus on SGI
big-endian systems, as these are still the most widely-available (eBay) MIPS
systems that can still compile code within reasonable timeframes on Linux.

The Gentoo Embedded team has mipsel3 stages based on uclibc-ng from ~2018
available here:
https://gentoo.osuosl.org/experimental/mips/uclibc/mipsel3/

And some uclibc-ng mips32r2 stages, also 2018, here:
https://gentoo.osuosl.org/experimental/mips/uclibc/mips32r2/

Unfortunately, our support for more recent uclibc-ng-based builds has fallen
off the radar due to lack of upstream uclibc-ng activity.  I believe the
Embedded team is only focusing on musl libc-based build from now on.

There are some unofficial MIPS32 little-endian softfloat stages provided by
Manuel Lauss on his website here, albeit from 2018:
http://mlau.at/files/mips32-linux/

Last, if you truly need something to work with, I have had luck using the
OpenADK project's build system to coax musl-based big-endian rootfs tarballs
out of it in the past.  It's a very flexible build system and should be
capable of also generating mips32r* and little-endian builds as well.  It is
somewhat fickle, though, so you may have to kludge the build process at
times to get it to complete.  Depends very heavily on what packages you want
to build in.

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
rsa6144/5C63F4E3F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943

"The past tempts us, the present confuses us, the future frightens us.  And
our lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Thomas Bogendoerfer]

On Wed, Jul 21, 2021 at 05:30:45PM +0800, Huang Pei wrote:
> +. According to Documentation/vm/split_page_table_lock, handle failure
> of pgtable_pmd_page_ctor
> 
> +. Use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
> 
> +. Adjust coding style
> 
> Fixes: ed914d48b6a1 ("MIPS: add PMD table accounting into MIPS')
> Reported-by: Joshua Kinard <kumba@gentoo.org>
> Signed-off-by: Huang Pei <huangpei@loongson.cn>
> ---
>  arch/mips/include/asm/pgalloc.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

applied to mips-fixes.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Joshua Kinard]

On 7/21/2021 05:30, Huang Pei wrote:
> +. According to Documentation/vm/split_page_table_lock, handle failure
> of pgtable_pmd_page_ctor
> 
> +. Use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
> 
> +. Adjust coding style
> 
> Fixes: ed914d48b6a1 ("MIPS: add PMD table accounting into MIPS')
> Reported-by: Joshua Kinard <kumba@gentoo.org>
> Signed-off-by: Huang Pei <huangpei@loongson.cn>
> ---
>  arch/mips/include/asm/pgalloc.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
> index d0cf997b4ba8..139b4050259f 100644
> --- a/arch/mips/include/asm/pgalloc.h
> +++ b/arch/mips/include/asm/pgalloc.h
> @@ -59,15 +59,20 @@ do {							\
>  
>  static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
>  {
> -	pmd_t *pmd = NULL;
> +	pmd_t *pmd;
>  	struct page *pg;
>  
> -	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
> -	if (pg) {
> -		pgtable_pmd_page_ctor(pg);
> -		pmd = (pmd_t *)page_address(pg);
> -		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
> +	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
> +	if (!pg)
> +		return NULL;
> +
> +	if (!pgtable_pmd_page_ctor(pg)) {
> +		__free_pages(pg, PMD_ORDER);
> +		return NULL;
>  	}
> +
> +	pmd = (pmd_t *)page_address(pg);
> +	pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
>  	return pmd;
>  }
>  
> 

Reviewed-by: Joshua Kinard <kumba@gentoo.org>

[PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Huang Pei]

+. According to Documentation/vm/split_page_table_lock, handle failure
of pgtable_pmd_page_ctor

+. Use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT

+. Adjust coding style

Fixes: ed914d48b6a1 ("MIPS: add PMD table accounting into MIPS')
Reported-by: Joshua Kinard <kumba@gentoo.org>
Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 arch/mips/include/asm/pgalloc.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
index d0cf997b4ba8..139b4050259f 100644
--- a/arch/mips/include/asm/pgalloc.h
+++ b/arch/mips/include/asm/pgalloc.h
@@ -59,15 +59,20 @@ do {							\
 
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
 {
-	pmd_t *pmd = NULL;
+	pmd_t *pmd;
 	struct page *pg;
 
-	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
-	if (pg) {
-		pgtable_pmd_page_ctor(pg);
-		pmd = (pmd_t *)page_address(pg);
-		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
+	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
+	if (!pg)
+		return NULL;
+
+	if (!pgtable_pmd_page_ctor(pg)) {
+		__free_pages(pg, PMD_ORDER);
+		return NULL;
 	}
+
+	pmd = (pmd_t *)page_address(pg);
+	pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
 	return pmd;
 }
 
-- 
2.25.1

Re: [PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Thomas Bogendoerfer]

On Wed, Jul 21, 2021 at 01:20:23PM +0800, Huang Pei wrote:
> +. According to Documentation/vm/split_page_table_lock, handle failure
> of pgtable_pmd_page_ctor
> 
> +. Use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT
> 
> +. Adjust coding style
> 
> Reported-by: Joshua Kinard <kumba@gentoo.org>
> Signed-off-by: Huang Pei <huangpei@loongson.cn>

do we need a Fixes tag ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

[PATCH] MIPS: check return value of pgtable_pmd_page_ctor

[Huang Pei]

+. According to Documentation/vm/split_page_table_lock, handle failure
of pgtable_pmd_page_ctor

+. Use GFP_KERNEL_ACCOUNT instead of GFP_KERNEL|__GFP_ACCOUNT

+. Adjust coding style

Reported-by: Joshua Kinard <kumba@gentoo.org>
Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 arch/mips/include/asm/pgalloc.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
index d0cf997b4ba8..139b4050259f 100644
--- a/arch/mips/include/asm/pgalloc.h
+++ b/arch/mips/include/asm/pgalloc.h
@@ -59,15 +59,20 @@ do {							\
 
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
 {
-	pmd_t *pmd = NULL;
+	pmd_t *pmd;
 	struct page *pg;
 
-	pg = alloc_pages(GFP_KERNEL | __GFP_ACCOUNT, PMD_ORDER);
-	if (pg) {
-		pgtable_pmd_page_ctor(pg);
-		pmd = (pmd_t *)page_address(pg);
-		pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
+	pg = alloc_pages(GFP_KERNEL_ACCOUNT, PMD_ORDER);
+	if (!pg)
+		return NULL;
+
+	if (!pgtable_pmd_page_ctor(pg)) {
+		__free_pages(pg, PMD_ORDER);
+		return NULL;
 	}
+
+	pmd = (pmd_t *)page_address(pg);
+	pmd_init((unsigned long)pmd, (unsigned long)invalid_pte_table);
 	return pmd;
 }
 
-- 
2.25.1