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

Groups > linux.kernel > #1666780 > unrolled thread

[PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

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

  [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-06-15 17:00 +0200
    Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits kbuild test robot <lkp@intel.com> - 2017-06-16 00:00 +0200
    Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits kbuild test robot <lkp@intel.com> - 2017-06-16 01:10 +0200
    Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits Minchan Kim <minchan@kernel.org> - 2017-06-16 05:10 +0200
      Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-16 15:20 +0200
        Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits Minchan Kim <minchan@kernel.org> - 2017-06-16 16:00 +0200
          Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits Andrea Arcangeli <aarcange@redhat.com> - 2017-06-16 16:30 +0200
            Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits Minchan Kim <minchan@kernel.org> - 2017-06-16 17:00 +0200
              Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-19 16:10 +0200
    Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-16 13:40 +0200
      Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-06-16 15:30 +0200
        Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track  dirty/accessed bits "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-16 18:00 +0200

#1666780 — [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
to transfer dirty and accessed bits.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 fs/proc/task_mmu.c |  8 ++++----
 mm/huge_memory.c   | 29 ++++++++++++-----------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f0c8b33d99b1..f2fc1ef5bba2 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -906,13 +906,13 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
 static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
 		unsigned long addr, pmd_t *pmdp)
 {
-	pmd_t pmd = *pmdp;
+	pmd_t old, pmd = *pmdp;
 
 	/* See comment in change_huge_pmd() */
-	pmdp_invalidate(vma, addr, pmdp);
-	if (pmd_dirty(*pmdp))
+	old = pmdp_invalidate(vma, addr, pmdp);
+	if (pmd_dirty(old))
 		pmd = pmd_mkdirty(pmd);
-	if (pmd_young(*pmdp))
+	if (pmd_young(old))
 		pmd = pmd_mkyoung(pmd);
 
 	pmd = pmd_wrprotect(pmd);
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a84909cf20d3..0433e73531bf 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1777,17 +1777,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 	 * pmdp_invalidate() is required to make sure we don't miss
 	 * dirty/young flags set by hardware.
 	 */
-	entry = *pmd;
-	pmdp_invalidate(vma, addr, pmd);
-
-	/*
-	 * Recover dirty/young flags.  It relies on pmdp_invalidate to not
-	 * corrupt them.
-	 */
-	if (pmd_dirty(*pmd))
-		entry = pmd_mkdirty(entry);
-	if (pmd_young(*pmd))
-		entry = pmd_mkyoung(entry);
+	entry = pmdp_invalidate(vma, addr, pmd);
 
 	entry = pmd_modify(entry, newprot);
 	if (preserve_write)
@@ -1927,8 +1917,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	struct mm_struct *mm = vma->vm_mm;
 	struct page *page;
 	pgtable_t pgtable;
-	pmd_t _pmd;
-	bool young, write, dirty, soft_dirty;
+	pmd_t old, _pmd;
+	bool young, write, soft_dirty;
 	unsigned long addr;
 	int i;
 
@@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	page_ref_add(page, HPAGE_PMD_NR - 1);
 	write = pmd_write(*pmd);
 	young = pmd_young(*pmd);
-	dirty = pmd_dirty(*pmd);
 	soft_dirty = pmd_soft_dirty(*pmd);
 
 	pmdp_huge_split_prepare(vma, haddr, pmd);
@@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 			if (soft_dirty)
 				entry = pte_mksoft_dirty(entry);
 		}
-		if (dirty)
-			SetPageDirty(page + i);
 		pte = pte_offset_map(&_pmd, addr);
 		BUG_ON(!pte_none(*pte));
 		set_pte_at(mm, addr, pte, entry);
@@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	 * and finally we write the non-huge version of the pmd entry with
 	 * pmd_populate.
 	 */
-	pmdp_invalidate(vma, haddr, pmd);
+	old = pmdp_invalidate(vma, haddr, pmd);
+
+	/*
+	 * Transfer dirty bit using value returned by pmd_invalidate() to be
+	 * sure we don't race with CPU that can set the bit under us.
+	 */
+	if (pmd_dirty(old))
+		SetPageDirty(page);
+
 	pmd_populate(mm, pmd, pgtable);
 
 	if (freeze) {
-- 
2.11.0

[toc] | [next] | [standalone]

#1667298 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

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

Hi Kirill,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.12-rc5 next-20170615]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-A-Shutemov/Do-not-loose-dirty-bit-on-THP-pages/20170616-030455
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: s390-default_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   fs/proc/task_mmu.c: In function 'clear_soft_dirty_pmd':
>> fs/proc/task_mmu.c:912:6: error: void value not ignored as it ought to be
     old = pmdp_invalidate(vma, addr, pmdp);
         ^

vim +912 fs/proc/task_mmu.c

   906	static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
   907			unsigned long addr, pmd_t *pmdp)
   908	{
   909		pmd_t old, pmd = *pmdp;
   910	
   911		/* See comment in change_huge_pmd() */
 > 912		old = pmdp_invalidate(vma, addr, pmdp);
   913		if (pmd_dirty(old))
   914			pmd = pmd_mkdirty(pmd);
   915		if (pmd_young(old))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1667326 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

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

Hi Kirill,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.12-rc5 next-20170615]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-A-Shutemov/Do-not-loose-dirty-bit-on-THP-pages/20170616-030455
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: s390-defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   mm/huge_memory.c: In function 'change_huge_pmd':
>> mm/huge_memory.c:1780:8: error: void value not ignored as it ought to be
     entry = pmdp_invalidate(vma, addr, pmd);
           ^
   mm/huge_memory.c: In function '__split_huge_pmd_locked':
   mm/huge_memory.c:2035:6: error: void value not ignored as it ought to be
     old = pmdp_invalidate(vma, haddr, pmd);
         ^

