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

Groups > linux.kernel > #1573913 > unrolled thread

[PATCH v3 00/14] mm: page migration enhancement for thp

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/14] mm: page migration enhancement for thp Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
    [PATCH v3 11/14] mm: hwpoison: soft offline supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
    [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
      Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp  migration Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
        Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration "Zi Yan" <zi.yan@sent.com> - 2017-02-09 18:40 +0100
    [PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1 Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
      Re: [PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7  to bit 1 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
    [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
      Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-02-06 05:10 +0100
      Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-06 08:50 +0100
        Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Zi Yan" <zi.yan@sent.com> - 2017-02-06 14:10 +0100
          Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-07 00:30 +0100
      Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-06 17:10 +0100
        Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Zi Yan" <zi.yan@sent.com> - 2017-02-06 17:40 +0100
          Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-06 18:40 +0100
        Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-02-07 15:00 +0100
      Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 15:30 +0100
        Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 17:40 +0100
          Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 19:00 +0100
            Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-13 12:00 +0100
              Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in  zap_pmd_range() Andrea Arcangeli <aarcange@redhat.com> - 2017-02-13 15:50 +0100
    [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
      Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-06 07:30 +0100
        Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. "Zi Yan" <zi.yan@sent.com> - 2017-02-06 13:20 +0100
      Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Matthew Wilcox <willy@infradead.org> - 2017-02-06 16:10 +0100
        Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. "Zi Yan" <zi.yan@sent.com> - 2017-02-06 16:10 +0100
    [PATCH v3 09/14] mm: thp: check pmd migration entry in common path Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
      Re: [PATCH v3 09/14] mm: thp: check pmd migration entry in common  path Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
    [PATCH v3 07/14] mm: thp: introduce CONFIG_ARCH_ENABLE_THP_MIGRATION Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
    [PATCH v3 05/14] mm: mempolicy: add queue_pages_node_check() Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
    [PATCH v3 14/14] mm: memory_hotplug: memory hotremove supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
    [PATCH v3 02/14] mm: thp: create new __zap_huge_pmd_locked function. Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100

Page 1 of 2  [1] 2  Next page →

#1573913 — [PATCH v3 00/14] mm: page migration enhancement for thp

From: Zi Yan <ziy@nvidia.com>

Hi all,

The patches are rebased on mmotm-2017-02-01-15-35 with feedbacks from 
Naoya Horiguchi's v2 patches.

I fix a bug in zap_pmd_range() and include the fixes in Patches 1-3.
The racy check in zap_pmd_range() can miss pmd_protnone and pmd_migration_entry,
which leads to PTE page table not freed.

In Patch 4, I move _PAGE_SWP_SOFT_DIRTY to bit 1. Because bit 6 (used in v2)
can be set by some CPUs by mistake and the new swap entry format does not use
bit 1-4.

I also adjust two core migration functions, set_pmd_migration_entry() and
remove_migration_pmd(), to use Kirill A. Shutemov's page_vma_mapped_walk()
function. Patch 8 needs Kirill's comments, since I also add changes
to his page_vma_mapped_walk() function with pmd_migration_entry handling.

In Patch 8, I replace pmdp_huge_get_and_clear() with pmdp_huge_clear_flush()
in set_pmd_migration_entry() to avoid data corruption after page migration.

In Patch 9, I include is_pmd_migration_entry() in pmd_none_or_trans_huge_or_clear_bad().
Otherwise, a pmd_migration_entry is treated as pmd_bad and cleared, which
leads to deposited PTE page table not freed.

I personally use this patchset with my customized kernel to test frequent
page migrations by replacing page reclaim with page migration.
The bugs fixed in Patches 1-3 and 8 was discovered while I am testing my kernel.
I did a 16-hour stress test that has ~7 billion total page migrations.
No error or data corruption was found. 


General description 
===========================================

This patchset enhances page migration functionality to handle thp migration
for various page migration's callers:
 - mbind(2)
 - move_pages(2)
 - migrate_pages(2)
 - cgroup/cpuset migration
 - memory hotremove
 - soft offline

The main benefit is that we can avoid unnecessary thp splits, which helps us
avoid performance decrease when your applications handles NUMA optimization on
their own.

The implementation is similar to that of normal page migration, the key point
is that we modify a pmd to a pmd migration entry in swap-entry like format.


Any comments or advices are welcomed.

Best Regards,
Yan Zi

Naoya Horiguchi (11):
  mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1
  mm: mempolicy: add queue_pages_node_check()
  mm: thp: introduce separate TTU flag for thp freezing
  mm: thp: introduce CONFIG_ARCH_ENABLE_THP_MIGRATION
  mm: thp: enable thp migration in generic path
  mm: thp: check pmd migration entry in common path
  mm: soft-dirty: keep soft-dirty bits over thp migration
  mm: hwpoison: soft offline supports thp migration
  mm: mempolicy: mbind and migrate_pages support thp migration
  mm: migrate: move_pages() supports thp migration
  mm: memory_hotplug: memory hotremove supports thp migration

Zi Yan (3):
  mm: thp: make __split_huge_pmd_locked visible.
  mm: thp: create new __zap_huge_pmd_locked function.
  mm: use pmd lock instead of racy checks in zap_pmd_range()

 arch/x86/Kconfig                     |   4 +
 arch/x86/include/asm/pgtable.h       |  17 ++
 arch/x86/include/asm/pgtable_64.h    |   2 +
 arch/x86/include/asm/pgtable_types.h |  10 +-
 arch/x86/mm/gup.c                    |   4 +-
 fs/proc/task_mmu.c                   |  37 +++--
 include/asm-generic/pgtable.h        | 105 ++++--------
 include/linux/huge_mm.h              |  36 ++++-
 include/linux/rmap.h                 |   1 +
 include/linux/swapops.h              | 146 ++++++++++++++++-
 mm/Kconfig                           |   3 +
 mm/gup.c                             |  20 ++-
 mm/huge_memory.c                     | 302 +++++++++++++++++++++++++++++------
 mm/madvise.c                         |   2 +
 mm/memcontrol.c                      |   2 +
 mm/memory-failure.c                  |  31 ++--
 mm/memory.c                          |  33 ++--
 mm/memory_hotplug.c                  |  17 +-
 mm/mempolicy.c                       | 124 ++++++++++----
 mm/migrate.c                         |  66 ++++++--
 mm/mprotect.c                        |   6 +-
 mm/mremap.c                          |   2 +-
 mm/page_vma_mapped.c                 |  13 +-
 mm/pagewalk.c                        |   2 +
 mm/pgtable-generic.c                 |   3 +-
 mm/rmap.c                            |  21 ++-
 26 files changed, 770 insertions(+), 239 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1573914 — [PATCH v3 11/14] mm: hwpoison: soft offline supports thp migration

From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

This patch enables thp migration for soft offline.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/memory-failure.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 3f3cfd4e1901..95db94207d01 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1483,7 +1483,17 @@ static struct page *new_page(struct page *p, unsigned long private, int **x)
 	if (PageHuge(p))
 		return alloc_huge_page_node(page_hstate(compound_head(p)),
 						   nid);
-	else
+	else if (thp_migration_supported() && PageTransHuge(p)) {
+		struct page *thp;
+
+		thp = alloc_pages_node(nid,
+			(GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
+			HPAGE_PMD_ORDER);
+		if (!thp)
+			return NULL;
+		prep_transhuge_page(thp);
+		return thp;
+	} else
 		return __alloc_pages_node(nid, GFP_HIGHUSER_MOVABLE, 0);
 }
 
@@ -1691,28 +1701,11 @@ static int __soft_offline_page(struct page *page, int flags)
 static int soft_offline_in_use_page(struct page *page, int flags)
 {
 	int ret;
-	struct page *hpage = compound_head(page);
-
-	if (!PageHuge(page) && PageTransHuge(hpage)) {
-		lock_page(hpage);
-		if (!PageAnon(hpage) || unlikely(split_huge_page(hpage))) {
-			unlock_page(hpage);
-			if (!PageAnon(hpage))
-				pr_info("soft offline: %#lx: non anonymous thp\n", page_to_pfn(page));
-			else
-				pr_info("soft offline: %#lx: thp split failed\n", page_to_pfn(page));
-			put_hwpoison_page(hpage);
-			return -EBUSY;
-		}
-		unlock_page(hpage);
-		get_hwpoison_page(page);
-		put_hwpoison_page(hpage);
-	}
 
 	if (PageHuge(page))
 		ret = soft_offline_huge_page(page, flags);
 	else
-		ret = __soft_offline_page(page, flags);
+		ret = __soft_offline_page(compound_head(page), flags);
 
 	return ret;
 }
-- 
2.11.0

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

#1573916 — [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration

From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

This patch enables thp migration for move_pages(2).

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/migrate.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 84181a3668c6..9bcaccb481ac 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1413,7 +1413,17 @@ static struct page *new_page_node(struct page *p, unsigned long private,
 	if (PageHuge(p))
 		return alloc_huge_page_node(page_hstate(compound_head(p)),
 					pm->node);
-	else
+	else if (thp_migration_supported() && PageTransHuge(p)) {
+		struct page *thp;
+
+		thp = alloc_pages_node(pm->node,
+			(GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
+			HPAGE_PMD_ORDER);
+		if (!thp)
+			return NULL;
+		prep_transhuge_page(thp);
+		return thp;
+	} else
 		return __alloc_pages_node(pm->node,
 				GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
 }
@@ -1440,6 +1450,8 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
 	for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
 		struct vm_area_struct *vma;
 		struct page *page;
+		struct page *head;
+		unsigned int follflags;
 
 		err = -EFAULT;
 		vma = find_vma(mm, pp->addr);
@@ -1447,8 +1459,10 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
 			goto set_status;
 
 		/* FOLL_DUMP to ignore special (like zero) pages */
-		page = follow_page(vma, pp->addr,
-				FOLL_GET | FOLL_SPLIT | FOLL_DUMP);
+		follflags = FOLL_GET | FOLL_SPLIT | FOLL_DUMP;
+		if (!thp_migration_supported())
+			follflags |= FOLL_SPLIT;
+		page = follow_page(vma, pp->addr, follflags);
 
 		err = PTR_ERR(page);
 		if (IS_ERR(page))
@@ -1458,7 +1472,6 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
 		if (!page)
 			goto set_status;
 
-		pp->page = page;
 		err = page_to_nid(page);
 
 		if (err == pp->node)
@@ -1473,16 +1486,22 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
 			goto put_and_set;
 
 		if (PageHuge(page)) {
-			if (PageHead(page))
+			if (PageHead(page)) {
 				isolate_huge_page(page, &pagelist);
+				err = 0;
+				pp->page = page;
+			}
 			goto put_and_set;
 		}
 
-		err = isolate_lru_page(page);
+		pp->page = compound_head(page);
+		head = compound_head(page);
+		err = isolate_lru_page(head);
 		if (!err) {
-			list_add_tail(&page->lru, &pagelist);
-			inc_node_page_state(page, NR_ISOLATED_ANON +
-					    page_is_file_cache(page));
+			list_add_tail(&head->lru, &pagelist);
+			mod_node_page_state(page_pgdat(head),
+				NR_ISOLATED_ANON + page_is_file_cache(head),
+				hpage_nr_pages(head));
 		}
 put_and_set:
 		/*
-- 
2.11.0

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

#1577410 — Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration

On Sun, Feb 05, 2017 at 11:12:51AM -0500, Zi Yan wrote:
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> This patch enables thp migration for move_pages(2).
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/migrate.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 84181a3668c6..9bcaccb481ac 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1413,7 +1413,17 @@ static struct page *new_page_node(struct page *p, unsigned long private,
>  	if (PageHuge(p))
>  		return alloc_huge_page_node(page_hstate(compound_head(p)),
>  					pm->node);
> -	else
> +	else if (thp_migration_supported() && PageTransHuge(p)) {
> +		struct page *thp;
> +
> +		thp = alloc_pages_node(pm->node,
> +			(GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> +			HPAGE_PMD_ORDER);
> +		if (!thp)
> +			return NULL;
> +		prep_transhuge_page(thp);
> +		return thp;
> +	} else
>  		return __alloc_pages_node(pm->node,
>  				GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
>  }
> @@ -1440,6 +1450,8 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>  	for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
>  		struct vm_area_struct *vma;
>  		struct page *page;
> +		struct page *head;
> +		unsigned int follflags;
>  
>  		err = -EFAULT;
>  		vma = find_vma(mm, pp->addr);
> @@ -1447,8 +1459,10 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>  			goto set_status;
>  
>  		/* FOLL_DUMP to ignore special (like zero) pages */
> -		page = follow_page(vma, pp->addr,
> -				FOLL_GET | FOLL_SPLIT | FOLL_DUMP);
> +		follflags = FOLL_GET | FOLL_SPLIT | FOLL_DUMP;

FOLL_SPLIT should be added depending on thp_migration_supported().

Thanks,
Naoya Horiguchi

> +		if (!thp_migration_supported())
> +			follflags |= FOLL_SPLIT;
> +		page = follow_page(vma, pp->addr, follflags);
>  
>  		err = PTR_ERR(page);
>  		if (IS_ERR(page))
> @@ -1458,7 +1472,6 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>  		if (!page)
>  			goto set_status;
>  
> -		pp->page = page;
>  		err = page_to_nid(page);
>  
>  		if (err == pp->node)
> @@ -1473,16 +1486,22 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>  			goto put_and_set;
>  
>  		if (PageHuge(page)) {
> -			if (PageHead(page))
> +			if (PageHead(page)) {
>  				isolate_huge_page(page, &pagelist);
> +				err = 0;
> +				pp->page = page;
> +			}
>  			goto put_and_set;
>  		}
>  
> -		err = isolate_lru_page(page);
> +		pp->page = compound_head(page);
> +		head = compound_head(page);
> +		err = isolate_lru_page(head);
>  		if (!err) {
> -			list_add_tail(&page->lru, &pagelist);
> -			inc_node_page_state(page, NR_ISOLATED_ANON +
> -					    page_is_file_cache(page));
> +			list_add_tail(&head->lru, &pagelist);
> +			mod_node_page_state(page_pgdat(head),
> +				NR_ISOLATED_ANON + page_is_file_cache(head),
> +				hpage_nr_pages(head));
>  		}
>  put_and_set:
>  		/*
> -- 
> 2.11.0
> 

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

#1577820 — Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration

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

On 9 Feb 2017, at 3:16, Naoya Horiguchi wrote:

> On Sun, Feb 05, 2017 at 11:12:51AM -0500, Zi Yan wrote:
>> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>
>> This patch enables thp migration for move_pages(2).
>>
>> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> ---
>>  mm/migrate.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 84181a3668c6..9bcaccb481ac 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1413,7 +1413,17 @@ static struct page *new_page_node(struct page *p, unsigned long private,
>>  	if (PageHuge(p))
>>  		return alloc_huge_page_node(page_hstate(compound_head(p)),
>>  					pm->node);
>> -	else
>> +	else if (thp_migration_supported() && PageTransHuge(p)) {
>> +		struct page *thp;
>> +
>> +		thp = alloc_pages_node(pm->node,
>> +			(GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
>> +			HPAGE_PMD_ORDER);
>> +		if (!thp)
>> +			return NULL;
>> +		prep_transhuge_page(thp);
>> +		return thp;
>> +	} else
>>  		return __alloc_pages_node(pm->node,
>>  				GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
>>  }
>> @@ -1440,6 +1450,8 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>>  	for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
>>  		struct vm_area_struct *vma;
>>  		struct page *page;
>> +		struct page *head;
>> +		unsigned int follflags;
>>
>>  		err = -EFAULT;
>>  		vma = find_vma(mm, pp->addr);
>> @@ -1447,8 +1459,10 @@ static int do_move_page_to_node_array(struct mm_struct *mm,
>>  			goto set_status;
>>
>>  		/* FOLL_DUMP to ignore special (like zero) pages */
>> -		page = follow_page(vma, pp->addr,
>> -				FOLL_GET | FOLL_SPLIT | FOLL_DUMP);
>> +		follflags = FOLL_GET | FOLL_SPLIT | FOLL_DUMP;
>
> FOLL_SPLIT should be added depending on thp_migration_supported().

Sure. I will fix it.


--
Best Regards
Yan Zi

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

#1573917 — [PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1

From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

pmd_present() checks _PAGE_PSE along with _PAGE_PRESENT to avoid
false negative return when it races with thp spilt
(during which _PAGE_PRESENT is temporary cleared.) I don't think that
dropping _PAGE_PSE check in pmd_present() works well because it can
hurt optimization of tlb handling in thp split.
In the current kernel, bits 1-4 are not used in non-present format
since commit 00839ee3b299 ("x86/mm: Move swap offset/type up in PTE to
work around erratum"). So let's move _PAGE_SWP_SOFT_DIRTY to bit 1.
Bit 7 is used as reserved (always clear), so please don't use it for
other purpose.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

ChangeLog v3:
- Move _PAGE_SWP_SOFT_DIRTY to bit 1, it was placed at bit 6. Because
some CPUs might accidentally set bit 5 or 6.

Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
---
 arch/x86/include/asm/pgtable_types.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 8b4de22d6429..3695abd58ef6 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -97,15 +97,15 @@
 /*
  * Tracking soft dirty bit when a page goes to a swap is tricky.
  * We need a bit which can be stored in pte _and_ not conflict
- * with swap entry format. On x86 bits 6 and 7 are *not* involved
- * into swap entry computation, but bit 6 is used for nonlinear
- * file mapping, so we borrow bit 7 for soft dirty tracking.
+ * with swap entry format. On x86 bits 1-4 are *not* involved
+ * into swap entry computation, but bit 7 is used for thp migration,
+ * so we borrow bit 1 for soft dirty tracking.
  *
  * Please note that this bit must be treated as swap dirty page
- * mark if and only if the PTE has present bit clear!
+ * mark if and only if the PTE/PMD has present bit clear!
  */
 #ifdef CONFIG_MEM_SOFT_DIRTY
-#define _PAGE_SWP_SOFT_DIRTY	_PAGE_PSE
+#define _PAGE_SWP_SOFT_DIRTY	_PAGE_RW
 #else
 #define _PAGE_SWP_SOFT_DIRTY	(_AT(pteval_t, 0))
 #endif
-- 
2.11.0

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

#1577404 — Re: [PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1

On Sun, Feb 05, 2017 at 11:12:42AM -0500, Zi Yan wrote:
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> pmd_present() checks _PAGE_PSE along with _PAGE_PRESENT to avoid
> false negative return when it races with thp spilt
> (during which _PAGE_PRESENT is temporary cleared.) I don't think that
> dropping _PAGE_PSE check in pmd_present() works well because it can
> hurt optimization of tlb handling in thp split.
> In the current kernel, bits 1-4 are not used in non-present format
> since commit 00839ee3b299 ("x86/mm: Move swap offset/type up in PTE to
> work around erratum"). So let's move _PAGE_SWP_SOFT_DIRTY to bit 1.
> Bit 7 is used as reserved (always clear), so please don't use it for
> other purpose.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> ChangeLog v3:
> - Move _PAGE_SWP_SOFT_DIRTY to bit 1, it was placed at bit 6. Because
> some CPUs might accidentally set bit 5 or 6.
> 
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> ---

More documenting will be helpful, could you do like follows?

Thanks,
Naoya Horiguchi
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Sun, 5 Feb 2017 11:12:42 -0500
Subject: [PATCH] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1

pmd_present() checks _PAGE_PSE along with _PAGE_PRESENT to avoid
false negative return when it races with thp spilt
(during which _PAGE_PRESENT is temporary cleared.) I don't think that
dropping _PAGE_PSE check in pmd_present() works well because it can
hurt optimization of tlb handling in thp split.
In the current kernel, bits 1-4 are not used in non-present format
since commit 00839ee3b299 ("x86/mm: Move swap offset/type up in PTE to
work around erratum"). So let's move _PAGE_SWP_SOFT_DIRTY to bit 1.
Bit 7 is used as reserved (always clear), so please don't use it for
other purpose.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
---
 arch/x86/include/asm/pgtable_64.h    | 12 +++++++++---
 arch/x86/include/asm/pgtable_types.h | 10 +++++-----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
index 73c7ccc38912..07c98c85cc96 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -157,15 +157,21 @@ static inline int pgd_large(pgd_t pgd) { return 0; }
 /*
  * Encode and de-code a swap entry
  *
- * |     ...            | 11| 10|  9|8|7|6|5| 4| 3|2|1|0| <- bit number
- * |     ...            |SW3|SW2|SW1|G|L|D|A|CD|WT|U|W|P| <- bit names
- * | OFFSET (14->63) | TYPE (9-13)  |0|X|X|X| X| X|X|X|0| <- swp entry
+ * |     ...            | 11| 10|  9|8|7|6|5| 4| 3|2| 1|0| <- bit number
+ * |     ...            |SW3|SW2|SW1|G|L|D|A|CD|WT|U| W|P| <- bit names
+ * | OFFSET (14->63) | TYPE (9-13)  |0|0|X|X| X| X|X|SD|0| <- swp entry
  *
  * G (8) is aliased and used as a PROT_NONE indicator for
  * !present ptes.  We need to start storing swap entries above
  * there.  We also need to avoid using A and D because of an
  * erratum where they can be incorrectly set by hardware on
  * non-present PTEs.
+ *
+ * SD (1) in swp entry is used to store soft dirty bit, which helps us
+ * remember soft dirty over page migration.
+ *
+ * Bit 7 in swp entry should be 0 because pmd_present checks not only P,
+ * but G.
  */
 #define SWP_TYPE_FIRST_BIT (_PAGE_BIT_PROTNONE + 1)
 #define SWP_TYPE_BITS 5
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 8b4de22d6429..3695abd58ef6 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -97,15 +97,15 @@
 /*
  * Tracking soft dirty bit when a page goes to a swap is tricky.
  * We need a bit which can be stored in pte _and_ not conflict
- * with swap entry format. On x86 bits 6 and 7 are *not* involved
- * into swap entry computation, but bit 6 is used for nonlinear
- * file mapping, so we borrow bit 7 for soft dirty tracking.
+ * with swap entry format. On x86 bits 1-4 are *not* involved
+ * into swap entry computation, but bit 7 is used for thp migration,
+ * so we borrow bit 1 for soft dirty tracking.
  *
  * Please note that this bit must be treated as swap dirty page
- * mark if and only if the PTE has present bit clear!
+ * mark if and only if the PTE/PMD has present bit clear!
  */
 #ifdef CONFIG_MEM_SOFT_DIRTY
-#define _PAGE_SWP_SOFT_DIRTY	_PAGE_PSE
+#define _PAGE_SWP_SOFT_DIRTY	_PAGE_RW
 #else
 #define _PAGE_SWP_SOFT_DIRTY	(_AT(pteval_t, 0))
 #endif
-- 
2.7.4

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

#1573918 — [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

From: Zi Yan <ziy@nvidia.com>

Originally, zap_pmd_range() checks pmd value without taking pmd lock.
This can cause pmd_protnone entry not being freed.

Because there are two steps in changing a pmd entry to a pmd_protnone
entry. First, the pmd entry is cleared to a pmd_none entry, then,
the pmd_none entry is changed into a pmd_protnone entry.
The racy check, even with barrier, might only see the pmd_none entry
in zap_pmd_range(), thus, the mapping is neither split nor zapped.

Later, in free_pmd_range(), pmd_none_or_clear() will see the
pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
since the pmd_protnone entry is not properly freed, the corresponding
deposited pte page table is not freed either.

This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.

This patch relies on __split_huge_pmd_locked() and
__zap_huge_pmd_locked().

Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
---
 mm/memory.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 3929b015faf7..7cfdd5208ef5 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
 				struct zap_details *details)
 {
 	pmd_t *pmd;
+	spinlock_t *ptl;
 	unsigned long next;
 
 	pmd = pmd_offset(pud, addr);
+	ptl = pmd_lock(vma->vm_mm, pmd);
 	do {
 		next = pmd_addr_end(addr, end);
 		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
 			if (next - addr != HPAGE_PMD_SIZE) {
 				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
 				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
-				__split_huge_pmd(vma, pmd, addr, false, NULL);
-			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
-				goto next;
+				__split_huge_pmd_locked(vma, pmd, addr, false);
+			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
+				continue;
 			/* fall through */
 		}
-		/*
-		 * Here there can be other concurrent MADV_DONTNEED or
-		 * trans huge page faults running, and if the pmd is
-		 * none or trans huge it can change under us. This is
-		 * because MADV_DONTNEED holds the mmap_sem in read
-		 * mode.
-		 */
-		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
-			goto next;
+
+		if (pmd_none_or_clear_bad(pmd))
+			continue;
+		spin_unlock(ptl);
 		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
-next:
 		cond_resched();
+		spin_lock(ptl);
 	} while (pmd++, addr = next, addr != end);
+	spin_unlock(ptl);
 
 	return addr;
 }
-- 
2.11.0

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

#1574351 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On February 06, 2017 12:13 AM Zi Yan wrote: 
> 
> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>  				struct zap_details *details)
>  {
>  	pmd_t *pmd;
> +	spinlock_t *ptl;
>  	unsigned long next;
> 
>  	pmd = pmd_offset(pud, addr);
> +	ptl = pmd_lock(vma->vm_mm, pmd);
>  	do {
>  		next = pmd_addr_end(addr, end);
>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>  			if (next - addr != HPAGE_PMD_SIZE) {
>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
> -				goto next;
> +				__split_huge_pmd_locked(vma, pmd, addr, false);
> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> +				continue;
>  			/* fall through */
>  		}
> -		/*
> -		 * Here there can be other concurrent MADV_DONTNEED or
> -		 * trans huge page faults running, and if the pmd is
> -		 * none or trans huge it can change under us. This is
> -		 * because MADV_DONTNEED holds the mmap_sem in read
> -		 * mode.
> -		 */
> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
> -			goto next;
> +
> +		if (pmd_none_or_clear_bad(pmd))
> +			continue;
> +		spin_unlock(ptl);
>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> -next:
>  		cond_resched();
> +		spin_lock(ptl);
>  	} while (pmd++, addr = next, addr != end);

spin_lock() is appointed to the bench of pmd_lock().

> +	spin_unlock(ptl);
> 
>  	return addr;
>  }

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

#1574427 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> This can cause pmd_protnone entry not being freed.
> 
> Because there are two steps in changing a pmd entry to a pmd_protnone
> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> the pmd_none entry is changed into a pmd_protnone entry.
> The racy check, even with barrier, might only see the pmd_none entry
> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
> 
> Later, in free_pmd_range(), pmd_none_or_clear() will see the
> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
> since the pmd_protnone entry is not properly freed, the corresponding
> deposited pte page table is not freed either.
> 
> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> 
> This patch relies on __split_huge_pmd_locked() and
> __zap_huge_pmd_locked().
> 
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> ---
>  mm/memory.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 3929b015faf7..7cfdd5208ef5 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>  				struct zap_details *details)
>  {
>  	pmd_t *pmd;
> +	spinlock_t *ptl;
>  	unsigned long next;
>  
>  	pmd = pmd_offset(pud, addr);
> +	ptl = pmd_lock(vma->vm_mm, pmd);

If USE_SPLIT_PMD_PTLOCKS is true, pmd_lock() returns different ptl for
each pmd. The following code runs over pmds within [addr, end) with
a single ptl (of the first pmd,) so I suspect this locking really works.
Maybe pmd_lock() should be called inside while loop?

Thanks,
Naoya Horiguchi

>  	do {
>  		next = pmd_addr_end(addr, end);
>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>  			if (next - addr != HPAGE_PMD_SIZE) {
>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
> -				goto next;
> +				__split_huge_pmd_locked(vma, pmd, addr, false);
> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> +				continue;
>  			/* fall through */
>  		}
> -		/*
> -		 * Here there can be other concurrent MADV_DONTNEED or
> -		 * trans huge page faults running, and if the pmd is
> -		 * none or trans huge it can change under us. This is
> -		 * because MADV_DONTNEED holds the mmap_sem in read
> -		 * mode.
> -		 */
> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
> -			goto next;
> +
> +		if (pmd_none_or_clear_bad(pmd))
> +			continue;
> +		spin_unlock(ptl);
>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> -next:
>  		cond_resched();
> +		spin_lock(ptl);
>  	} while (pmd++, addr = next, addr != end);
> +	spin_unlock(ptl);
>  
>  	return addr;
>  }
> -- 
> 2.11.0
> 

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

#1574666 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

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

On 6 Feb 2017, at 1:43, Naoya Horiguchi wrote:

> On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>>
>> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
>> This can cause pmd_protnone entry not being freed.
>>
>> Because there are two steps in changing a pmd entry to a pmd_protnone
>> entry. First, the pmd entry is cleared to a pmd_none entry, then,
>> the pmd_none entry is changed into a pmd_protnone entry.
>> The racy check, even with barrier, might only see the pmd_none entry
>> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
>>
>> Later, in free_pmd_range(), pmd_none_or_clear() will see the
>> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
>> since the pmd_protnone entry is not properly freed, the corresponding
>> deposited pte page table is not freed either.
>>
>> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
>>
>> This patch relies on __split_huge_pmd_locked() and
>> __zap_huge_pmd_locked().
>>
>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> ---
>>  mm/memory.c | 24 +++++++++++-------------
>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 3929b015faf7..7cfdd5208ef5 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>>  				struct zap_details *details)
>>  {
>>  	pmd_t *pmd;
>> +	spinlock_t *ptl;
>>  	unsigned long next;
>>
>>  	pmd = pmd_offset(pud, addr);
>> +	ptl = pmd_lock(vma->vm_mm, pmd);
>
> If USE_SPLIT_PMD_PTLOCKS is true, pmd_lock() returns different ptl for
> each pmd. The following code runs over pmds within [addr, end) with
> a single ptl (of the first pmd,) so I suspect this locking really works.
> Maybe pmd_lock() should be called inside while loop?

According to include/linux/mm.h, pmd_lockptr() first gets the page the pmd is in,
using mask = ~(PTRS_PER_PMD * sizeof(pmd_t) -1) = 0xfffffffffffff000 and virt_to_page().
Then, ptlock_ptr() gets spinlock_t either from page->ptl (split case) or
mm->page_table_lock (not split case).

It seems to me that all PMDs in one page table page share a single spinlock. Let me know
if I misunderstand any code.

But your suggestion can avoid holding the pmd lock for long without cond_sched(),
I can move the spinlock inside the loop.

Thanks.

diff --git a/mm/memory.c b/mm/memory.c
index 5299b261c4b4..ff61d45eaea7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1260,31 +1260,34 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
                                struct zap_details *details)
 {
        pmd_t *pmd;
-       spinlock_t *ptl;
+       spinlock_t *ptl = NULL;
        unsigned long next;

        pmd = pmd_offset(pud, addr);
-       ptl = pmd_lock(vma->vm_mm, pmd);
        do {
+               ptl = pmd_lock(vma->vm_mm, pmd);
                next = pmd_addr_end(addr, end);
                if (is_swap_pmd(*pmd) || pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
                        if (next - addr != HPAGE_PMD_SIZE) {
                                VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
                                    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
                                __split_huge_pmd_locked(vma, pmd, addr, false);
-                       } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
-                               continue;
+                       } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr)) {
+                               spin_unlock(ptl);
+                               goto next;
+                       }
                        /* fall through */
                }

-               if (pmd_none_or_clear_bad(pmd))
-                       continue;
+               if (pmd_none_or_clear_bad(pmd)) {
+                       spin_unlock(ptl);
+                       goto next;
+               }
                spin_unlock(ptl);
                next = zap_pte_range(tlb, vma, pmd, addr, next, details);
+next:
                cond_resched();
-               spin_lock(ptl);
        } while (pmd++, addr = next, addr != end);
-       spin_unlock(ptl);

        return addr;
 }


>
> Thanks,
> Naoya Horiguchi
>
>>  	do {
>>  		next = pmd_addr_end(addr, end);
>>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>>  			if (next - addr != HPAGE_PMD_SIZE) {
>>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
>> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
>> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
>> -				goto next;
>> +				__split_huge_pmd_locked(vma, pmd, addr, false);
>> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
>> +				continue;
>>  			/* fall through */
>>  		}
>> -		/*
>> -		 * Here there can be other concurrent MADV_DONTNEED or
>> -		 * trans huge page faults running, and if the pmd is
>> -		 * none or trans huge it can change under us. This is
>> -		 * because MADV_DONTNEED holds the mmap_sem in read
>> -		 * mode.
>> -		 */
>> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
>> -			goto next;
>> +
>> +		if (pmd_none_or_clear_bad(pmd))
>> +			continue;
>> +		spin_unlock(ptl);
>>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
>> -next:
>>  		cond_resched();
>> +		spin_lock(ptl);
>>  	} while (pmd++, addr = next, addr != end);
>> +	spin_unlock(ptl);
>>
>>  	return addr;
>>  }
>> -- 
>> 2.11.0
>>


--
Best Regards
Yan Zi

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

#1575248 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Mon, Feb 06, 2017 at 07:02:41AM -0600, Zi Yan wrote:
> On 6 Feb 2017, at 1:43, Naoya Horiguchi wrote:
> 
> > On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> >> From: Zi Yan <ziy@nvidia.com>
> >>
> >> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> >> This can cause pmd_protnone entry not being freed.
> >>
> >> Because there are two steps in changing a pmd entry to a pmd_protnone
> >> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> >> the pmd_none entry is changed into a pmd_protnone entry.
> >> The racy check, even with barrier, might only see the pmd_none entry
> >> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
> >>
> >> Later, in free_pmd_range(), pmd_none_or_clear() will see the
> >> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
> >> since the pmd_protnone entry is not properly freed, the corresponding
> >> deposited pte page table is not freed either.
> >>
> >> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> >>
> >> This patch relies on __split_huge_pmd_locked() and
> >> __zap_huge_pmd_locked().
> >>
> >> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> >> ---
> >>  mm/memory.c | 24 +++++++++++-------------
> >>  1 file changed, 11 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 3929b015faf7..7cfdd5208ef5 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
> >>  				struct zap_details *details)
> >>  {
> >>  	pmd_t *pmd;
> >> +	spinlock_t *ptl;
> >>  	unsigned long next;
> >>
> >>  	pmd = pmd_offset(pud, addr);
> >> +	ptl = pmd_lock(vma->vm_mm, pmd);
> >
> > If USE_SPLIT_PMD_PTLOCKS is true, pmd_lock() returns different ptl for
> > each pmd. The following code runs over pmds within [addr, end) with
> > a single ptl (of the first pmd,) so I suspect this locking really works.
> > Maybe pmd_lock() should be called inside while loop?
> 
> According to include/linux/mm.h, pmd_lockptr() first gets the page the pmd is in,
> using mask = ~(PTRS_PER_PMD * sizeof(pmd_t) -1) = 0xfffffffffffff000 and virt_to_page().
> Then, ptlock_ptr() gets spinlock_t either from page->ptl (split case) or
> mm->page_table_lock (not split case).
> 
> It seems to me that all PMDs in one page table page share a single spinlock. Let me know
> if I misunderstand any code.

Thanks for clarification, it was my misunderstanding.

Naoya

> 
> But your suggestion can avoid holding the pmd lock for long without cond_sched(),
> I can move the spinlock inside the loop.
> 
> Thanks.
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 5299b261c4b4..ff61d45eaea7 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1260,31 +1260,34 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>                                 struct zap_details *details)
>  {
>         pmd_t *pmd;
> -       spinlock_t *ptl;
> +       spinlock_t *ptl = NULL;
>         unsigned long next;
> 
>         pmd = pmd_offset(pud, addr);
> -       ptl = pmd_lock(vma->vm_mm, pmd);
>         do {
> +               ptl = pmd_lock(vma->vm_mm, pmd);
>                 next = pmd_addr_end(addr, end);
>                 if (is_swap_pmd(*pmd) || pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>                         if (next - addr != HPAGE_PMD_SIZE) {
>                                 VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>                                     !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
>                                 __split_huge_pmd_locked(vma, pmd, addr, false);
> -                       } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> -                               continue;
> +                       } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr)) {
> +                               spin_unlock(ptl);
> +                               goto next;
> +                       }
>                         /* fall through */
>                 }
> 
> -               if (pmd_none_or_clear_bad(pmd))
> -                       continue;
> +               if (pmd_none_or_clear_bad(pmd)) {
> +                       spin_unlock(ptl);
> +                       goto next;
> +               }
>                 spin_unlock(ptl);
>                 next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> +next:
>                 cond_resched();
> -               spin_lock(ptl);
>         } while (pmd++, addr = next, addr != end);
> -       spin_unlock(ptl);
> 
>         return addr;
>  }
> 
> 
> >
> > Thanks,
> > Naoya Horiguchi
> >
> >>  	do {
> >>  		next = pmd_addr_end(addr, end);
> >>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
> >>  			if (next - addr != HPAGE_PMD_SIZE) {
> >>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
> >>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> >> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
> >> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
> >> -				goto next;
> >> +				__split_huge_pmd_locked(vma, pmd, addr, false);
> >> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> >> +				continue;
> >>  			/* fall through */
> >>  		}
> >> -		/*
> >> -		 * Here there can be other concurrent MADV_DONTNEED or
> >> -		 * trans huge page faults running, and if the pmd is
> >> -		 * none or trans huge it can change under us. This is
> >> -		 * because MADV_DONTNEED holds the mmap_sem in read
> >> -		 * mode.
> >> -		 */
> >> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
> >> -			goto next;
> >> +
> >> +		if (pmd_none_or_clear_bad(pmd))
> >> +			continue;
> >> +		spin_unlock(ptl);
> >>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> >> -next:
> >>  		cond_resched();
> >> +		spin_lock(ptl);
> >>  	} while (pmd++, addr = next, addr != end);
> >> +	spin_unlock(ptl);
> >>
> >>  	return addr;
> >>  }
> >> -- 
> >> 2.11.0
> >>
> 
> 
> --
> Best Regards
> Yan Zi

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

