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

Groups > linux.kernel > #1687195 > unrolled thread

Re: [PATCH v8 06/10] mm: thp: check pmd migration entry in common path

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 06/10] mm: thp: check pmd migration entry in common  path Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-07-14 11:40 +0200
    Re: [PATCH v8 06/10] mm: thp: check pmd migration entry in common  path "Zi Yan" <zi.yan@sent.com> - 2017-07-14 20:30 +0200

#1687195 — Re: [PATCH v8 06/10] mm: thp: check pmd migration entry in common path

On Sat, Jul 01, 2017 at 09:40:04AM -0400, Zi Yan wrote:
> From: Zi Yan <zi.yan@cs.rutgers.edu>
> 
> If one of callers of page migration starts to handle thp,
> memory management code start to see pmd migration entry, so we need
> to prepare for it before enabling. This patch changes various code
> point which checks the status of given pmds in order to prevent race
> between thp migration and the pmd-related works.
> 
> ChangeLog v1 -> v2:
> - introduce pmd_related() (I know the naming is not good, but can't
>   think up no better name. Any suggesntion is welcomed.)
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> ChangeLog v2 -> v3:
> - add is_swap_pmd()
> - a pmd entry should be pmd pointing to pte pages, is_swap_pmd(),
>   pmd_trans_huge(), pmd_devmap(), or pmd_none()
> - pmd_none_or_trans_huge_or_clear_bad() and pmd_trans_unstable() return
>   true on pmd_migration_entry, so that migration entries are not
>   treated as pmd page table entries.
> 
> ChangeLog v4 -> v5:
> - add explanation in pmd_none_or_trans_huge_or_clear_bad() to state
>   the equivalence of !pmd_present() and is_pmd_migration_entry()
> - fix migration entry wait deadlock code (from v1) in follow_page_mask()
> - remove unnecessary code (from v1) in follow_trans_huge_pmd()
> - use is_swap_pmd() instead of !pmd_present() for pmd migration entry,
>   so it will not be confused with pmd_none()
> - change author information
> 
> ChangeLog v5 -> v7
> - use macro to disable the code when thp migration is not enabled
> 
> ChangeLog v7 -> v8
> - remove not used code in do_huge_pmd_wp_page()
> - copy the comment from change_pte_range() on downgrading
>   write migration entry to read to change_huge_pmd()
> 
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/mm/gup.c             |  7 +++--
>  fs/proc/task_mmu.c            | 33 ++++++++++++++-------
>  include/asm-generic/pgtable.h | 17 ++++++++++-
>  include/linux/huge_mm.h       | 14 +++++++--
>  mm/gup.c                      | 22 ++++++++++++--
>  mm/huge_memory.c              | 67 +++++++++++++++++++++++++++++++++++++++----
>  mm/memcontrol.c               |  5 ++++
>  mm/memory.c                   | 12 ++++++--
>  mm/mprotect.c                 |  4 +--
>  mm/mremap.c                   |  2 +-
>  10 files changed, 154 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
> index 456dfdfd2249..096bbcc801e6 100644
> --- a/arch/x86/mm/gup.c
> +++ b/arch/x86/mm/gup.c
> @@ -9,6 +9,7 @@
>  #include <linux/vmstat.h>
>  #include <linux/highmem.h>
>  #include <linux/swap.h>
> +#include <linux/swapops.h>
>  #include <linux/memremap.h>
>  
>  #include <asm/mmu_context.h>
> @@ -243,9 +244,11 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
>  		pmd_t pmd = *pmdp;
>  
>  		next = pmd_addr_end(addr, end);
> -		if (pmd_none(pmd))
> +		if (!pmd_present(pmd)) {
> +			VM_BUG_ON(is_swap_pmd(pmd) && IS_ENABLED(CONFIG_MIGRATION) &&
> +					  !is_pmd_migration_entry(pmd));

This VM_BUG_ON() triggers when gup is called on hugetlb hwpoison entry.
I think that in such case kernel falls into the gup slow path, and
a page fault in follow_hugetlb_page() can properly report the error to
affected processes, so no need to alarm with BUG_ON.

Could you make this VM_BUG_ON more specific, or just remove it?

Thanks,
Naoya Horiguchi

>  			return 0;
> -		if (unlikely(pmd_large(pmd) || !pmd_present(pmd))) {
> +		} else if (unlikely(pmd_large(pmd))) {
>  			/*
>  			 * NUMA hinting faults need to be handled in the GUP
>  			 * slowpath for accounting purposes and so that they

[toc] | [next] | [standalone]

#1687601

[Multipart message — attachments visible in raw view] — view raw

On 14 Jul 2017, at 5:29, Naoya Horiguchi wrote:

> On Sat, Jul 01, 2017 at 09:40:04AM -0400, Zi Yan wrote:
>> From: Zi Yan <zi.yan@cs.rutgers.edu>
>>
>> If one of callers of page migration starts to handle thp,
>> memory management code start to see pmd migration entry, so we need
>> to prepare for it before enabling. This patch changes various code
>> point which checks the status of given pmds in order to prevent race
>> between thp migration and the pmd-related works.
>>
>> ChangeLog v1 -> v2:
>> - introduce pmd_related() (I know the naming is not good, but can't
>>   think up no better name. Any suggesntion is welcomed.)
>>
>> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>
>> ChangeLog v2 -> v3:
>> - add is_swap_pmd()
>> - a pmd entry should be pmd pointing to pte pages, is_swap_pmd(),
>>   pmd_trans_huge(), pmd_devmap(), or pmd_none()
>> - pmd_none_or_trans_huge_or_clear_bad() and pmd_trans_unstable() return
>>   true on pmd_migration_entry, so that migration entries are not
>>   treated as pmd page table entries.
>>
>> ChangeLog v4 -> v5:
>> - add explanation in pmd_none_or_trans_huge_or_clear_bad() to state
>>   the equivalence of !pmd_present() and is_pmd_migration_entry()
>> - fix migration entry wait deadlock code (from v1) in follow_page_mask()
>> - remove unnecessary code (from v1) in follow_trans_huge_pmd()
>> - use is_swap_pmd() instead of !pmd_present() for pmd migration entry,
>>   so it will not be confused with pmd_none()
>> - change author information
>>
>> ChangeLog v5 -> v7
>> - use macro to disable the code when thp migration is not enabled
>>
>> ChangeLog v7 -> v8
>> - remove not used code in do_huge_pmd_wp_page()
>> - copy the comment from change_pte_range() on downgrading
>>   write migration entry to read to change_huge_pmd()
>>
>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> ---
>>  arch/x86/mm/gup.c             |  7 +++--
>>  fs/proc/task_mmu.c            | 33 ++++++++++++++-------
>>  include/asm-generic/pgtable.h | 17 ++++++++++-
>>  include/linux/huge_mm.h       | 14 +++++++--
>>  mm/gup.c                      | 22 ++++++++++++--
>>  mm/huge_memory.c              | 67 +++++++++++++++++++++++++++++++++++++++----
>>  mm/memcontrol.c               |  5 ++++
>>  mm/memory.c                   | 12 ++++++--
>>  mm/mprotect.c                 |  4 +--
>>  mm/mremap.c                   |  2 +-
>>  10 files changed, 154 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
>> index 456dfdfd2249..096bbcc801e6 100644
>> --- a/arch/x86/mm/gup.c
>> +++ b/arch/x86/mm/gup.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/vmstat.h>
>>  #include <linux/highmem.h>
>>  #include <linux/swap.h>
>> +#include <linux/swapops.h>
>>  #include <linux/memremap.h>
>>
>>  #include <asm/mmu_context.h>
>> @@ -243,9 +244,11 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
>>  		pmd_t pmd = *pmdp;
>>
>>  		next = pmd_addr_end(addr, end);
>> -		if (pmd_none(pmd))
>> +		if (!pmd_present(pmd)) {
>> +			VM_BUG_ON(is_swap_pmd(pmd) && IS_ENABLED(CONFIG_MIGRATION) &&
>> +					  !is_pmd_migration_entry(pmd));
>
> This VM_BUG_ON() triggers when gup is called on hugetlb hwpoison entry.
> I think that in such case kernel falls into the gup slow path, and
> a page fault in follow_hugetlb_page() can properly report the error to
> affected processes, so no need to alarm with BUG_ON.
>
> Could you make this VM_BUG_ON more specific, or just remove it?

I will remove it, since adding code to detect hugetlb hwpoison entry
to existing VM_BUG_ON() will be quite messy.

Thanks for pointing this out.

--
Best Regards
Yan Zi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web