vim +1780 mm/huge_memory.c

  1774		 * The race makes MADV_DONTNEED miss the huge pmd and don't clear it
  1775		 * which may break userspace.
  1776		 *
  1777		 * pmdp_invalidate() is required to make sure we don't miss
  1778		 * dirty/young flags set by hardware.
  1779		 */
> 1780		entry = pmdp_invalidate(vma, addr, pmd);
  1781	
  1782		entry = pmd_modify(entry, newprot);
  1783		if (preserve_write)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1667391 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

Hello,

On Thu, Jun 15, 2017 at 05:52:24PM +0300, Kirill A. Shutemov wrote:
> This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
> to transfer dirty and accessed bits.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  fs/proc/task_mmu.c |  8 ++++----
>  mm/huge_memory.c   | 29 ++++++++++++-----------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f0c8b33d99b1..f2fc1ef5bba2 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -906,13 +906,13 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
>  static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
>  		unsigned long addr, pmd_t *pmdp)
>  {
> -	pmd_t pmd = *pmdp;
> +	pmd_t old, pmd = *pmdp;
>  
>  	/* See comment in change_huge_pmd() */
> -	pmdp_invalidate(vma, addr, pmdp);
> -	if (pmd_dirty(*pmdp))
> +	old = pmdp_invalidate(vma, addr, pmdp);
> +	if (pmd_dirty(old))
>  		pmd = pmd_mkdirty(pmd);
> -	if (pmd_young(*pmdp))
> +	if (pmd_young(old))
>  		pmd = pmd_mkyoung(pmd);
>  
>  	pmd = pmd_wrprotect(pmd);
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a84909cf20d3..0433e73531bf 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1777,17 +1777,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  	 * pmdp_invalidate() is required to make sure we don't miss
>  	 * dirty/young flags set by hardware.
>  	 */
> -	entry = *pmd;
> -	pmdp_invalidate(vma, addr, pmd);
> -
> -	/*
> -	 * Recover dirty/young flags.  It relies on pmdp_invalidate to not
> -	 * corrupt them.
> -	 */
> -	if (pmd_dirty(*pmd))
> -		entry = pmd_mkdirty(entry);
> -	if (pmd_young(*pmd))
> -		entry = pmd_mkyoung(entry);
> +	entry = pmdp_invalidate(vma, addr, pmd);
>  
>  	entry = pmd_modify(entry, newprot);
>  	if (preserve_write)
> @@ -1927,8 +1917,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	struct mm_struct *mm = vma->vm_mm;
>  	struct page *page;
>  	pgtable_t pgtable;
> -	pmd_t _pmd;
> -	bool young, write, dirty, soft_dirty;
> +	pmd_t old, _pmd;
> +	bool young, write, soft_dirty;
>  	unsigned long addr;
>  	int i;
>  
> @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	page_ref_add(page, HPAGE_PMD_NR - 1);
>  	write = pmd_write(*pmd);
>  	young = pmd_young(*pmd);
> -	dirty = pmd_dirty(*pmd);
>  	soft_dirty = pmd_soft_dirty(*pmd);
>  
>  	pmdp_huge_split_prepare(vma, haddr, pmd);
> @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			if (soft_dirty)
>  				entry = pte_mksoft_dirty(entry);
>  		}
> -		if (dirty)
> -			SetPageDirty(page + i);
>  		pte = pte_offset_map(&_pmd, addr);
>  		BUG_ON(!pte_none(*pte));
>  		set_pte_at(mm, addr, pte, entry);
> @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	 * and finally we write the non-huge version of the pmd entry with
>  	 * pmd_populate.
>  	 */
> -	pmdp_invalidate(vma, haddr, pmd);
> +	old = pmdp_invalidate(vma, haddr, pmd);
> +
> +	/*
> +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
> +	 * sure we don't race with CPU that can set the bit under us.
> +	 */
> +	if (pmd_dirty(old))
> +		SetPageDirty(page);
> +