#1574937 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> This can cause pmd_protnone entry not being freed.
> 
> Because there are two steps in changing a pmd entry to a pmd_protnone
> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> the pmd_none entry is changed into a pmd_protnone entry.
> The racy check, even with barrier, might only see the pmd_none entry
> in zap_pmd_range(), thus, the mapping is neither split nor zapped.

That's definately a good catch.

But I don't agree with the solution. Taking pmd lock on each
zap_pmd_range() is a significant hit by scalability of the code path.
Yes, split ptl lock helps, but it would be nice to avoid the lock in first
place.

Can we fix change_huge_pmd() instead? Is there a reason why we cannot
setup the pmd_protnone() atomically?

Mel? Rik?

> 
> Later, in free_pmd_range(), pmd_none_or_clear() will see the
> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
> since the pmd_protnone entry is not properly freed, the corresponding
> deposited pte page table is not freed either.
> 
> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> 
> This patch relies on __split_huge_pmd_locked() and
> __zap_huge_pmd_locked().
> 
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> ---
>  mm/memory.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 3929b015faf7..7cfdd5208ef5 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>  				struct zap_details *details)
>  {
>  	pmd_t *pmd;
> +	spinlock_t *ptl;
>  	unsigned long next;
>  
>  	pmd = pmd_offset(pud, addr);
> +	ptl = pmd_lock(vma->vm_mm, pmd);
>  	do {
>  		next = pmd_addr_end(addr, end);
>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>  			if (next - addr != HPAGE_PMD_SIZE) {
>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
> -				goto next;
> +				__split_huge_pmd_locked(vma, pmd, addr, false);
> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> +				continue;
>  			/* fall through */
>  		}
> -		/*
> -		 * Here there can be other concurrent MADV_DONTNEED or
> -		 * trans huge page faults running, and if the pmd is
> -		 * none or trans huge it can change under us. This is
> -		 * because MADV_DONTNEED holds the mmap_sem in read
> -		 * mode.
> -		 */
> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
> -			goto next;
> +
> +		if (pmd_none_or_clear_bad(pmd))
> +			continue;
> +		spin_unlock(ptl);
>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> -next:
>  		cond_resched();
> +		spin_lock(ptl);
>  	} while (pmd++, addr = next, addr != end);
> +	spin_unlock(ptl);
>  
>  	return addr;
>  }
> -- 
> 2.11.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

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

