Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658975 > unrolled thread

[PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove Jérôme Glisse <jglisse@redhat.com> - 2017-06-06 19:40 +0200
    Re: [PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove Logan Gunthorpe <logang@deltatee.com> - 2017-06-06 22:00 +0200
    Re: [PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-07 12:50 +0200
      Re: [PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove Jerome Glisse <jglisse@redhat.com> - 2017-06-07 17:10 +0200

#1658975 — [PATCH] x86/mm/hotplug: fix BUG_ON() after hotremove

With commit af2cf278ef4f we no longer free pud so that we
do not have synchronize all pgd on hotremove/vfree. But the
new 5 level page table code re-added that code f2a6a705 and
thus we now trigger a BUG_ON() l128 in sync_global_pgds()

This patch remove free_pud() like in af2cf278ef4f

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
---
 arch/x86/mm/init_64.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index a8a9972..8cf7e99 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -772,24 +772,6 @@ static void __meminit free_pmd_table(pmd_t *pmd_start, pud_t *pud)
 	spin_unlock(&init_mm.page_table_lock);
 }
 
-static void __meminit free_pud_table(pud_t *pud_start, p4d_t *p4d)
-{
-	pud_t *pud;
-	int i;
-
-	for (i = 0; i < PTRS_PER_PUD; i++) {
-		pud = pud_start + i;
-		if (!pud_none(*pud))
-			return;
-	}
-
-	/* free a pud talbe */
-	free_pagetable(p4d_page(*p4d), 0);
-	spin_lock(&init_mm.page_table_lock);
-	p4d_clear(p4d);
-	spin_unlock(&init_mm.page_table_lock);
-}
-
 static void __meminit
 remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end,
 		 bool direct)
@@ -991,7 +973,6 @@ remove_p4d_table(p4d_t *p4d_start, unsigned long addr, unsigned long end,
 
 		pud_base = pud_offset(p4d, 0);
 		remove_pud_table(pud_base, addr, next, direct);
-		free_pud_table(pud_base, p4d);
 	}
 
 	if (direct)
-- 
2.9.3

[toc] | [next] | [standalone]

#1659163

Thanks Jerome! This indeed fixes the bug I reported.

Tested-by: Logan Gunthorpe <logang@deltatee.com>

Logan


On 06/06/17 11:35 AM, Jérôme Glisse wrote:
> With commit af2cf278ef4f we no longer free pud so that we
> do not have synchronize all pgd on hotremove/vfree. But the
> new 5 level page table code re-added that code f2a6a705 and
> thus we now trigger a BUG_ON() l128 in sync_global_pgds()
> 
> This patch remove free_pud() like in af2cf278ef4f
> 
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> ---
>  arch/x86/mm/init_64.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index a8a9972..8cf7e99 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -772,24 +772,6 @@ static void __meminit free_pmd_table(pmd_t *pmd_start, pud_t *pud)
>  	spin_unlock(&init_mm.page_table_lock);
>  }
>  
> -static void __meminit free_pud_table(pud_t *pud_start, p4d_t *p4d)
> -{
> -	pud_t *pud;
> -	int i;
> -
> -	for (i = 0; i < PTRS_PER_PUD; i++) {
> -		pud = pud_start + i;
> -		if (!pud_none(*pud))
> -			return;
> -	}
> -
> -	/* free a pud talbe */
> -	free_pagetable(p4d_page(*p4d), 0);
> -	spin_lock(&init_mm.page_table_lock);
> -	p4d_clear(p4d);
> -	spin_unlock(&init_mm.page_table_lock);
> -}
> -
>  static void __meminit
>  remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end,
>  		 bool direct)
> @@ -991,7 +973,6 @@ remove_p4d_table(p4d_t *p4d_start, unsigned long addr, unsigned long end,
>  
>  		pud_base = pud_offset(p4d, 0);
>  		remove_pud_table(pud_base, addr, next, direct);
> -		free_pud_table(pud_base, p4d);
>  	}
>  
>  	if (direct)
> 

[toc] | [prev] | [next] | [standalone]

#1659668

On Tue, Jun 06, 2017 at 01:35:12PM -0400, Jérôme Glisse wrote:
> With commit af2cf278ef4f we no longer free pud so that we
> do not have synchronize all pgd on hotremove/vfree. But the
> new 5 level page table code re-added that code f2a6a705 and
> thus we now trigger a BUG_ON() l128 in sync_global_pgds()
> 
> This patch remove free_pud() like in af2cf278ef4f

Good catch. Thanks!

But I think we only need to skip free_pud_table() for 4-level paging.
If we don't we would leave 513 page tables around instead of one in
5-level paging case.

I don't think it's acceptable.

And please use patch subject lines along with commit hashes to simplify
reading commit message.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1659917

On Wed, Jun 07, 2017 at 01:47:15PM +0300, Kirill A. Shutemov wrote:
> On Tue, Jun 06, 2017 at 01:35:12PM -0400, Jérôme Glisse wrote:
> > With commit af2cf278ef4f we no longer free pud so that we
> > do not have synchronize all pgd on hotremove/vfree. But the
> > new 5 level page table code re-added that code f2a6a705 and
> > thus we now trigger a BUG_ON() l128 in sync_global_pgds()
> > 
> > This patch remove free_pud() like in af2cf278ef4f
> 
> Good catch. Thanks!
> 
> But I think we only need to skip free_pud_table() for 4-level paging.
> If we don't we would leave 513 page tables around instead of one in
> 5-level paging case.
> 
> I don't think it's acceptable.
> 
> And please use patch subject lines along with commit hashes to simplify
> reading commit message.
> 

I sent a v2 that disable free_pud in 4 level page table config.
Note that your patchset that allow switching between 4 and 5 at
boot time will need to update that code. As this patch is a fix
and your boot time switching is an RFC i assume the fix will go
in first.

Cheers,
Jérôme

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web