When I see this, without this patch, MADV_FREE has been broken because
it can lose dirty bit by early checking. Right?
If so, isn't it a candidate for -stable?

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

#1667743 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

On Fri, Jun 16, 2017 at 12:02:50PM +0900, Minchan Kim wrote:
> Hello,
> 
> On Thu, Jun 15, 2017 at 05:52:24PM +0300, Kirill A. Shutemov wrote:
> > This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
> > to transfer dirty and accessed bits.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  fs/proc/task_mmu.c |  8 ++++----
> >  mm/huge_memory.c   | 29 ++++++++++++-----------------
> >  2 files changed, 16 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index f0c8b33d99b1..f2fc1ef5bba2 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -906,13 +906,13 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> >  static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> >  		unsigned long addr, pmd_t *pmdp)
> >  {
> > -	pmd_t pmd = *pmdp;
> > +	pmd_t old, pmd = *pmdp;
> >  
> >  	/* See comment in change_huge_pmd() */
> > -	pmdp_invalidate(vma, addr, pmdp);
> > -	if (pmd_dirty(*pmdp))
> > +	old = pmdp_invalidate(vma, addr, pmdp);
> > +	if (pmd_dirty(old))
> >  		pmd = pmd_mkdirty(pmd);
> > -	if (pmd_young(*pmdp))
> > +	if (pmd_young(old))
> >  		pmd = pmd_mkyoung(pmd);
> >  
> >  	pmd = pmd_wrprotect(pmd);
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index a84909cf20d3..0433e73531bf 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1777,17 +1777,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >  	 * pmdp_invalidate() is required to make sure we don't miss
> >  	 * dirty/young flags set by hardware.
> >  	 */
> > -	entry = *pmd;
> > -	pmdp_invalidate(vma, addr, pmd);
> > -
> > -	/*
> > -	 * Recover dirty/young flags.  It relies on pmdp_invalidate to not
> > -	 * corrupt them.
> > -	 */
> > -	if (pmd_dirty(*pmd))
> > -		entry = pmd_mkdirty(entry);
> > -	if (pmd_young(*pmd))
> > -		entry = pmd_mkyoung(entry);
> > +	entry = pmdp_invalidate(vma, addr, pmd);
> >  
> >  	entry = pmd_modify(entry, newprot);
> >  	if (preserve_write)
> > @@ -1927,8 +1917,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  	struct mm_struct *mm = vma->vm_mm;
> >  	struct page *page;
> >  	pgtable_t pgtable;
> > -	pmd_t _pmd;
> > -	bool young, write, dirty, soft_dirty;
> > +	pmd_t old, _pmd;
> > +	bool young, write, soft_dirty;
> >  	unsigned long addr;
> >  	int i;
> >  
> > @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  	page_ref_add(page, HPAGE_PMD_NR - 1);
> >  	write = pmd_write(*pmd);
> >  	young = pmd_young(*pmd);
> > -	dirty = pmd_dirty(*pmd);
> >  	soft_dirty = pmd_soft_dirty(*pmd);
> >  
> >  	pmdp_huge_split_prepare(vma, haddr, pmd);
> > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  			if (soft_dirty)
> >  				entry = pte_mksoft_dirty(entry);
> >  		}
> > -		if (dirty)
> > -			SetPageDirty(page + i);
> >  		pte = pte_offset_map(&_pmd, addr);
> >  		BUG_ON(!pte_none(*pte));
> >  		set_pte_at(mm, addr, pte, entry);
> > @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  	 * and finally we write the non-huge version of the pmd entry with
> >  	 * pmd_populate.
> >  	 */
> > -	pmdp_invalidate(vma, haddr, pmd);
> > +	old = pmdp_invalidate(vma, haddr, pmd);
> > +
> > +	/*
> > +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
> > +	 * sure we don't race with CPU that can set the bit under us.
> > +	 */
> > +	if (pmd_dirty(old))
> > +		SetPageDirty(page);
> > +
> 
> When I see this, without this patch, MADV_FREE has been broken because
> it can lose dirty bit by early checking. Right?
> If so, isn't it a candidate for -stable?