#1574968 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

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

On 6 Feb 2017, at 10:07, Kirill A. Shutemov wrote:

> On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>>
>> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
>> This can cause pmd_protnone entry not being freed.
>>
>> Because there are two steps in changing a pmd entry to a pmd_protnone
>> entry. First, the pmd entry is cleared to a pmd_none entry, then,
>> the pmd_none entry is changed into a pmd_protnone entry.
>> The racy check, even with barrier, might only see the pmd_none entry
>> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
>
> That's definately a good catch.
>
> But I don't agree with the solution. Taking pmd lock on each
> zap_pmd_range() is a significant hit by scalability of the code path.
> Yes, split ptl lock helps, but it would be nice to avoid the lock in first
> place.
>
> Can we fix change_huge_pmd() instead? Is there a reason why we cannot
> setup the pmd_protnone() atomically?

If you want to setup the pmd_protnone() atomically, we need a new way of
changing pmds, like pmdp_huge_cmp_exchange_and_clear(). Otherwise, due to
the nature of racy check of pmd in zap_pmd_range(), it is impossible to
eliminate the chance of catching this bug if pmd_protnone() is setup
in two steps: first, clear it, second, set it.

However, if we use pmdp_huge_cmp_exchange_and_clear() to change pmds from now on,
instead of current two-step approach, it will eliminate the possibility of
using batched TLB shootdown optimization (introduced by Mel Gorman for base page swapping)
when THP is swappable in the future. Maybe other optimizations?

