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

Groups > linux.kernel > #1670218

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

Show all headers | View raw

Hello Kirill,

On Mon, Jun 19, 2017 at 05:03:23PM +0300, Kirill A. Shutemov wrote:
> 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.

Okay, I'm not against but please rewrite changelog to indicate it fixes
the problem. One more thing, as I mentioned, I don't want to remove
pmd dirty bit -> PG_dirty propagate to subpage part because it would be
helpful for MADV_FREE users.

Thanks.

> 
> -- 
>  Kirill A. Shutemov

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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 Minchan Kim <minchan@kernel.org> - 2017-06-20 05:00 +0200
                Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to  track dirty/accessed bits Minchan Kim <minchan@kernel.org> - 2017-06-20 12:00 +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

csiph-web