Actually, I don't see how MADV_FREE supposed to work: vmscan splits THP on
reclaim and split_huge_page() would set unconditionally, so MADV_FREE
seems no effect on THP.

Or have I missed anything?

-- 
 Kirill A. Shutemov

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

#1667764 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

On Fri, Jun 16, 2017 at 04:19:08PM +0300, Kirill A. Shutemov wrote:
> On Fri, Jun 16, 2017 at 12:02:50PM +0900, Minchan Kim wrote:
> > Hello,
> > 
> > On Thu, Jun 15, 2017 at 05:52:24PM +0300, Kirill A. Shutemov wrote:
> > > This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
> > > to transfer dirty and accessed bits.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > ---
> > >  fs/proc/task_mmu.c |  8 ++++----
> > >  mm/huge_memory.c   | 29 ++++++++++++-----------------
> > >  2 files changed, 16 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index f0c8b33d99b1..f2fc1ef5bba2 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -906,13 +906,13 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> > >  static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > >  		unsigned long addr, pmd_t *pmdp)
> > >  {
> > > -	pmd_t pmd = *pmdp;
> > > +	pmd_t old, pmd = *pmdp;
> > >  
> > >  	/* See comment in change_huge_pmd() */
> > > -	pmdp_invalidate(vma, addr, pmdp);
> > > -	if (pmd_dirty(*pmdp))
> > > +	old = pmdp_invalidate(vma, addr, pmdp);
> > > +	if (pmd_dirty(old))
> > >  		pmd = pmd_mkdirty(pmd);
> > > -	if (pmd_young(*pmdp))
> > > +	if (pmd_young(old))
> > >  		pmd = pmd_mkyoung(pmd);
> > >  
> > >  	pmd = pmd_wrprotect(pmd);
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index a84909cf20d3..0433e73531bf 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -1777,17 +1777,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> > >  	 * pmdp_invalidate() is required to make sure we don't miss
> > >  	 * dirty/young flags set by hardware.
> > >  	 */
> > > -	entry = *pmd;
> > > -	pmdp_invalidate(vma, addr, pmd);
> > > -
> > > -	/*
> > > -	 * Recover dirty/young flags.  It relies on pmdp_invalidate to not
> > > -	 * corrupt them.
> > > -	 */
> > > -	if (pmd_dirty(*pmd))
> > > -		entry = pmd_mkdirty(entry);
> > > -	if (pmd_young(*pmd))
> > > -		entry = pmd_mkyoung(entry);
> > > +	entry = pmdp_invalidate(vma, addr, pmd);
> > >  
> > >  	entry = pmd_modify(entry, newprot);
> > >  	if (preserve_write)
> > > @@ -1927,8 +1917,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > >  	struct mm_struct *mm = vma->vm_mm;
> > >  	struct page *page;
> > >  	pgtable_t pgtable;
> > > -	pmd_t _pmd;
> > > -	bool young, write, dirty, soft_dirty;
> > > +	pmd_t old, _pmd;
> > > +	bool young, write, soft_dirty;
> > >  	unsigned long addr;
> > >  	int i;
> > >  
> > > @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > >  	page_ref_add(page, HPAGE_PMD_NR - 1);
> > >  	write = pmd_write(*pmd);
> > >  	young = pmd_young(*pmd);
> > > -	dirty = pmd_dirty(*pmd);
> > >  	soft_dirty = pmd_soft_dirty(*pmd);
> > >  
> > >  	pmdp_huge_split_prepare(vma, haddr, pmd);
> > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > >  			if (soft_dirty)
> > >  				entry = pte_mksoft_dirty(entry);
> > >  		}
> > > -		if (dirty)
> > > -			SetPageDirty(page + i);
> > >  		pte = pte_offset_map(&_pmd, addr);
> > >  		BUG_ON(!pte_none(*pte));
> > >  		set_pte_at(mm, addr, pte, entry);
> > > @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > >  	 * and finally we write the non-huge version of the pmd entry with
> > >  	 * pmd_populate.
> > >  	 */
> > > -	pmdp_invalidate(vma, haddr, pmd);
> > > +	old = pmdp_invalidate(vma, haddr, pmd);
> > > +
> > > +	/*
> > > +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
> > > +	 * sure we don't race with CPU that can set the bit under us.
> > > +	 */
> > > +	if (pmd_dirty(old))
> > > +		SetPageDirty(page);
> > > +
> > 
> > When I see this, without this patch, MADV_FREE has been broken because
> > it can lose dirty bit by early checking. Right?
> > If so, isn't it a candidate for -stable?
> 
> Actually, I don't see how MADV_FREE supposed to work: vmscan splits THP on
> reclaim and split_huge_page() would set unconditionally, so MADV_FREE
> seems no effect on THP.