Why do you think holding pmd lock is bad? In zap_pte_range(), pte lock
is also held when each PTE is zapped.

BTW, I am following Naoya's suggestion and going to take pmd lock inside
the loop. So pmd lock is held when each pmd is being checked and it will be released
when the pmd entry is zapped, split, or pointed to a page table.
Does it still hurt much on performance?

Thanks.



>
> Mel? Rik?
>
>>
>> Later, in free_pmd_range(), pmd_none_or_clear() will see the
>> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
>> since the pmd_protnone entry is not properly freed, the corresponding
>> deposited pte page table is not freed either.
>>
>> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
>>
>> This patch relies on __split_huge_pmd_locked() and
>> __zap_huge_pmd_locked().
>>
>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> ---
>>  mm/memory.c | 24 +++++++++++-------------
>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 3929b015faf7..7cfdd5208ef5 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
>>  				struct zap_details *details)
>>  {
>>  	pmd_t *pmd;
>> +	spinlock_t *ptl;
>>  	unsigned long next;
>>
>>  	pmd = pmd_offset(pud, addr);
>> +	ptl = pmd_lock(vma->vm_mm, pmd);
>>  	do {
>>  		next = pmd_addr_end(addr, end);
>>  		if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
>>  			if (next - addr != HPAGE_PMD_SIZE) {
>>  				VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
>>  				    !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
>> -				__split_huge_pmd(vma, pmd, addr, false, NULL);
>> -			} else if (zap_huge_pmd(tlb, vma, pmd, addr))
>> -				goto next;
>> +				__split_huge_pmd_locked(vma, pmd, addr, false);
>> +			} else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
>> +				continue;
>>  			/* fall through */
>>  		}
>> -		/*
>> -		 * Here there can be other concurrent MADV_DONTNEED or
>> -		 * trans huge page faults running, and if the pmd is
>> -		 * none or trans huge it can change under us. This is
>> -		 * because MADV_DONTNEED holds the mmap_sem in read
>> -		 * mode.
>> -		 */
>> -		if (pmd_none_or_trans_huge_or_clear_bad(pmd))
>> -			goto next;
>> +
>> +		if (pmd_none_or_clear_bad(pmd))
>> +			continue;
>> +		spin_unlock(ptl);
>>  		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
>> -next:
>>  		cond_resched();
>> +		spin_lock(ptl);
>>  	} while (pmd++, addr = next, addr != end);
>> +	spin_unlock(ptl);
>>
>>  	return addr;
>>  }
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
> -- 
>  Kirill A. Shutemov


--
Best Regards
Yan Zi

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

#1575024 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Mon, Feb 06, 2017 at 10:32:10AM -0600, Zi Yan wrote:
> On 6 Feb 2017, at 10:07, Kirill A. Shutemov wrote:
> 
> > On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> >> From: Zi Yan <ziy@nvidia.com>
> >>
> >> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> >> This can cause pmd_protnone entry not being freed.
> >>
> >> Because there are two steps in changing a pmd entry to a pmd_protnone
> >> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> >> the pmd_none entry is changed into a pmd_protnone entry.
> >> The racy check, even with barrier, might only see the pmd_none entry
> >> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
> >
> > That's definately a good catch.
> >
> > But I don't agree with the solution. Taking pmd lock on each
> > zap_pmd_range() is a significant hit by scalability of the code path.
> > Yes, split ptl lock helps, but it would be nice to avoid the lock in first
> > place.
> >
> > Can we fix change_huge_pmd() instead? Is there a reason why we cannot
> > setup the pmd_protnone() atomically?
> 
> If you want to setup the pmd_protnone() atomically, we need a new way of
> changing pmds, like pmdp_huge_cmp_exchange_and_clear(). Otherwise, due to
> the nature of racy check of pmd in zap_pmd_range(), it is impossible to
> eliminate the chance of catching this bug if pmd_protnone() is setup
> in two steps: first, clear it, second, set it.
> 
> However, if we use pmdp_huge_cmp_exchange_and_clear() to change pmds from now on,
> instead of current two-step approach, it will eliminate the possibility of
> using batched TLB shootdown optimization (introduced by Mel Gorman for base page swapping)
> when THP is swappable in the future. Maybe other optimizations?