split_huge_page set PG_dirty to all subpages unconditionally?
If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot
that piece of code. What I found one is just __split_huge_page_tail
which set PG_dirty to subpage if head page is dirty. IOW, if the head
page is not dirty, tail page will be clean, too.
Could you point out what routine set PG_dirty to all subpages unconditionally?

Thanks.

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

#1667795 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

Hello Minchan,

On Fri, Jun 16, 2017 at 10:52:09PM +0900, Minchan Kim wrote:
> > > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > > >  			if (soft_dirty)
> > > >  				entry = pte_mksoft_dirty(entry);
> > > >  		}
> > > > -		if (dirty)
> > > > -			SetPageDirty(page + i);
> > > >  		pte = pte_offset_map(&_pmd, addr);
[..]
> 
> split_huge_page set PG_dirty to all subpages unconditionally?
> If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot
> that piece of code. What I found one is just __split_huge_page_tail
> which set PG_dirty to subpage if head page is dirty. IOW, if the head
> page is not dirty, tail page will be clean, too.
> Could you point out what routine set PG_dirty to all subpages unconditionally?

On a side note the snippet deleted above was useless, as long as
there's one left hugepmd to split, the physical page has to be still
compound and huge and as long as that's the case the tail pages
PG_dirty bit is meaningless (even if set, it's going to be clobbered
during the physical split).

In short PG_dirty is only meaningful in the head as long as it's
compound. The physical split in __split_huge_page_tail transfer the
head value to the tails like you mentioned, that's all as far as I can
tell.

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

#1667821 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

Hi Andrea,

On Fri, Jun 16, 2017 at 04:27:20PM +0200, Andrea Arcangeli wrote:
> Hello Minchan,
> 
> On Fri, Jun 16, 2017 at 10:52:09PM +0900, Minchan Kim wrote:
> > > > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > > > >  			if (soft_dirty)
> > > > >  				entry = pte_mksoft_dirty(entry);
> > > > >  		}
> > > > > -		if (dirty)
> > > > > -			SetPageDirty(page + i);
> > > > >  		pte = pte_offset_map(&_pmd, addr);
> [..]
> > 
> > split_huge_page set PG_dirty to all subpages unconditionally?
> > If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot
> > that piece of code. What I found one is just __split_huge_page_tail
> > which set PG_dirty to subpage if head page is dirty. IOW, if the head
> > page is not dirty, tail page will be clean, too.
> > Could you point out what routine set PG_dirty to all subpages unconditionally?
> 
> On a side note the snippet deleted above was useless, as long as
> there's one left hugepmd to split, the physical page has to be still
> compound and huge and as long as that's the case the tail pages
> PG_dirty bit is meaningless (even if set, it's going to be clobbered
> during the physical split).

I got it during reviewing this patch. That's why I didn't argue
this patch would break MADV_FREE by deleting routine which propagate
dirty to pte of subpages. However, although it's useless, I prefer
not removing the transfer of dirty bit. Because it would help MADV_FREE
users who want to use smaps to know how many of pages are not freeable
(i.e, dirtied) since MADV_FREE although it is not 100% correct.

> 
> In short PG_dirty is only meaningful in the head as long as it's
> compound. The physical split in __split_huge_page_tail transfer the
> head value to the tails like you mentioned, that's all as far as I can
> tell.

Thanks for the comment. Then, this patch is to fix MADV_FREE's bug
which has lost dirty bit by transferring dirty bit too early.

Thanks.

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

#1669092 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

On Fri, Jun 16, 2017 at 11:53:33PM +0900, Minchan Kim wrote:
> Hi Andrea,
> 
> On Fri, Jun 16, 2017 at 04:27:20PM +0200, Andrea Arcangeli wrote:
> > Hello Minchan,
> > 
> > On Fri, Jun 16, 2017 at 10:52:09PM +0900, Minchan Kim wrote:
> > > > > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> > > > > >  			if (soft_dirty)
> > > > > >  				entry = pte_mksoft_dirty(entry);
> > > > > >  		}
> > > > > > -		if (dirty)
> > > > > > -			SetPageDirty(page + i);
> > > > > >  		pte = pte_offset_map(&_pmd, addr);
> > [..]
> > > 
> > > split_huge_page set PG_dirty to all subpages unconditionally?
> > > If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot
> > > that piece of code. What I found one is just __split_huge_page_tail
> > > which set PG_dirty to subpage if head page is dirty. IOW, if the head
> > > page is not dirty, tail page will be clean, too.
> > > Could you point out what routine set PG_dirty to all subpages unconditionally?

When I wrote this code, I considered that we may want to track dirty
status on per-4k basis for file-backed THPs.

> > On a side note the snippet deleted above was useless, as long as
> > there's one left hugepmd to split, the physical page has to be still
> > compound and huge and as long as that's the case the tail pages
> > PG_dirty bit is meaningless (even if set, it's going to be clobbered
> > during the physical split).
> 
> I got it during reviewing this patch. That's why I didn't argue
> this patch would break MADV_FREE by deleting routine which propagate
> dirty to pte of subpages. However, although it's useless, I prefer
> not removing the transfer of dirty bit. Because it would help MADV_FREE
> users who want to use smaps to know how many of pages are not freeable
> (i.e, dirtied) since MADV_FREE although it is not 100% correct.
> 
> > 
> > In short PG_dirty is only meaningful in the head as long as it's
> > compound. The physical split in __split_huge_page_tail transfer the
> > head value to the tails like you mentioned, that's all as far as I can
> > tell.
> 
> Thanks for the comment. Then, this patch is to fix MADV_FREE's bug
> which has lost dirty bit by transferring dirty bit too early.

Erghh. I've misread splitting code. Yes, it's not unconditional. So we fix
actual bug.

But I'm not sure it's subject for -stable. I haven't seen any bug reports
that can be attributed to the bug.

-- 
 Kirill A. Shutemov

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

#1667650

"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
> to transfer dirty and accessed bits.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  fs/proc/task_mmu.c |  8 ++++----
>  mm/huge_memory.c   | 29 ++++++++++++-----------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f0c8b33d99b1..f2fc1ef5bba2 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c

.....