I'll think about this more.

> Why do you think holding pmd lock is bad?

Each additional atomic operation in fast-path hurts scalability.
Cost of atomic operations rises fast as machine gets bigger.

> In zap_pte_range(), pte lock is also held when each PTE is zapped.

It's necessary evil for pte. Not so much for pmd so far.

> BTW, I am following Naoya's suggestion and going to take pmd lock inside
> the loop. So pmd lock is held when each pmd is being checked and it will be released
> when the pmd entry is zapped, split, or pointed to a page table.
> Does it still hurt much on performance?

Naoya's suggestion is not correct: pmd_lock() can be different not for
each pmd entry, but for each pmd table. So taking it outside of the loop
is correct.


-- 
 Kirill A. Shutemov

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

#1575756 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>> 
>> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
>> This can cause pmd_protnone entry not being freed.
>> 
>> Because there are two steps in changing a pmd entry to a pmd_protnone
>> entry. First, the pmd entry is cleared to a pmd_none entry, then,
>> the pmd_none entry is changed into a pmd_protnone entry.
>> The racy check, even with barrier, might only see the pmd_none entry
>> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
>
> That's definately a good catch.
>
> But I don't agree with the solution. Taking pmd lock on each
> zap_pmd_range() is a significant hit by scalability of the code path.
> Yes, split ptl lock helps, but it would be nice to avoid the lock in first
> place.
>
> Can we fix change_huge_pmd() instead? Is there a reason why we cannot
> setup the pmd_protnone() atomically?
>
> Mel? Rik?
>