> @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	page_ref_add(page, HPAGE_PMD_NR - 1);
>  	write = pmd_write(*pmd);
>  	young = pmd_young(*pmd);
> -	dirty = pmd_dirty(*pmd);
>  	soft_dirty = pmd_soft_dirty(*pmd);
>
>  	pmdp_huge_split_prepare(vma, haddr, pmd);
> @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			if (soft_dirty)
>  				entry = pte_mksoft_dirty(entry);
>  		}
> -		if (dirty)
> -			SetPageDirty(page + i);
>  		pte = pte_offset_map(&_pmd, addr);
>  		BUG_ON(!pte_none(*pte));
>  		set_pte_at(mm, addr, pte, entry);
> @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	 * and finally we write the non-huge version of the pmd entry with
>  	 * pmd_populate.
>  	 */
> -	pmdp_invalidate(vma, haddr, pmd);
> +	old = pmdp_invalidate(vma, haddr, pmd);
> +
> +	/*
> +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
> +	 * sure we don't race with CPU that can set the bit under us.
> +	 */
> +	if (pmd_dirty(old))
> +		SetPageDirty(page);
> +
>  	pmd_populate(mm, pmd, pgtable);
>
>  	if (freeze) {


Can we invalidate the pmd early here ? ie, do pmdp_invalidate instead of
pmdp_huge_split_prepare() ?


-aneesh

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

#1667749 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

On Fri, Jun 16, 2017 at 05:01:30PM +0530, Aneesh Kumar K.V wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> 
> > This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
> > to transfer dirty and accessed bits.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  fs/proc/task_mmu.c |  8 ++++----
> >  mm/huge_memory.c   | 29 ++++++++++++-----------------
> >  2 files changed, 16 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index f0c8b33d99b1..f2fc1ef5bba2 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> 
> .....
> 
> > @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  	page_ref_add(page, HPAGE_PMD_NR - 1);
> >  	write = pmd_write(*pmd);
> >  	young = pmd_young(*pmd);
> > -	dirty = pmd_dirty(*pmd);
> >  	soft_dirty = pmd_soft_dirty(*pmd);
> >
> >  	pmdp_huge_split_prepare(vma, haddr, pmd);
> > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  			if (soft_dirty)
> >  				entry = pte_mksoft_dirty(entry);
> >  		}
> > -		if (dirty)
> > -			SetPageDirty(page + i);
> >  		pte = pte_offset_map(&_pmd, addr);
> >  		BUG_ON(!pte_none(*pte));
> >  		set_pte_at(mm, addr, pte, entry);
> > @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  	 * and finally we write the non-huge version of the pmd entry with
> >  	 * pmd_populate.
> >  	 */
> > -	pmdp_invalidate(vma, haddr, pmd);
> > +	old = pmdp_invalidate(vma, haddr, pmd);
> > +
> > +	/*
> > +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
> > +	 * sure we don't race with CPU that can set the bit under us.
> > +	 */
> > +	if (pmd_dirty(old))
> > +		SetPageDirty(page);
> > +
> >  	pmd_populate(mm, pmd, pgtable);
> >
> >  	if (freeze) {
> 
> 
> Can we invalidate the pmd early here ? ie, do pmdp_invalidate instead of
> pmdp_huge_split_prepare() ?

I think we can. But it means we would block access to the page for longer
than it's necessary on most architectures. I guess it's not a bit deal.

Maybe as separate patch on top of this patchet? Aneesh, would you take
care of this?

-- 
 Kirill A. Shutemov

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

#1667844 — Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits

On Friday 16 June 2017 06:51 PM, Kirill A. Shutemov wrote:
> On Fri, Jun 16, 2017 at 05:01:30PM +0530, Aneesh Kumar K.V wrote:
>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
>>
>>> This patch uses modifed pmdp_invalidate(), that return previous value of pmd,
>>> to transfer dirty and accessed bits.
>>>
>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> ---
>>>   fs/proc/task_mmu.c |  8 ++++----
>>>   mm/huge_memory.c   | 29 ++++++++++++-----------------
>>>   2 files changed, 16 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index f0c8b33d99b1..f2fc1ef5bba2 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>
>> .....
>>
>>> @@ -1965,7 +1955,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>   	page_ref_add(page, HPAGE_PMD_NR - 1);
>>>   	write = pmd_write(*pmd);
>>>   	young = pmd_young(*pmd);
>>> -	dirty = pmd_dirty(*pmd);
>>>   	soft_dirty = pmd_soft_dirty(*pmd);
>>>
>>>   	pmdp_huge_split_prepare(vma, haddr, pmd);
>>> @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>   			if (soft_dirty)
>>>   				entry = pte_mksoft_dirty(entry);
>>>   		}
>>> -		if (dirty)
>>> -			SetPageDirty(page + i);
>>>   		pte = pte_offset_map(&_pmd, addr);
>>>   		BUG_ON(!pte_none(*pte));
>>>   		set_pte_at(mm, addr, pte, entry);
>>> @@ -2045,7 +2032,15 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>   	 * and finally we write the non-huge version of the pmd entry with
>>>   	 * pmd_populate.
>>>   	 */
>>> -	pmdp_invalidate(vma, haddr, pmd);
>>> +	old = pmdp_invalidate(vma, haddr, pmd);
>>> +
>>> +	/*
>>> +	 * Transfer dirty bit using value returned by pmd_invalidate() to be
>>> +	 * sure we don't race with CPU that can set the bit under us.
>>> +	 */
>>> +	if (pmd_dirty(old))
>>> +		SetPageDirty(page);
>>> +
>>>   	pmd_populate(mm, pmd, pgtable);
>>>
>>>   	if (freeze) {
>>
>>
>> Can we invalidate the pmd early here ? ie, do pmdp_invalidate instead of
>> pmdp_huge_split_prepare() ?
> 
> I think we can. But it means we would block access to the page for longer
> than it's necessary on most architectures. I guess it's not a bit deal.
> 
> Maybe as separate patch on top of this patchet? Aneesh, would you take
> care of this?
> 

Yes, I cam do that.

-aneesh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web