I am also trying to fixup the usage of set_pte_at on ptes that are
valid/present (that this autonuma ptes). I guess what we are missing is a
variant of pte update routines that can atomically update a pte without
clearing it and that also doesn't do a tlb flush ?

-aneesh

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

#1575766 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> This can cause pmd_protnone entry not being freed.
> 
> Because there are two steps in changing a pmd entry to a pmd_protnone
> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> the pmd_none entry is changed into a pmd_protnone entry.
> The racy check, even with barrier, might only see the pmd_none entry
> in zap_pmd_range(), thus, the mapping is neither split nor zapped.

Okay, this only can happen to MADV_DONTNEED as we hold
down_write(mmap_sem) for the rest of zap_pmd_range() and whoever modifies
page tables has to hold at least down_read(mmap_sem) or exclude parallel
modification in other ways.

See 1a5a9906d4e8 ("mm: thp: fix pmd_bad() triggering in code paths holding
mmap_sem read mode") for more details.

+Andrea.

> Later, in free_pmd_range(), pmd_none_or_clear() will see the
> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
> since the pmd_protnone entry is not properly freed, the corresponding
> deposited pte page table is not freed either.

free_pmd_range() should be fine: we only free page tables after vmas gone
(under down_write(mmap_sem() in exit_mmap() and unmap_region()) or after
pagetables moved (under down_write(mmap_sem) in shift_arg_pages()).

> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.

The problem is that numabalancing calls change_huge_pmd() under
down_read(mmap_sem), not down_write(mmap_sem) as the rest of users do.
It makes numabalancing the only code path beyond page fault that can turn
pmd_none() into pmd_trans_huge() under down_read(mmap_sem).

This can lead to race when MADV_DONTNEED miss THP. That's not critical for
pagefault vs. MADV_DONTNEED race as we will end up with clear page in that
case. Not so much for change_huge_pmd().

Looks like we need pmdp_modify() or something to modify protection bits
inplace, without clearing pmd.

Not sure how to get crash scenario.

BTW, Zi, have you observed the crash? Or is it based on code inspection?
Any backtraces?

Ouch! madvise_free_huge_pmd() is broken too. We shouldn't clear pmd in the
middle of it as we only hold down_read(mmap_sem). I guess we need a helper
to clear both access and dirty bits.
Minchan, could you look into it?

-- 
 Kirill A. Shutemov

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

#1575859 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Tue, Feb 07, 2017 at 09:11:05AM -0600, Zi Yan wrote:
> >> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> > 
> > The problem is that numabalancing calls change_huge_pmd() under
> > down_read(mmap_sem), not down_write(mmap_sem) as the rest of users do.
> > It makes numabalancing the only code path beyond page fault that can turn
> > pmd_none() into pmd_trans_huge() under down_read(mmap_sem).
> > 
> > This can lead to race when MADV_DONTNEED miss THP. That's not critical for
> > pagefault vs. MADV_DONTNEED race as we will end up with clear page in that
> > case. Not so much for change_huge_pmd().
> > 
> > Looks like we need pmdp_modify() or something to modify protection bits
> > inplace, without clearing pmd.
> > 
> > Not sure how to get crash scenario.
> > 
> > BTW, Zi, have you observed the crash? Or is it based on code inspection?
> > Any backtraces?
> 
> The problem should be very rare in the upstream kernel. I discover the
> problem in my customized kernel which does very frequent page migration
> and uses numa_protnone.
> 
> The crash scenario I guess is like:
> 1. A huge page pmd entry is in the middle of being changed into either a
> pmd_protnone or a pmd_migration_entry. It is cleared to pmd_none.
> 
> 2. At the same time, the application frees the vma this page belongs to.

Em... no.

This shouldn't be possible: your 1. must be done under down_read(mmap_sem).
And we only be able to remove vma under down_write(mmap_sem), so the
scenario should be excluded.

What do I miss?

> 3. zap_pmd_range() only see pmd_none in
> "if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd))",
> it might catch pmd_protnone in
> "if (pmd_none_or_trans_huge_or_clear_bad(pmd))". But nothing is done for
> it. So the deposited PTE page table page associated with the huge pmd
> entry is not withdrawn.
> 
> 4. free_pmd_range() calls pmd_free_tlb() and in pgtable_pmd_page_dtor(),
> VM_BUG_ON_PAGE(page->pmd_huge_pte, page) is triggered.
> 
> The crash log (you will see a pmd_migration_entry is regarded as bad
> pmd, which should not be. I also saw pmd_protnone before.):
> 
> [ 1945.978677] mm/pgtable-generic.c:33: bad pmd
> ffff8f07b13c1b90(0000004fed803c00)
>                  ^^^^^^^^^^^^^^^^ a pmd migration entry
> 
> [ 1946.964974] page:fffffd1dd0c4f040 count:1 mapcount:-511 mapping:
>      (null) index:0x0
> [ 1946.974265] flags: 0x6ffff0000000000()
> [ 1946.978486] raw: 06ffff0000000000 0000000000000000 0000000000000000
> 00000001fffffe00
> [ 1946.987202] raw: dead000000000100 fffffd1dd0c45c80 ffff8f07aa38e340
> ffff8efdca466678
> [ 1946.995927] page dumped because: VM_BUG_ON_PAGE(page->pmd_huge_pte)
> [ 1947.002984] page->mem_cgroup:ffff8efdca466678
> [ 1947.007927] ------------[ cut here ]------------
> [ 1947.013123] kernel BUG at ./include/linux/mm.h:1733!
> [ 1947.018706] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
> [ 1947.024774] Modules linked in: ipt_MASQUERADE nf_nat_masquerade_ipv4
> iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4
> xt_conntrack nf_conntrack intel_rapl sb_edac edac_corei
> [ 1947.077814] CPU: 19 PID: 3303 Comm: python Not tainted
> 4.10.0-rc5-page-migration+ #283
> [ 1947.086721] Hardware name: Dell Inc. PowerEdge R530/0HFG24, BIOS
> 1.5.4 10/05/2015
> [ 1947.095140] task: ffff8f07a5870040 task.stack: ffffc37d64adc000
> [ 1947.101796] RIP: 0010:___pmd_free_tlb+0x83/0x90
> [ 1947.106890] RSP: 0018:ffffc37d64adfce8 EFLAGS: 00010282
> [ 1947.112762] RAX: 0000000000000021 RBX: ffffc37d64adfe10 RCX:
> 0000000000000000
> [ 1947.120770] RDX: 0000000000000000 RSI: ffff8f07c224dea8 RDI:
> ffff8f07c224dea8
> [ 1947.128809] RBP: ffffc37d64adfcf8 R08: 0000000000000001 R09:
> 0000000000000000
> [ 1947.136818] R10: 000000000000000f R11: 0000000000000001 R12:
> fffffd1dd0c4f040
> [ 1947.144825] R13: 00007fae2d7fd000 R14: ffff8f07b13c1b60 R15:
> ffffc37d64adfe10
> [ 1947.152832] FS:  00007fafbcce6700(0000) GS:ffff8f07c2240000(0000)
> knlGS:0000000000000000
> [ 1947.161934] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1947.168380] CR2: 00007fafeb851188 CR3: 0000001432184000 CR4:
> 00000000001406e0
> [ 1947.176393] Call Trace:
> [ 1947.179160]  free_pgd_range+0x487/0x5d0
> [ 1947.183476]  free_pgtables+0xc4/0x120
> [ 1947.187593]  unmap_region+0xe1/0x130
> [ 1947.191620]  do_munmap+0x273/0x400
> [ 1947.195452]  SyS_munmap+0x53/0x70
> [ 1947.199190]  entry_SYSCALL_64_fastpath+0x23/0xc6
> [ 1947.204382] RIP: 0033:0x7fafeb59d387
> [ 1947.208406] RSP: 002b:00007fafbcce5358 EFLAGS: 00000207 ORIG_RAX:
> 000000000000000b
> [ 1947.216924] RAX: ffffffffffffffda RBX: 00007faf2c0b6780 RCX:
> 00007fafeb59d387
> [ 1947.224933] RDX: 00007fae247fc030 RSI: 0000000009001000 RDI:
> 00007fae247fc000
> [ 1947.232940] RBP: 00007fafbcce5390 R08: 00007faf48f9ed00 R09:
> 0000000000000100
> [ 1947.240947] R10: 0000000000000020 R11: 0000000000000207 R12:
> 0000000002cf1fc0
> [ 1947.248955] R13: 0000000002cf1fc0 R14: 000000000343d530 R15:
> 00007fafbcce5810
> [ 1947.256965] Code: 4c 89 e6 48 89 df e8 0d b5 1a 00 84 c0 74 08 48 89
> df e8 91 b4 1a 00 5b 41 5c 5d c3 48 c7 c6 d8 73 c7 b8 4c 89 e7 e8 dd 7b
> 1a 00 <0f> 0b 48 8b 3d 34 80 d9 00 eb 99 66 90
> [ 1947.278200] RIP: ___pmd_free_tlb+0x83/0x90 RSP: ffffc37d64adfce8
> [ 1947.285688] ---[ end trace 7864a23976d71e0a ]---
> 
> -- 
> Best Regards,
> Yan Zi
> 



-- 
 Kirill A. Shutemov

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

#1575925 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Tue, Feb 07, 2017 at 11:14:56AM -0600, Zi Yan wrote:
> 
> 
> Kirill A. Shutemov wrote:
> > On Tue, Feb 07, 2017 at 09:11:05AM -0600, Zi Yan wrote:
> >>>> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> >>> The problem is that numabalancing calls change_huge_pmd() under
> >>> down_read(mmap_sem), not down_write(mmap_sem) as the rest of users do.
> >>> It makes numabalancing the only code path beyond page fault that can turn
> >>> pmd_none() into pmd_trans_huge() under down_read(mmap_sem).
> >>>
> >>> This can lead to race when MADV_DONTNEED miss THP. That's not critical for
> >>> pagefault vs. MADV_DONTNEED race as we will end up with clear page in that
> >>> case. Not so much for change_huge_pmd().
> >>>
> >>> Looks like we need pmdp_modify() or something to modify protection bits
> >>> inplace, without clearing pmd.
> >>>
> >>> Not sure how to get crash scenario.
> >>>
> >>> BTW, Zi, have you observed the crash? Or is it based on code inspection?
> >>> Any backtraces?
> >> The problem should be very rare in the upstream kernel. I discover the
> >> problem in my customized kernel which does very frequent page migration
> >> and uses numa_protnone.
> >>
> >> The crash scenario I guess is like:
> >> 1. A huge page pmd entry is in the middle of being changed into either a
> >> pmd_protnone or a pmd_migration_entry. It is cleared to pmd_none.
> >>
> >> 2. At the same time, the application frees the vma this page belongs to.
> > 
> > Em... no.
> > 
> > This shouldn't be possible: your 1. must be done under down_read(mmap_sem).
> > And we only be able to remove vma under down_write(mmap_sem), so the
> > scenario should be excluded.
> > 
> > What do I miss?
> 
> You are right. This problem will not happen in the upstream kernel.
> 
> The problem comes from my customized kernel, where I migrate pages away
> instead of reclaiming them when memory is under pressure. I did not take
> any mmap_sem when I migrate pages. So I got this error.
> 
> It is a false alarm. Sorry about that. Thanks for clarifying the problem.

I think there's still a race between MADV_DONTNEED and
change_huge_pmd(.prot_numa=1) resulting in skipping THP by
zap_pmd_range(). It need to be addressed.

And MADV_FREE requires a fix.

So, minus one non-bug, plus two bugs. 

-- 
 Kirill A. Shutemov

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

#1579619 — Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range()

On Sun, Feb 12, 2017 at 06:25:09PM -0600, Zi Yan wrote:
> Hi Kirill,
> 
> >>>> The crash scenario I guess is like:
> >>>> 1. A huge page pmd entry is in the middle of being changed into either a
> >>>> pmd_protnone or a pmd_migration_entry. It is cleared to pmd_none.
> >>>>
> >>>> 2. At the same time, the application frees the vma this page belongs to.
> >>>
> >>> Em... no.
> >>>
> >>> This shouldn't be possible: your 1. must be done under down_read(mmap_sem).
> >>> And we only be able to remove vma under down_write(mmap_sem), so the
> >>> scenario should be excluded.
> >>>
> >>> What do I miss?
> >>
> >> You are right. This problem will not happen in the upstream kernel.
> >>
> >> The problem comes from my customized kernel, where I migrate pages away
> >> instead of reclaiming them when memory is under pressure. I did not take
> >> any mmap_sem when I migrate pages. So I got this error.
> >>
> >> It is a false alarm. Sorry about that. Thanks for clarifying the problem.
> >
> > I think there's still a race between MADV_DONTNEED and
> > change_huge_pmd(.prot_numa=1) resulting in skipping THP by
> > zap_pmd_range(). It need to be addressed.
> >
> > And MADV_FREE requires a fix.
> >
> > So, minus one non-bug, plus two bugs.
> >
> 
> You said a huge page pmd entry needs to be changed under down_read(mmap_sem).
> It is only true for huge pages, right?

mmap_sem is a way to make sure that the VMA will not go away under you.
Besides mmap_sem, anon_vma_lock/i_mmap_lock can be used for this.

> Since in mm/compaction.c, the kernel does not down_read(mmap_sem) during memory
> compaction. Namely, base page migrations do not hold down_read(mmap_sem),
> so in zap_pte_range(), the kernel needs to hold PTE page table locks.
> Am I right about this?
> 
> If yes. IMHO, ultimately, when we need to compact 2MB pages to form 1GB pages,
> in zap_pmd_range(), pmd locks have to be taken to make that kind of compactions
> possible.
> 
> Do you agree?

I *think* we can get away with speculative (without ptl) check in
zap_pmd_range() if we make page fault the only place that can turn
pmd_none() into something else.

It means all other sides that change pmd must not clear it intermittently
during pmd change, unless run under down_write(mmap_sem).

I found two such problematic places in kernel:

 - change_huge_pmd(.prot_numa=1);

 - madvise_free_huge_pmd();

Both clear pmd before setting up a modified version. Both under
down_read(mmap_sem).

The migration path also would need to establish migration pmd atomically
to make this work.

Once all these cases will be fixed, zap_pmd_range() would only be able to
race with page fault if it called from MADV_DONTNEED.
This case is not a problem.

Andrea, does it sound reasonable to you?

-- 
 Kirill A. Shutemov

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web